From 3695adeb227813d96d9c41850703fb53a23845eb Mon Sep 17 00:00:00 2001 From: Sven Dowideit Date: Sat, 3 Oct 2015 22:42:55 +1000 Subject: [PATCH 01/14] Add a link to the lwn.net article Signed-off-by: Sven Dowideit Signed-off-by: Will Deacon --- README | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README b/README index 6667f23..5501f05 100644 --- a/README +++ b/README @@ -97,6 +97,9 @@ project: http://thread.gmane.org/gmane.linux.kernel/962051/focus=962620 +Another detailed example can be found in the lwn.net article: + +http://lwn.net/Articles/658511/ Contributing ------------ From 8a7163f3dbc77b3734ccf7cee0b79119379a2a2d Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Mon, 19 Oct 2015 16:23:39 +0200 Subject: [PATCH 02/14] kvmtool/run: append cfg.kernel_cmdline at the end of real_cmdline This allows the user to always override the paramaters set by lkvm. Say, currently 'lkvm run -p ro' doesn't work. To keep the current logic we need to change strstr("root=") to check cfg.kernel_cmdline, not real_cmdline. And perhaps we can even add a simple helper add_param(name, val) to make this all more consistent; it should only append "name=val" to real_cmdline if cfg.kernel_cmdline doesn't include this paramater. Signed-off-by: Oleg Nesterov Signed-off-by: Will Deacon --- builtin-run.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/builtin-run.c b/builtin-run.c index e0c8732..aa5eaad 100644 --- a/builtin-run.c +++ b/builtin-run.c @@ -566,12 +566,6 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv) memset(real_cmdline, 0, sizeof(real_cmdline)); kvm__arch_set_cmdline(real_cmdline, kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk); - if (strlen(real_cmdline) > 0) - strcat(real_cmdline, " "); - - if (kvm->cfg.kernel_cmdline) - strlcat(real_cmdline, kvm->cfg.kernel_cmdline, sizeof(real_cmdline)); - if (!kvm->cfg.guest_name) { if (kvm->cfg.custom_rootfs) { kvm->cfg.guest_name = kvm->cfg.custom_rootfs_name; @@ -607,10 +601,15 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv) if (kvm_setup_guest_init(kvm->cfg.custom_rootfs_name)) die("Failed to setup init for guest."); } - } else if (!strstr(real_cmdline, "root=")) { + } else if (!strstr(kvm->cfg.kernel_cmdline, "root=")) { strlcat(real_cmdline, " root=/dev/vda rw ", sizeof(real_cmdline)); } + if (kvm->cfg.kernel_cmdline) { + strcat(real_cmdline, " "); + strlcat(real_cmdline, kvm->cfg.kernel_cmdline, sizeof(real_cmdline)); + } + kvm->cfg.real_cmdline = real_cmdline; printf(" # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME, From 4d7f252f21108371553c2a3904195bdf98e94e04 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Mon, 19 Oct 2015 16:28:53 +0200 Subject: [PATCH 03/14] kvmtool/term: unexport term_set_tty, term_init, term_exit According to git grep they can be static. term_got_escape can be static too, and we can even move it into term_getc(). "int term_escape_char" doesn't make sense at least until we allow to redefine it, turn it into preprocessor constant. Signed-off-by: Oleg Nesterov Signed-off-by: Will Deacon --- include/kvm/term.h | 3 --- term.c | 15 ++++++++------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/include/kvm/term.h b/include/kvm/term.h index dc9882e..b9793f0 100644 --- a/include/kvm/term.h +++ b/include/kvm/term.h @@ -18,9 +18,6 @@ int term_putc(char *addr, int cnt, int term); int term_getc(struct kvm *kvm, int term); bool term_readable(int term); -void term_set_tty(int term); -int term_init(struct kvm *kvm); -int term_exit(struct kvm *kvm); int tty_parser(const struct option *opt, const char *arg, int unset); #endif /* KVM__TERM_H */ diff --git a/term.c b/term.c index 9763211..aebd174 100644 --- a/term.c +++ b/term.c @@ -19,15 +19,16 @@ static struct termios orig_term; -int term_escape_char = 0x01; /* ctrl-a is used for escape */ -bool term_got_escape = false; - -int term_fds[TERM_MAX_DEVS][2]; +static int term_fds[TERM_MAX_DEVS][2]; static pthread_t term_poll_thread; +/* ctrl-a is used for escape */ +#define term_escape_char 0x01 + int term_getc(struct kvm *kvm, int term) { + static bool term_got_escape = false; unsigned char c; if (read_in_full(term_fds[term][TERM_FD_IN], &c, 1) < 0) @@ -137,7 +138,7 @@ static void term_sig_cleanup(int sig) raise(sig); } -void term_set_tty(int term) +static void term_set_tty(int term) { struct termios orig_term; int master, slave; @@ -167,7 +168,7 @@ int tty_parser(const struct option *opt, const char *arg, int unset) return 0; } -int term_init(struct kvm *kvm) +static int term_init(struct kvm *kvm) { struct termios term; int i, r; @@ -204,7 +205,7 @@ int term_init(struct kvm *kvm) } dev_init(term_init); -int term_exit(struct kvm *kvm) +static int term_exit(struct kvm *kvm) { return 0; } From a44c32936dffd6d5b01abf2fbf8e9f1632f3273d Mon Sep 17 00:00:00 2001 From: Sasha Levin Date: Mon, 19 Oct 2015 10:40:49 -0400 Subject: [PATCH 04/14] kvmtool: correct order of the vcpu destructor The vcpu module is a core component which should be removed last, but the destructor was mistakenly marked as something that should be done first. This would cause the vcpu data to be freed up before anything else had the chance to exit, and assuming that that data was still valid - causing use after frees. Reported-by: Dmitry Vyukov Signed-off-by: Sasha Levin Signed-off-by: Will Deacon --- kvm-cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kvm-cpu.c b/kvm-cpu.c index 664795f..ad4441b 100644 --- a/kvm-cpu.c +++ b/kvm-cpu.c @@ -266,4 +266,4 @@ int kvm_cpu__exit(struct kvm *kvm) return r; } -late_exit(kvm_cpu__exit); +core_exit(kvm_cpu__exit); From 26e94dc4257933b7c03b4da4a90c156e5a1d38fd Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Mon, 19 Oct 2015 12:59:42 +0200 Subject: [PATCH 05/14] kvmtool/setup: Introduce extract_file() helper Turn kvm_setup_guest_init(guestfs_name) into a more generic helper, extract_file(guestfs_name, filename, data, size) and reimplement kvm_setup_guest_init() as a trivial wrapper. Acked-by: Pekka Enberg Signed-off-by: Oleg Nesterov Signed-off-by: Will Deacon --- builtin-setup.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/builtin-setup.c b/builtin-setup.c index 40fef15..bb7c71c 100644 --- a/builtin-setup.c +++ b/builtin-setup.c @@ -122,30 +122,34 @@ static const char *guestfs_symlinks[] = { }; #ifdef CONFIG_GUEST_INIT -extern char _binary_guest_init_start; -extern char _binary_guest_init_size; - -int kvm_setup_guest_init(const char *guestfs_name) +static int extract_file(const char *guestfs_name, const char *filename, + const void *data, const void *_size) { char path[PATH_MAX]; - size_t size; int fd, ret; - char *data; - size = (size_t)&_binary_guest_init_size; - data = (char *)&_binary_guest_init_start; - snprintf(path, PATH_MAX, "%s%s/virt/init", kvm__get_dir(), guestfs_name); + snprintf(path, PATH_MAX, "%s%s/%s", kvm__get_dir(), + guestfs_name, filename); remove(path); fd = open(path, O_CREAT | O_WRONLY, 0755); if (fd < 0) die("Fail to setup %s", path); - ret = xwrite(fd, data, size); + ret = xwrite(fd, data, (size_t)_size); if (ret < 0) die("Fail to setup %s", path); close(fd); return 0; +} +extern char _binary_guest_init_start; +extern char _binary_guest_init_size; + +int kvm_setup_guest_init(const char *guestfs_name) +{ + return extract_file(guestfs_name, "virt/init", + &_binary_guest_init_start, + &_binary_guest_init_size); } #else int kvm_setup_guest_init(const char *guestfs_name) From 0f04bdf429a732f37180aa7411ec87c4197e8990 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Mon, 19 Oct 2015 12:59:45 +0200 Subject: [PATCH 06/14] kvmtool/build: introduce GUEST_PRE_INIT target This comes as a separate patch because I do not really understand /usr/bin/make, probably it should be updated. Change the main Makefile so that if an arch defines ARCH_PRE_INIT then we - build $GUEST_INIT without "-static" - add -DCONFIG_GUEST_PRE_INIT to $CFLAGS - build $ARCH_PRE_INIT as guest/guest_pre_init.o and embed it into lkvm the same as we do with guest/guest_init.o This also means that ARCH_PRE_INIT case doesn't depend on glibc-static, we can relax the SOURCE_STATIC check later. Acked-by: Pekka Enberg Signed-off-by: Oleg Nesterov Signed-off-by: Will Deacon --- .gitignore | 1 + Makefile | 25 ++++++++++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index f10d3c5..697a63f 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,7 @@ include/common-cmds.h tests/boot/boot_test.iso tests/boot/rootfs/ guest/init +guest/pre_init guest/init_stage2 KVMTOOLS-VERSION-FILE /x86/bios/bios.bin diff --git a/Makefile b/Makefile index f1701aa..0f8e003 100644 --- a/Makefile +++ b/Makefile @@ -281,6 +281,14 @@ ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y) CFLAGS += -DCONFIG_GUEST_INIT GUEST_INIT := guest/init GUEST_OBJS = guest/guest_init.o + ifeq ($(ARCH_PRE_INIT),) + GUEST_INIT_FLAGS += -static + else + CFLAGS += -DCONFIG_GUEST_PRE_INIT + GUEST_INIT_FLAGS += -DCONFIG_GUEST_PRE_INIT + GUEST_PRE_INIT := guest/pre_init + GUEST_OBJS += guest/guest_pre_init.o + endif else $(warning No static libc found. Skipping guest init) NOTFOUND += static-libc @@ -346,7 +354,7 @@ ifneq ($(WERROR),0) CFLAGS += -Werror endif -all: $(PROGRAM) $(PROGRAM_ALIAS) $(GUEST_INIT) +all: $(PROGRAM) $(PROGRAM_ALIAS) $(GUEST_INIT) $(GUEST_PRE_INIT) # CFLAGS used when building objects # This is intentionally not assigned using := @@ -360,11 +368,11 @@ c_flags = -Wp,-MD,$(depfile) $(CFLAGS) # STATIC_OBJS = $(patsubst %.o,%.static.o,$(OBJS) $(OBJS_STATOPT)) -$(PROGRAM)-static: $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_INIT) +$(PROGRAM)-static: $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_INIT) $(GUEST_PRE_INIT) $(E) " LINK " $@ $(Q) $(CC) -static $(CFLAGS) $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_OBJS) $(LIBS) $(LIBS_STATOPT) -o $@ -$(PROGRAM): $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_INIT) +$(PROGRAM): $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_INIT) $(GUEST_PRE_INIT) $(E) " LINK " $@ $(Q) $(CC) $(CFLAGS) $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_OBJS) $(LIBS) $(LIBS_DYNOPT) -o $@ @@ -372,9 +380,16 @@ $(PROGRAM_ALIAS): $(PROGRAM) $(E) " LN " $@ $(Q) ln -f $(PROGRAM) $@ +ifneq ($(ARCH_PRE_INIT),) +$(GUEST_PRE_INIT): $(ARCH_PRE_INIT) + $(E) " LINK " $@ + $(Q) $(CC) -s -nostdlib $(ARCH_PRE_INIT) -o $@ + $(Q) $(LD) $(LDFLAGS) -r -b binary -o guest/guest_pre_init.o $(GUEST_PRE_INIT) +endif + $(GUEST_INIT): guest/init.c $(E) " LINK " $@ - $(Q) $(CC) -static guest/init.c -o $@ + $(Q) $(CC) $(GUEST_INIT_FLAGS) guest/init.c -o $@ $(Q) $(LD) $(LDFLAGS) -r -b binary -o guest/guest_init.o $(GUEST_INIT) %.s: %.c @@ -473,7 +488,7 @@ clean: $(Q) rm -f x86/bios/bios-rom.h $(Q) rm -f tests/boot/boot_test.iso $(Q) rm -rf tests/boot/rootfs/ - $(Q) rm -f $(DEPS) $(OBJS) $(OTHEROBJS) $(OBJS_DYNOPT) $(STATIC_OBJS) $(PROGRAM) $(PROGRAM_ALIAS) $(PROGRAM)-static $(GUEST_INIT) $(GUEST_OBJS) + $(Q) rm -f $(DEPS) $(OBJS) $(OTHEROBJS) $(OBJS_DYNOPT) $(STATIC_OBJS) $(PROGRAM) $(PROGRAM_ALIAS) $(PROGRAM)-static $(GUEST_INIT) $(GUEST_PRE_INIT) $(GUEST_OBJS) $(Q) rm -f cscope.* $(Q) rm -f tags $(Q) rm -f TAGS From 5614f9d933c9fac94886fe359f807a60acda3fa4 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Mon, 19 Oct 2015 12:59:48 +0200 Subject: [PATCH 07/14] kvmtool/x86: implement guest_pre_init Add the tiny x86/init.S which just mounts /host and execs /virt/init. NOTE: of course, the usage of CONFIG_GUEST_PRE_INIT is ugly, we need to cleanup this code. But I'd prefer to do this on top of this minimal/simple change. And I think this needs cleanups in any case, for example I think lkvm shouldn't abuse the "init=" kernel parameter at all. Acked-by: Pekka Enberg Signed-off-by: Oleg Nesterov Signed-off-by: Will Deacon --- Makefile | 1 + builtin-run.c | 4 ++++ builtin-setup.c | 14 +++++++++++++- guest/init.c | 2 ++ x86/init.S | 38 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 x86/init.S diff --git a/Makefile b/Makefile index 0f8e003..f8f7cc4 100644 --- a/Makefile +++ b/Makefile @@ -110,6 +110,7 @@ endif ifeq ($(ARCH),x86_64) ARCH := x86 DEFINES += -DCONFIG_X86_64 + ARCH_PRE_INIT = x86/init.S endif ### Arch-specific stuff diff --git a/builtin-run.c b/builtin-run.c index aa5eaad..ab1fc2a 100644 --- a/builtin-run.c +++ b/builtin-run.c @@ -594,7 +594,11 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv) if (kvm->cfg.custom_rootfs) { kvm_run_set_sandbox(kvm); +#ifdef CONFIG_GUEST_PRE_INIT + strcat(real_cmdline, " init=/virt/pre_init"); +#else strcat(real_cmdline, " init=/virt/init"); +#endif if (!kvm->cfg.no_dhcp) strcat(real_cmdline, " ip=dhcp"); diff --git a/builtin-setup.c b/builtin-setup.c index bb7c71c..1e5b1e4 100644 --- a/builtin-setup.c +++ b/builtin-setup.c @@ -144,12 +144,24 @@ static int extract_file(const char *guestfs_name, const char *filename, extern char _binary_guest_init_start; extern char _binary_guest_init_size; +extern char _binary_guest_pre_init_start; +extern char _binary_guest_pre_init_size; int kvm_setup_guest_init(const char *guestfs_name) { - return extract_file(guestfs_name, "virt/init", + int err; + +#ifdef CONFIG_GUEST_PRE_INIT + err = extract_file(guestfs_name, "virt/pre_init", + &_binary_guest_pre_init_start, + &_binary_guest_pre_init_size); + if (err) + return err; +#endif + err = extract_file(guestfs_name, "virt/init", &_binary_guest_init_start, &_binary_guest_init_size); + return err; } #else int kvm_setup_guest_init(const char *guestfs_name) diff --git a/guest/init.c b/guest/init.c index 7277a07..46e3fa4 100644 --- a/guest/init.c +++ b/guest/init.c @@ -30,7 +30,9 @@ static int run_process_sandbox(char *filename) static void do_mounts(void) { +#ifndef CONFIG_GUEST_PRE_INIT mount("hostfs", "/host", "9p", MS_RDONLY, "trans=virtio,version=9p2000.L"); +#endif mount("", "/sys", "sysfs", 0, NULL); mount("proc", "/proc", "proc", 0, NULL); mount("devtmpfs", "/dev", "devtmpfs", 0, NULL); diff --git a/x86/init.S b/x86/init.S new file mode 100644 index 0000000..488a93f --- /dev/null +++ b/x86/init.S @@ -0,0 +1,38 @@ +.data + +.m_dev: +.string "hostfs" +.m_dir: +.string "/host" +.m_typ: +.string "9p" +.m_opt: +.string "trans=virtio,version=9p2000.L" + +.e_nam: +.string "/virt/init" + +.text +.globl _start +_start: + + mov $165, %rax # __NR_mount + mov $.m_dev, %rdi + mov $.m_dir, %rsi + mov $.m_typ, %rdx + mov $1, %r10 # MS_RDONLY + mov $.m_opt, %r8 + syscall + + mov $59, %rax # __NR_execve + mov $.e_nam, %rdi + lea 8(%rsp), %rsi # argv[] + mov %rdi, (%rsi) # change argv[0] + pop %rcx # argc + inc %rcx + lea (%rsi,%rcx,8), %rdx # envp[] + syscall + + mov $60, %rax # __NR_exit + mov $1, %rdi + syscall # panic From f8a7e35cbb1613bfe1d4b322cf40405e18bd960e Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Thu, 22 Oct 2015 17:59:40 +0200 Subject: [PATCH 08/14] kvmtool: add lkvm-static to gitignore add lkvm-static to gitignore Signed-off-by: Oleg Nesterov Signed-off-by: Will Deacon --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 697a63f..a16a97f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ /lkvm +/lkvm-static /vm *.o *.d From 1c2a21f054060228e42cdc935275cdd57ca37709 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Thu, 22 Oct 2015 17:59:42 +0200 Subject: [PATCH 09/14] kvmtool/run: don't abuse "root=" parameter, don't pass "rw" to v9fs_mount() 1. kvm_cmd_run_init() appends "root=/dev/root" to real_cmdline if cfg.using_rootfs == T. This doesn't hurt but makes no sense and looks confusing. We do not need to initialiaze the kernel's saved_root_name[] and "/dev/root" means nothing to name_to_dev_t(). We only need to pass this mount-tag to 9p but the kernel always uses dev_name="/dev/root" in mount_root() path, so we can safely remove this option from the command line. 2. "rw" in rootflags looks confusing too, it is silently ignored by v9fs_parse_options() and has no effect. We need to clear MS_RDONLY from root_mountflags, this is what the "standalone" kernel parameter correctly does. Signed-off-by: Oleg Nesterov Signed-off-by: Will Deacon --- builtin-run.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin-run.c b/builtin-run.c index ab1fc2a..6e4491c 100644 --- a/builtin-run.c +++ b/builtin-run.c @@ -590,7 +590,7 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv) } if (kvm->cfg.using_rootfs) { - strcat(real_cmdline, " root=/dev/root rw rootflags=rw,trans=virtio,version=9p2000.L rootfstype=9p"); + strcat(real_cmdline, " rw rootflags=trans=virtio,version=9p2000.L rootfstype=9p"); if (kvm->cfg.custom_rootfs) { kvm_run_set_sandbox(kvm); From 30867c55012f05f3ad28322bcf86dcfb83b9f21b Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Thu, 22 Oct 2015 17:59:45 +0200 Subject: [PATCH 10/14] kvmtool/run: do not overwrite /virt/init To me kvm_setup_guest_init() behaviour looks "obviously wrong" and unfriendly because it always overwrites /virt/init. kvm_setup_guest_init() is also called when we are going to use this tree as a rootfs, and without another patch ("kvmtool/run: append cfg.kernel_cmdline at the end of real_cmdline") the user can't use "lkvm run -p init=my_init_path". This simply means that you can not use a customized init unless you patch kvmtool. Change extract_file() to do nothing if the file already exists. This should not affect do_setup() which calls kvm_setup_guest_init() only if make_dir(guestfs_name) creates the new/empty dir. Signed-off-by: Oleg Nesterov Signed-off-by: Will Deacon --- builtin-setup.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/builtin-setup.c b/builtin-setup.c index 1e5b1e4..8be8d62 100644 --- a/builtin-setup.c +++ b/builtin-setup.c @@ -130,10 +130,14 @@ static int extract_file(const char *guestfs_name, const char *filename, snprintf(path, PATH_MAX, "%s%s/%s", kvm__get_dir(), guestfs_name, filename); - remove(path); - fd = open(path, O_CREAT | O_WRONLY, 0755); - if (fd < 0) + + fd = open(path, O_EXCL | O_CREAT | O_WRONLY, 0755); + if (fd < 0) { + if (errno == EEXIST) + return 0; die("Fail to setup %s", path); + } + ret = xwrite(fd, data, (size_t)_size); if (ret < 0) die("Fail to setup %s", path); From 6d7eeb7a1328fcce82b5783d9e4605bf5e4737dd Mon Sep 17 00:00:00 2001 From: Sasha Levin Date: Tue, 20 Oct 2015 23:32:33 -0400 Subject: [PATCH 11/14] kvmtool: set 9p caching mode to support writable mmaps 9p doesn't support writable mmaps by default (when cache=none), set it to loose caching to allow for writable mmaps. Reported-by: Dmitry Vyukov Signed-off-by: Sasha Levin Signed-off-by: Will Deacon --- builtin-run.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin-run.c b/builtin-run.c index 6e4491c..d7d3afd 100644 --- a/builtin-run.c +++ b/builtin-run.c @@ -590,7 +590,7 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv) } if (kvm->cfg.using_rootfs) { - strcat(real_cmdline, " rw rootflags=trans=virtio,version=9p2000.L rootfstype=9p"); + strcat(real_cmdline, " rw rootflags=trans=virtio,version=9p2000.L,cache=loose rootfstype=9p"); if (kvm->cfg.custom_rootfs) { kvm_run_set_sandbox(kvm); From d583df25ed1efca38c629612e3c5f703db41b2ad Mon Sep 17 00:00:00 2001 From: William Dauchy Date: Sat, 31 Oct 2015 14:11:29 +0100 Subject: [PATCH 12/14] kvmtool/run: set a default cmdline if not set when starting with custom kernel and disk options, kernel_cmdline is NULL; it results in a segfault while trying to look for a string using `strstr`: __strstr_sse2_unaligned () at ../sysdeps/x86_64/multiarch/strstr-sse2-unaligned.S:40 0x00000000004056bf in kvm_cmd_run_init (argc=, argv=) at builtin-run.c:608 0x000000000040639d in kvm_cmd_run (argc=, argv=, prefix=) at builtin-run.c:659 0x0000000000412b8f in handle_command (command=0x62bbc0 , argc=5, argv=0x7fffffffe840) at kvm-cmd.c:84 0x00007ffff7211b45 in __libc_start_main (main=0x403540
, argc=6, argv=0x7fffffffe838, init=, fini=, rtld_fini=, stack_end=0x7fffffffe828) at libc-start.c:287 0x0000000000403962 in _start () this patch suggests to set a minimal cmdline when kernel_cmdline is NULL Fixes: 8a7163f3dbc7 ("kvmtool/run: append cfg.kernel_cmdline at the end of real_cmdline") Signed-off-by: William Dauchy Signed-off-by: Will Deacon --- builtin-run.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin-run.c b/builtin-run.c index d7d3afd..edcaf3e 100644 --- a/builtin-run.c +++ b/builtin-run.c @@ -605,7 +605,7 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv) if (kvm_setup_guest_init(kvm->cfg.custom_rootfs_name)) die("Failed to setup init for guest."); } - } else if (!strstr(kvm->cfg.kernel_cmdline, "root=")) { + } else if (!kvm->cfg.kernel_cmdline || !strstr(kvm->cfg.kernel_cmdline, "root=")) { strlcat(real_cmdline, " root=/dev/vda rw ", sizeof(real_cmdline)); } From d0e2772b93abcc8a66f83ed8ed248c94adabce4b Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Fri, 30 Oct 2015 17:20:49 +0000 Subject: [PATCH 13/14] Makefile: allow overriding CFLAGS on the command line When a Makefile variable is set on the make command line, all Makefile-internal assignments to that very variable are _ignored_. Since we add quite some essential values to CFLAGS internally, specifying some CFLAGS on the command line will usually break the build (and not fix any include file problems you hoped to overcome with that). Somewhat against intuition GNU make provides the "override" directive to change this behavior; with that assignments in the Makefile get _appended_ to the value given on the command line. [1] Change any internal assignments to use that directive, so that a user can use: $ make CFLAGS=/path/to/my/include/dir to teach kvmtool about non-standard header file locations (helpful for cross-compilation) or to tweak other compiler options. Signed-off-by: Andre Przywara [1] https://www.gnu.org/software/make/manual/html_node/Override-Directive.html Signed-off-by: Will Deacon --- Makefile | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index f8f7cc4..77a7c9f 100644 --- a/Makefile +++ b/Makefile @@ -15,9 +15,7 @@ include config/utilities.mak include config/feature-tests.mak CC := $(CROSS_COMPILE)gcc -CFLAGS := LD := $(CROSS_COMPILE)ld -LDFLAGS := FIND := find CSCOPE := cscope @@ -162,7 +160,7 @@ ifeq ($(ARCH), arm) OBJS += arm/aarch32/kvm-cpu.o ARCH_INCLUDE := $(HDRS_ARM_COMMON) ARCH_INCLUDE += -Iarm/aarch32/include - CFLAGS += -march=armv7-a + override CFLAGS += -march=armv7-a ARCH_WANT_LIBFDT := y endif @@ -274,12 +272,12 @@ endif ifeq ($(LTO),1) FLAGS_LTO := -flto ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(FLAGS_LTO)),y) - CFLAGS += $(FLAGS_LTO) + override CFLAGS += $(FLAGS_LTO) endif endif ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y) - CFLAGS += -DCONFIG_GUEST_INIT + override CFLAGS += -DCONFIG_GUEST_INIT GUEST_INIT := guest/init GUEST_OBJS = guest/guest_init.o ifeq ($(ARCH_PRE_INIT),) @@ -331,7 +329,8 @@ DEFINES += -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"' DEFINES += -DBUILD_ARCH='"$(ARCH)"' KVM_INCLUDE := include -CFLAGS += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g +override CFLAGS += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) +override CFLAGS += -O2 -fno-strict-aliasing -g WARNINGS += -Wall WARNINGS += -Wformat=2 @@ -349,10 +348,10 @@ WARNINGS += -Wvolatile-register-var WARNINGS += -Wwrite-strings WARNINGS += -Wno-format-nonliteral -CFLAGS += $(WARNINGS) +override CFLAGS += $(WARNINGS) ifneq ($(WERROR),0) - CFLAGS += -Werror + override CFLAGS += -Werror endif all: $(PROGRAM) $(PROGRAM_ALIAS) $(GUEST_INIT) $(GUEST_PRE_INIT) From 03c49af7fd78e29a369a540c0c4cd9cf6b385259 Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Fri, 30 Oct 2015 17:20:50 +0000 Subject: [PATCH 14/14] Makefile: consider LDFLAGS on feature tests and when linking executables While we have an LDFLAGS variable in kvmtool's Makefile, it's not really used when both doing the feature tests and when finally linking the lkvm executable. Add that variable to all the linking steps to allow the user to specify custom library directories or linker options on the command line. Signed-off-by: Andre Przywara Signed-off-by: Will Deacon --- Makefile | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index 77a7c9f..eac1220 100644 --- a/Makefile +++ b/Makefile @@ -196,12 +196,12 @@ endif # On a given system, some libs may link statically, some may not; so, check # both and only build those that link! -ifeq ($(call try-build,$(SOURCE_STRLCPY),$(CFLAGS),),y) +ifeq ($(call try-build,$(SOURCE_STRLCPY),$(CFLAGS),$(LDFLAGS)),y) CFLAGS_DYNOPT += -DHAVE_STRLCPY CFLAGS_STATOPT += -DHAVE_STRLCPY endif -ifeq ($(call try-build,$(SOURCE_BFD),$(CFLAGS),-lbfd -static),y) +ifeq ($(call try-build,$(SOURCE_BFD),$(CFLAGS),$(LDFLAGS) -lbfd -static),y) CFLAGS_STATOPT += -DCONFIG_HAS_BFD OBJS_STATOPT += symbol.o LIBS_STATOPT += -lbfd @@ -212,7 +212,7 @@ endif ifeq (y,$(ARCH_HAS_FRAMEBUFFER)) CFLAGS_GTK3 := $(shell pkg-config --cflags gtk+-3.0 2>/dev/null) LDFLAGS_GTK3 := $(shell pkg-config --libs gtk+-3.0 2>/dev/null) - ifeq ($(call try-build,$(SOURCE_GTK3),$(CFLAGS) $(CFLAGS_GTK3),$(LDFLAGS_GTK3)),y) + ifeq ($(call try-build,$(SOURCE_GTK3),$(CFLAGS) $(CFLAGS_GTK3),$(LDFLAGS) $(LDFLAGS_GTK3)),y) OBJS_DYNOPT += ui/gtk3.o CFLAGS_DYNOPT += -DCONFIG_HAS_GTK3 $(CFLAGS_GTK3) LIBS_DYNOPT += $(LDFLAGS_GTK3) @@ -220,63 +220,63 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER)) NOTFOUND += GTK3 endif - ifeq ($(call try-build,$(SOURCE_VNCSERVER),$(CFLAGS),-lvncserver),y) + ifeq ($(call try-build,$(SOURCE_VNCSERVER),$(CFLAGS),$(LDFLAGS) -lvncserver),y) OBJS_DYNOPT += ui/vnc.o CFLAGS_DYNOPT += -DCONFIG_HAS_VNCSERVER LIBS_DYNOPT += -lvncserver else NOTFOUND += vncserver endif - ifeq ($(call try-build,$(SOURCE_VNCSERVER),$(CFLAGS),-lvncserver -static),y) + ifeq ($(call try-build,$(SOURCE_VNCSERVER),$(CFLAGS),$(LDFLAGS) -lvncserver -static),y) OBJS_STATOPT += ui/vnc.o CFLAGS_STATOPT += -DCONFIG_HAS_VNCSERVER LIBS_STATOPT += -lvncserver endif - ifeq ($(call try-build,$(SOURCE_SDL),$(CFLAGS),-lSDL),y) + ifeq ($(call try-build,$(SOURCE_SDL),$(CFLAGS),$(LDFLAGS) -lSDL),y) OBJS_DYNOPT += ui/sdl.o CFLAGS_DYNOPT += -DCONFIG_HAS_SDL LIBS_DYNOPT += -lSDL else NOTFOUND += SDL endif - ifeq ($(call try-build,$(SOURCE_SDL),$(CFLAGS),-lSDL -static), y) + ifeq ($(call try-build,$(SOURCE_SDL),$(CFLAGS),$(LDFLAGS) -lSDL -static), y) OBJS_STATOPT += ui/sdl.o CFLAGS_STATOPT += -DCONFIG_HAS_SDL LIBS_STATOPT += -lSDL endif endif -ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),-lz),y) +ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y) CFLAGS_DYNOPT += -DCONFIG_HAS_ZLIB LIBS_DYNOPT += -lz else NOTFOUND += zlib endif -ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),-lz -static),y) +ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz -static),y) CFLAGS_STATOPT += -DCONFIG_HAS_ZLIB LIBS_STATOPT += -lz endif -ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),-laio),y) +ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio),y) CFLAGS_DYNOPT += -DCONFIG_HAS_AIO LIBS_DYNOPT += -laio else NOTFOUND += aio endif -ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),-laio -static),y) +ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio -static),y) CFLAGS_STATOPT += -DCONFIG_HAS_AIO LIBS_STATOPT += -laio endif ifeq ($(LTO),1) FLAGS_LTO := -flto - ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(FLAGS_LTO)),y) + ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(LDFLAGS) $(FLAGS_LTO)),y) override CFLAGS += $(FLAGS_LTO) endif endif -ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y) +ifeq ($(call try-build,$(SOURCE_STATIC),$(CFLAGS),$(LDFLAGS) -static),y) override CFLAGS += -DCONFIG_GUEST_INIT GUEST_INIT := guest/init GUEST_OBJS = guest/guest_init.o @@ -370,11 +370,11 @@ STATIC_OBJS = $(patsubst %.o,%.static.o,$(OBJS) $(OBJS_STATOPT)) $(PROGRAM)-static: $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_INIT) $(GUEST_PRE_INIT) $(E) " LINK " $@ - $(Q) $(CC) -static $(CFLAGS) $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_OBJS) $(LIBS) $(LIBS_STATOPT) -o $@ + $(Q) $(CC) -static $(CFLAGS) $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_OBJS) $(LDFLAGS) $(LIBS) $(LIBS_STATOPT) -o $@ $(PROGRAM): $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_INIT) $(GUEST_PRE_INIT) $(E) " LINK " $@ - $(Q) $(CC) $(CFLAGS) $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_OBJS) $(LIBS) $(LIBS_DYNOPT) -o $@ + $(Q) $(CC) $(CFLAGS) $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_OBJS) $(LDFLAGS) $(LIBS) $(LIBS_DYNOPT) -o $@ $(PROGRAM_ALIAS): $(PROGRAM) $(E) " LN " $@