[PATCH kvmtool 00/10] Run kvm-unit-tests with --kernel

[PATCH kvmtool 00/10] Run kvm-unit-tests with --kernel

[Alexandru Elisei]

What prompted this series (which I really hoped will turn out smaller than
it did) is my attempt to add support for kvmtool to kvm-unit-tests
automated test runner [1]. When working through the review comments for
that series, I realized that kvmtool must be able to load an initrd when
running a test to get all the features that tests rely on.

kvm-unit-tests uses the initrd, which is expected to be a text file in the
format key=value, to pass parameters to a test. The initrd is by default
generated by the runner script, but the location of a custom initrd file
can also be set using the environment variable KVM_UNIT_TEST_ENV (many
thanks to Andrew Jones for explaining that). Contained in the automatically
generated initrd is information about the presence of certain commits in
the host kernel.  These commits are important because they fix serious bugs
in KVM, and running tests which are designed to exercise the fix on systems
where it isn't present can cause the host kernel to crash. kvm-unit-tests
calls these bug fixing commits erratas, and their presence is signalled by
an entry ERRATA_<commit_id>=y in the initrd.

Using --kernel to run a test is not possible for two reasons:

1. Commit fd0a05bd27dd ("arm64: Obtain text offset from kernel image") made
it such that the kernel load offset is read from the binary file even if
the kernel header is not detected. kvmtool will try to load a test at a
random address read from the text section of the binary, which most of the
time is well above the upper limit for the VM's RAM in a normal VM
configuration.

2. kvm-unit-tests uses the kernel command line as the source for various
test parameters. kvmtool modifies the kernel command line that the user
specified to try to make it as painless as possible to boot a kernel, but
for a test this means that parsing the kernel command line for the required
parameters fails because of those unexpected, and in this case, unwanted
additions.

Currently, running any kvm-unit-tests test with kvmtool can only be done
with --firmware, which does not touch the command line, but it has the
downside of not being able to load an initrd. I decided to add a new
kvmtool command line option, --nodefaults, which disables all the automated
stuff that kvmtool does without being explicitly told so by the user (which
includes modifying the kernel command line).

I believe a --nodefaults option has merit even outside enabling support for
automating kvm-unit-tests runs. There are legitimate reasons for a user to
remove all the parameters that kvmtool adds to the kernel command line
(like testing or trying to understand how something works), or the
generated rootfs filesystem (for example, the initrd with the rootfs is
included in the kernel). I think that giving the user as much control as
possible is very useful for a program like kvmtool, which lends itself very
well to quick testing and prototyping.

The --nocompat option was added because compat warnings, in certain
situations, can be more confusing than useful on arm64, which has virtio in
defconfig. Of course, this is under the assumption that a user who removes
virtio from the kernel config knows what he's doing, but the compat
warnings are still shown by default just in case. Also, with the --firmware
option, the assumption that every guest is a Linux kernel and has working
virtio is not really true any more.

A quick summary of the patches:

* Patches #1 through #4 are for making kvmtool's command line more
  informative when options are ignored because --kernel and --firmware are
  handled differently.

* Patch #5 removes kvm->cfg.image_count, which is really the same
  thing as kvm->nr_disks. I found this very confusing when trying to
  understand the function kvm_cmd_run_init(), but the patch is not
  necessary for what this series is trying to achieve.

* Patch #6 is a preparatory patch and #7 adds the --nodefaults option.

* Patch #8 adds the --nocompat option.

* Patch #9 and #10 is my attempt at making kvmtool slightly more lenient
  when loading something other than a kernel with --kernel. Patch #9 is
  squarely aimed at loading kvm-unit-tests with --kernel (which was possible
  before kernel header parsing, but not anymore). Patch #10 is my attempt
  at hiding a warning which is harmless in the context of loading a
  kvm-unit-tests test.

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2021-July/047747.html

Alexandru Elisei (10):
  builtin-run: Treat specifying both --kernel and --firmware as an error
  builtin-run: Warn when ignoring initrd because --firmware was
    specified
  builtin-run: Do not attempt to find vmlinux if --firmware
  builtin-run: Abstract argument validation into a separate function
  Use kvm->nr_disks instead of kvm->cfg.image_count
  builtin-run: Move kernel command line generation to a separate
    function
  Add --nodefaults command line argument
  Add --nocompat option to disable compat warnings
  arm64: Use the default offset when the kernel image magic is not found
  arm64: Be more permissive when parsing the kernel header

 arm/aarch64/kvm.c        |  18 ++---
 arm/fdt.c                |   3 +-
 builtin-run.c            | 144 ++++++++++++++++++++++++---------------
 disk/core.c              |  18 ++---
 guest_compat.c           |   1 +
 include/kvm/kvm-config.h |   3 +-
 mips/kvm.c               |   3 +-
 7 files changed, 114 insertions(+), 76 deletions(-)

-- 
2.31.1

[PATCH kvmtool 01/10] builtin-run: Treat specifying both --kernel and --firmware as an error

[Alexandru Elisei]

If the user specifies both the --kernel and the --firmware arguments,
--firmware takes precedence and --kernel is silently ignored. Since kvmtool
has no way of knowing what the user really intended, and guessing that
--firmware is the right argument might prove to be quite unexpected for the
user, be vocal about the incompatibility and refuse to create the VM.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin-run.c b/builtin-run.c
index 7f93b9d9312c..8bb8051680b1 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -513,6 +513,9 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 
 	kvm->nr_disks = kvm->cfg.image_count;
 
+	if (kvm->cfg.kernel_filename && kvm->cfg.firmware_filename)
+		die("Only one of --kernel or --firmware can be specified");
+
 	if (!kvm->cfg.kernel_filename && !kvm->cfg.firmware_filename) {
 		kvm->cfg.kernel_filename = find_kernel();
 
-- 
2.31.1

[PATCH kvmtool 02/10] builtin-run: Warn when ignoring initrd because --firmware was specified

[Alexandru Elisei]

The firmware image is copied into the guest memory with the arch specific
function kvm__load_firmware() in kvm__init(). That function ignores the
initrd file, if the user specified one. Let the user know that the file is
ignored by KVM and the --initrd argument does nothing with --firmware.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin-run.c b/builtin-run.c
index 8bb8051680b1..083c7a2abea7 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -516,6 +516,9 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 	if (kvm->cfg.kernel_filename && kvm->cfg.firmware_filename)
 		die("Only one of --kernel or --firmware can be specified");
 
+	if (kvm->cfg.firmware_filename && kvm->cfg.initrd_filename)
+		pr_warning("Ignoring initrd file when loading a firmware image");
+
 	if (!kvm->cfg.kernel_filename && !kvm->cfg.firmware_filename) {
 		kvm->cfg.kernel_filename = find_kernel();
 
-- 
2.31.1

[PATCH kvmtool 03/10] builtin-run: Do not attempt to find vmlinux if --firmware

[Alexandru Elisei]

kvm->vmlinux is used by symbol.c on x86 to translate a PC address to a
kernel symbol when kvmtool exits unexpectedly. When the --firmware argument
is used, a kernel image is not used for the VM, and the vmlinux file has no
relevance in this case.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index 083c7a2abea7..6a55e34ab7f9 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -528,8 +528,10 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 		}
 	}
 
-	kvm->cfg.vmlinux_filename = find_vmlinux();
-	kvm->vmlinux = kvm->cfg.vmlinux_filename;
+	if (kvm->cfg.kernel_filename) {
+		kvm->cfg.vmlinux_filename = find_vmlinux();
+		kvm->vmlinux = kvm->cfg.vmlinux_filename;
+	}
 
 	if (kvm->cfg.nrcpus == 0)
 		kvm->cfg.nrcpus = nr_online_cpus;
-- 
2.31.1

[PATCH kvmtool 04/10] builtin-run: Abstract argument validation into a separate function

[Alexandru Elisei]

kvm_cmd_run_init() is a complex function which parses the command line
arguments, configures various aspects of a VM (the size of the RAM, the
number of CPUs, the network, the active console, the kernel command line,
creates a custom rootfs, etc), and after the recent patches, also does a
few checks against mutually exclusive kvmtool arguments.

Make the function just that little bit easier to read by moving the
argument validation into a separate function.

No functional change intended.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index 6a55e34ab7f9..2a14723ba042 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -455,6 +455,19 @@ static void kvm_run_write_sandbox_cmd(struct kvm *kvm, const char **argv, int ar
 	close(fd);
 }
 
+static void kvm_run_validate_cfg(struct kvm *kvm)
+{
+	if (kvm->cfg.kernel_filename && kvm->cfg.firmware_filename)
+		die("Only one of --kernel or --firmware can be specified");
+
+	if ((kvm->cfg.vnc && (kvm->cfg.sdl || kvm->cfg.gtk)) ||
+	    (kvm->cfg.sdl && kvm->cfg.gtk))
+		die("Only one of --vnc, --sdl or --gtk can be specified");
+
+	if (kvm->cfg.firmware_filename && kvm->cfg.initrd_filename)
+		pr_warning("Ignoring initrd file when loading a firmware image");
+}
+
 static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 {
 	static char real_cmdline[2048], default_name[20];
@@ -511,13 +524,9 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 
 	}
 
-	kvm->nr_disks = kvm->cfg.image_count;
-
-	if (kvm->cfg.kernel_filename && kvm->cfg.firmware_filename)
-		die("Only one of --kernel or --firmware can be specified");
+	kvm_run_validate_cfg(kvm);
 
-	if (kvm->cfg.firmware_filename && kvm->cfg.initrd_filename)
-		pr_warning("Ignoring initrd file when loading a firmware image");
+	kvm->nr_disks = kvm->cfg.image_count;
 
 	if (!kvm->cfg.kernel_filename && !kvm->cfg.firmware_filename) {
 		kvm->cfg.kernel_filename = find_kernel();
@@ -552,13 +561,6 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 	if (!kvm->cfg.console)
 		kvm->cfg.console = DEFAULT_CONSOLE;
 
-	video = kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk;
-	if (video) {
-		if ((kvm->cfg.vnc && (kvm->cfg.sdl || kvm->cfg.gtk)) ||
-		    (kvm->cfg.sdl && kvm->cfg.gtk))
-			die("Only one of --vnc, --sdl or --gtk can be specified");
-	}
-
 	if (!strncmp(kvm->cfg.console, "virtio", 6))
 		kvm->cfg.active_console  = CONSOLE_VIRTIO;
 	else if (!strncmp(kvm->cfg.console, "serial", 6))
@@ -586,6 +588,8 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 	if (!kvm->cfg.network)
                 kvm->cfg.network = DEFAULT_NETWORK;
 
+	video = kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk;
+
 	memset(real_cmdline, 0, sizeof(real_cmdline));
 	kvm__arch_set_cmdline(real_cmdline, video);
 
-- 
2.31.1

[PATCH kvmtool 05/10] Use kvm->nr_disks instead of kvm->cfg.image_count

[Alexandru Elisei]

A user can specify multiple disk images using the --disk/-d argument. The
callback for the argument ends up in
disk/core.c::calling disk_img_name_parser(), which increments
kvm->cfg.image_count for each disk image.

Immediately after parsing the arguments in kvm_cmd_run_init(),
kvm->nr_disks is set to kvm->cfg.image_count, effectively making
kvm->nr_disks an alias for kvm->cfg.image_count, as image_count is never
changed afterward.

Later on, the core disk code uses kvm->cfg.image_count when opening all the
disk images, but kvm->nr_disks when closing them, which is inconsistent,
but technically correct since they represent the same thing and have the
same value.

Let's remove all this confusing usage and use only kvm->nr_disks to
represent the number of disk images specified by the user.

While this technically means that kvmtool now supports up to INT_MAX disk
images, in practice this is limited by MAX_DISK_IMAGES, which is equal to
four. Which means there are no functional changes.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c            |  2 --
 disk/core.c              | 18 +++++++++---------
 include/kvm/kvm-config.h |  1 -
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index 2a14723ba042..6822c321883e 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -526,8 +526,6 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 
 	kvm_run_validate_cfg(kvm);
 
-	kvm->nr_disks = kvm->cfg.image_count;
-
 	if (!kvm->cfg.kernel_filename && !kvm->cfg.firmware_filename) {
 		kvm->cfg.kernel_filename = find_kernel();
 
diff --git a/disk/core.c b/disk/core.c
index 8d95c98e2169..d8d04cb0a24e 100644
--- a/disk/core.c
+++ b/disk/core.c
@@ -16,20 +16,20 @@ int disk_img_name_parser(const struct option *opt, const char *arg, int unset)
 	char *sep;
 	struct kvm *kvm = opt->ptr;
 
-	if (kvm->cfg.image_count >= MAX_DISK_IMAGES)
+	if (kvm->nr_disks >= MAX_DISK_IMAGES)
 		die("Currently only 4 images are supported");
 
-	kvm->cfg.disk_image[kvm->cfg.image_count].filename = arg;
+	kvm->cfg.disk_image[kvm->nr_disks].filename = arg;
 	cur = arg;
 
 	if (strncmp(arg, "scsi:", 5) == 0) {
 		sep = strstr(arg, ":");
 		if (sep)
-			kvm->cfg.disk_image[kvm->cfg.image_count].wwpn = sep + 1;
+			kvm->cfg.disk_image[kvm->nr_disks].wwpn = sep + 1;
 		sep = strstr(sep + 1, ":");
 		if (sep) {
 			*sep = 0;
-			kvm->cfg.disk_image[kvm->cfg.image_count].tpgt = sep + 1;
+			kvm->cfg.disk_image[kvm->nr_disks].tpgt = sep + 1;
 		}
 		cur = sep + 1;
 	}
@@ -38,15 +38,15 @@ int disk_img_name_parser(const struct option *opt, const char *arg, int unset)
 		sep = strstr(cur, ",");
 		if (sep) {
 			if (strncmp(sep + 1, "ro", 2) == 0)
-				kvm->cfg.disk_image[kvm->cfg.image_count].readonly = true;
+				kvm->cfg.disk_image[kvm->nr_disks].readonly = true;
 			else if (strncmp(sep + 1, "direct", 6) == 0)
-				kvm->cfg.disk_image[kvm->cfg.image_count].direct = true;
+				kvm->cfg.disk_image[kvm->nr_disks].direct = true;
 			*sep = 0;
 			cur = sep + 1;
 		}
 	} while (sep);
 
-	kvm->cfg.image_count++;
+	kvm->nr_disks++;
 
 	return 0;
 }
@@ -152,7 +152,7 @@ static struct disk_image **disk_image__open_all(struct kvm *kvm)
 	void *err;
 	int i;
 	struct disk_image_params *params = (struct disk_image_params *)&kvm->cfg.disk_image;
-	int count = kvm->cfg.image_count;
+	int count = kvm->nr_disks;
 
 	if (!count)
 		return ERR_PTR(-EINVAL);
@@ -328,7 +328,7 @@ void disk_image__set_callback(struct disk_image *disk,
 
 int disk_image__init(struct kvm *kvm)
 {
-	if (kvm->cfg.image_count) {
+	if (kvm->nr_disks) {
 		kvm->disks = disk_image__open_all(kvm);
 		if (IS_ERR(kvm->disks))
 			return PTR_ERR(kvm->disks);
diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
index 8b6c151191f6..35d45c0f7ab1 100644
--- a/include/kvm/kvm-config.h
+++ b/include/kvm/kvm-config.h
@@ -23,7 +23,6 @@ struct kvm_config {
 	struct disk_image_params disk_image[MAX_DISK_IMAGES];
 	struct vfio_device_params *vfio_devices;
 	u64 ram_size;
-	u8  image_count;
 	u8 num_net_devices;
 	u8 num_vfio_devices;
 	u64 vsock_cid;
-- 
2.31.1

[PATCH kvmtool 06/10] builtin-run: Move kernel command line generation to a separate function

[Alexandru Elisei]

The real kernel command line is gradually generated in kvm_cmd_run_init()
and it is interspersed with the initialization code. This means that both
the code that generates the command line and the rest of the code is
unnecessarily difficult to follow and to modify. Move the code that
generates the command line to one function, to make it easier to
understand, and to declutter kvm_cmd_run_init().

No functional change intended.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c | 100 +++++++++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 46 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index 6822c321883e..478b5a95b726 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -455,6 +455,54 @@ static void kvm_run_write_sandbox_cmd(struct kvm *kvm, const char **argv, int ar
 	close(fd);
 }
 
+static void kvm_run_set_real_cmdline(struct kvm *kvm)
+{
+	static char real_cmdline[2048];
+	bool video;
+
+	video = kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk;
+
+	memset(real_cmdline, 0, sizeof(real_cmdline));
+	kvm__arch_set_cmdline(real_cmdline, video);
+
+	if (video) {
+		strcat(real_cmdline, " console=tty0");
+	} else {
+		switch (kvm->cfg.active_console) {
+		case CONSOLE_HV:
+			/* Fallthrough */
+		case CONSOLE_VIRTIO:
+			strcat(real_cmdline, " console=hvc0");
+			break;
+		case CONSOLE_8250:
+			strcat(real_cmdline, " console=ttyS0");
+			break;
+		}
+	}
+
+	if (kvm->cfg.using_rootfs) {
+		strcat(real_cmdline, " rw rootflags=trans=virtio,version=9p2000.L,cache=loose rootfstype=9p");
+		if (kvm->cfg.custom_rootfs) {
+#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");
+		}
+	} else if (!kvm->cfg.kernel_cmdline || !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;
+}
+
 static void kvm_run_validate_cfg(struct kvm *kvm)
 {
 	if (kvm->cfg.kernel_filename && kvm->cfg.firmware_filename)
@@ -470,10 +518,9 @@ static void kvm_run_validate_cfg(struct kvm *kvm)
 
 static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 {
-	static char real_cmdline[2048], default_name[20];
+	static char default_name[20];
 	unsigned int nr_online_cpus;
 	struct kvm *kvm = kvm__new();
-	bool video;
 
 	if (IS_ERR(kvm))
 		return kvm;
@@ -586,26 +633,6 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 	if (!kvm->cfg.network)
                 kvm->cfg.network = DEFAULT_NETWORK;
 
-	video = kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk;
-
-	memset(real_cmdline, 0, sizeof(real_cmdline));
-	kvm__arch_set_cmdline(real_cmdline, video);
-
-	if (video) {
-		strcat(real_cmdline, " console=tty0");
-	} else {
-		switch (kvm->cfg.active_console) {
-		case CONSOLE_HV:
-			/* Fallthrough */
-		case CONSOLE_VIRTIO:
-			strcat(real_cmdline, " console=hvc0");
-			break;
-		case CONSOLE_8250:
-			strcat(real_cmdline, " console=ttyS0");
-			break;
-		}
-	}
-
 	if (!kvm->cfg.guest_name) {
 		if (kvm->cfg.custom_rootfs) {
 			kvm->cfg.guest_name = kvm->cfg.custom_rootfs_name;
@@ -629,32 +656,13 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 		kvm->cfg.using_rootfs = kvm->cfg.custom_rootfs = 1;
 	}
 
-	if (kvm->cfg.using_rootfs) {
-		strcat(real_cmdline, " rw rootflags=trans=virtio,version=9p2000.L,cache=loose rootfstype=9p");
-		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");
-			if (kvm_setup_guest_init(kvm->cfg.custom_rootfs_name))
-				die("Failed to setup init for guest.");
-		}
-	} else if (!kvm->cfg.kernel_cmdline || !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));
+	if (kvm->cfg.custom_rootfs) {
+		kvm_run_set_sandbox(kvm);
+		if (kvm_setup_guest_init(kvm->cfg.custom_rootfs_name))
+			die("Failed to setup init for guest.");
 	}
 
-	kvm->cfg.real_cmdline = real_cmdline;
+	kvm_run_set_real_cmdline(kvm);
 
 	if (kvm->cfg.kernel_filename) {
 		printf("  # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
-- 
2.31.1

[PATCH kvmtool 07/10] Add --nodefaults command line argument

[Alexandru Elisei]

kvmtool attempts to make it as easier as possible on the user to run a VM
by doing a few different things: it tries to create a rootfs filesystem in
a directory if not disk or initrd is set by the user, and it adds various
parameters to the kernel command line based on the VM configuration
options.

While this is generally very useful, today there isn't any way for the user
to prohibit this behaviour, even though there are situations where this
might not be desirable, like, for example: loading something which is not a
kernel (kvm-unit-tests comes to mind, which expects test parameters on the
kernel command line); the kernel has a built-in initramfs and there is no
need to generate the root filesystem, or it not possible; and what is
probably the most important use case, when the user is actively trying to
break things for testing purposes.

Add a --nodefaults command line argument which disables everything that
cannot be disabled via another command line switch. The purpose of this
knob is not to disable the default options for arguments that can be set
via the kvmtool command line, but rather to inhibit behaviour that cannot
be disabled otherwise.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/fdt.c                |  3 ++-
 builtin-run.c            | 13 +++++++++++--
 include/kvm/kvm-config.h |  1 +
 mips/kvm.c               |  3 ++-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 7032985e99a3..635de7f27fa5 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -136,9 +136,10 @@ static int setup_fdt(struct kvm *kvm)
 		if (kvm->cfg.kernel_cmdline)
 			_FDT(fdt_property_string(fdt, "bootargs",
 						 kvm->cfg.kernel_cmdline));
-	} else
+	} else if (kvm->cfg.real_cmdline) {
 		_FDT(fdt_property_string(fdt, "bootargs",
 					 kvm->cfg.real_cmdline));
+	}
 
 	_FDT(fdt_property_u64(fdt, "kaslr-seed", kvm->cfg.arch.kaslr_seed));
 	_FDT(fdt_property_string(fdt, "stdout-path", "serial0"));
diff --git a/builtin-run.c b/builtin-run.c
index 478b5a95b726..9a1a0c1fa6fb 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -108,6 +108,9 @@ void kvm_run_set_wrapper_sandbox(void)
 	OPT_BOOLEAN('\0', "sdl", &(cfg)->sdl, "Enable SDL framebuffer"),\
 	OPT_BOOLEAN('\0', "rng", &(cfg)->virtio_rng, "Enable virtio"	\
 			" Random Number Generator"),			\
+	OPT_BOOLEAN('\0', "nodefaults", &(cfg)->nodefaults, "Disable"   \
+			" implicit configuration that cannot be"	\
+			" disabled otherwise"),				\
 	OPT_CALLBACK('\0', "9p", NULL, "dir_to_share,tag_name",		\
 		     "Enable virtio 9p to share files between host and"	\
 		     " guest", virtio_9p_rootdir_parser, kvm),		\
@@ -642,7 +645,10 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 		}
 	}
 
-	if (!kvm->cfg.using_rootfs && !kvm->cfg.disk_image[0].filename && !kvm->cfg.initrd_filename) {
+	if (!kvm->cfg.nodefaults &&
+	    !kvm->cfg.using_rootfs &&
+	    !kvm->cfg.disk_image[0].filename &&
+	    !kvm->cfg.initrd_filename) {
 		char tmp[PATH_MAX];
 
 		kvm_setup_create_new(kvm->cfg.custom_rootfs_name);
@@ -662,7 +668,10 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 			die("Failed to setup init for guest.");
 	}
 
-	kvm_run_set_real_cmdline(kvm);
+	if (kvm->cfg.nodefaults)
+		kvm->cfg.real_cmdline = kvm->cfg.kernel_cmdline;
+	else
+		kvm_run_set_real_cmdline(kvm);
 
 	if (kvm->cfg.kernel_filename) {
 		printf("  # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
index 35d45c0f7ab1..6a5720c4c7d4 100644
--- a/include/kvm/kvm-config.h
+++ b/include/kvm/kvm-config.h
@@ -27,6 +27,7 @@ struct kvm_config {
 	u8 num_vfio_devices;
 	u64 vsock_cid;
 	bool virtio_rng;
+	bool nodefaults;
 	int active_console;
 	int debug_iodelay;
 	int nrcpus;
diff --git a/mips/kvm.c b/mips/kvm.c
index e110e5d5de8a..3470dbb2e433 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -131,7 +131,8 @@ static void kvm__mips_install_cmdline(struct kvm *kvm)
 			(unsigned long long)kvm->ram_size - KVM_MMIO_START,
 			(unsigned long long)(KVM_MMIO_START + KVM_MMIO_SIZE));
 
-	strcat(p + cmdline_offset, kvm->cfg.real_cmdline); /* maximum size is 2K */
+	if (kvm->cfg.real_cmdline)
+		strcat(p + cmdline_offset, kvm->cfg.real_cmdline); /* maximum size is 2K */
 
 	while (p[cmdline_offset]) {
 		if (!isspace(p[cmdline_offset])) {
-- 
2.31.1

[PATCH kvmtool 08/10] Add --nocompat option to disable compat warnings

[Alexandru Elisei]

Commit e66942073035 ("kvm tools: Guest kernel compatability") added the
functionality that enables devices to print a warning message if the device
hasn't been initialized by the time the VM is destroyed. The purpose of
these messages is to let the user know if the kernel hasn't been built with
the correct Kconfig options to take advantage of the said devices (all
using virtio).

Since then, kvmtool has evolved and now supports loading different payloads
(like firmware images), and having those warnings even when it is entirely
intentional for the payload not to touch the devices can be confusing for
the user and makes the output unnecessarily verbose in those cases.

Add the --nocompat option to disable the warnings; the warnings are still
enabled by default.

Reported-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c            | 5 ++++-
 guest_compat.c           | 1 +
 include/kvm/kvm-config.h | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin-run.c b/builtin-run.c
index 9a1a0c1fa6fb..a736b63ce7e5 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -111,6 +111,8 @@ void kvm_run_set_wrapper_sandbox(void)
 	OPT_BOOLEAN('\0', "nodefaults", &(cfg)->nodefaults, "Disable"   \
 			" implicit configuration that cannot be"	\
 			" disabled otherwise"),				\
+	OPT_BOOLEAN('\0', "nocompat", &(cfg)->nocompat, "Disable"	\
+			" compat warnings"),				\
 	OPT_CALLBACK('\0', "9p", NULL, "dir_to_share,tag_name",		\
 		     "Enable virtio 9p to share files between host and"	\
 		     " guest", virtio_9p_rootdir_parser, kvm),		\
@@ -709,7 +711,8 @@ static int kvm_cmd_run_work(struct kvm *kvm)
 
 static void kvm_cmd_run_exit(struct kvm *kvm, int guest_ret)
 {
-	compat__print_all_messages();
+	if (!kvm->cfg.nocompat)
+		compat__print_all_messages();
 
 	init_list__exit(kvm);
 
diff --git a/guest_compat.c b/guest_compat.c
index fd4704b20b16..a413c12ccd2e 100644
--- a/guest_compat.c
+++ b/guest_compat.c
@@ -88,6 +88,7 @@ int compat__print_all_messages(void)
 
 		printf("\n  # KVM compatibility warning.\n\t%s\n\t%s\n",
 			msg->title, msg->desc);
+		printf("\tTo stop seeing this warning, use the --nocompat option.\n");
 
 		list_del(&msg->list);
 		compat__free(msg);
diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
index 6a5720c4c7d4..329aa46e6eda 100644
--- a/include/kvm/kvm-config.h
+++ b/include/kvm/kvm-config.h
@@ -28,6 +28,7 @@ struct kvm_config {
 	u64 vsock_cid;
 	bool virtio_rng;
 	bool nodefaults;
+	bool nocompat;
 	int active_console;
 	int debug_iodelay;
 	int nrcpus;
-- 
2.31.1

[PATCH kvmtool 09/10] arm64: Use the default offset when the kernel image magic is not found

[Alexandru Elisei]

Commit fd0a05bd27dd ("arm64: Obtain text offset from kernel image") added
support for getting the kernel offset from the kernel header. The code
checks for the kernel header magic number, and if not found, prints a
warning and continues searching for the kernel offset in the image.

The -k/--kernel option can be used to load things which are not a Linux
kernel, but behave like one, like a kvm-unit-tests test. The tests don't
have a valid kernel header, and because kvmtool insists on searching for
the offset, creating a virtual machine can fail with this message:

$ ./vm run -c2 -m256 -k ../kvm-unit-tests/arm/cache.flat
  # lkvm run -k ../kvm-unit-tests/arm/cache.flat -m 256 -c 2 --name guest-7529
  Warning: Kernel image magic not matching
  Warning: unable to translate host address 0x910100a502a00085 to guest
  Fatal: kernel image too big to contain in guest memory.

The host address is a random number read from the test binary from the
location where text_offset is found in the kernel header. Before the
commit, the test was executing just fine:

$ ./vm run -c2 -m256 -k ../kvm-unit-tests/arm/cache.flat
  # lkvm run -k ../kvm-unit-tests/arm/cache.flat -m 256 -c 2 --name guest-8105
INFO: IDC-DIC: dcache clean to PoU required
INFO: IDC-DIC: icache invalidation to PoU required
PASS: IDC-DIC: code generation
SUMMARY: 1 tests

Change kvm__arch_get_kern_offset() so it returns the default text_offset
value if the kernel image magic number is not found, making it possible
again to use something other than a Linux kernel with --kernel.

Reported-by: Vivek Kumar Gautam <vivek.gautam@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/aarch64/kvm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
index 4e66a22ec06d..b38365fb7156 100644
--- a/arm/aarch64/kvm.c
+++ b/arm/aarch64/kvm.c
@@ -35,8 +35,10 @@ unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, int fd)
 
 	lseek(fd, cur_offset, SEEK_SET);
 
-	if (memcmp(&header.magic, ARM64_IMAGE_MAGIC, sizeof(header.magic)))
-		pr_warning("Kernel image magic not matching");
+	if (memcmp(&header.magic, ARM64_IMAGE_MAGIC, sizeof(header.magic))) {
+		warn_str = "Kernel image magic not matching";
+		goto fail;
+	}
 
 	if (le64_to_cpu(header.image_size))
 		return le64_to_cpu(header.text_offset);
-- 
2.31.1

[PATCH kvmtool 10/10] arm64: Be more permissive when parsing the kernel header

[Alexandru Elisei]

kvmtool complains loudly when it parses the kernel header and doesn't find
what it expects, but unless it outright fails to read the kernel image, it
will copy the image in the guest memory at the default offset of 0x80000.

There's no technical reason to stop the user from loading payloads other
than a Linux kernel with the --kernel option. These payloads can behave
just like a kernel and can use an initrd (which is not possible with
--firmware), but don't have the kernel header (like kvm-unit-tests), and
the warnings kvmtool emites can be confusing for this type of payloads.

Change the warnings to debug statements, which can be enabled via the
--debug kvmtool command line option, to make them disappear for these cases
where they aren't really relevant.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/aarch64/kvm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
index b38365fb7156..56a0aedc263d 100644
--- a/arm/aarch64/kvm.c
+++ b/arm/aarch64/kvm.c
@@ -16,7 +16,7 @@ unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, int fd)
 	struct arm64_image_header header;
 	off_t cur_offset;
 	ssize_t size;
-	const char *warn_str;
+	const char *debug_str;
 
 	/* the 32bit kernel offset is a well known value */
 	if (kvm->cfg.arch.aarch32_guest)
@@ -25,8 +25,8 @@ unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, int fd)
 	cur_offset = lseek(fd, 0, SEEK_CUR);
 	if (cur_offset == (off_t)-1 ||
 	    lseek(fd, 0, SEEK_SET) == (off_t)-1) {
-		warn_str = "Failed to seek in kernel image file";
-		goto fail;
+		debug_str = "Failed to seek in kernel image file";
+		goto default_offset;
 	}
 
 	size = xread(fd, &header, sizeof(header));
@@ -36,16 +36,16 @@ unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, int fd)
 	lseek(fd, cur_offset, SEEK_SET);
 
 	if (memcmp(&header.magic, ARM64_IMAGE_MAGIC, sizeof(header.magic))) {
-		warn_str = "Kernel image magic not matching";
-		goto fail;
+		debug_str = "Kernel image magic not matching";
+		goto default_offset;
 	}
 
 	if (le64_to_cpu(header.image_size))
 		return le64_to_cpu(header.text_offset);
 
-	warn_str = "Image size is 0";
-fail:
-	pr_warning("%s, assuming TEXT_OFFSET to be 0x80000", warn_str);
+	debug_str = "Image size is 0";
+default_offset:
+	pr_debug("%s, assuming TEXT_OFFSET to be 0x80000", debug_str);
 	return 0x80000;
 }
 
-- 
2.31.1

Re: [PATCH kvmtool 08/10] Add --nocompat option to disable compat warnings

[Will Deacon]

On Thu, Sep 23, 2021 at 03:45:03PM +0100, Alexandru Elisei wrote:
> Commit e66942073035 ("kvm tools: Guest kernel compatability") added the
> functionality that enables devices to print a warning message if the device
> hasn't been initialized by the time the VM is destroyed. The purpose of
> these messages is to let the user know if the kernel hasn't been built with
> the correct Kconfig options to take advantage of the said devices (all
> using virtio).
> 
> Since then, kvmtool has evolved and now supports loading different payloads
> (like firmware images), and having those warnings even when it is entirely
> intentional for the payload not to touch the devices can be confusing for
> the user and makes the output unnecessarily verbose in those cases.
> 
> Add the --nocompat option to disable the warnings; the warnings are still
> enabled by default.
> 
> Reported-by: Christoffer Dall <christoffer.dall@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  builtin-run.c            | 5 ++++-
>  guest_compat.c           | 1 +
>  include/kvm/kvm-config.h | 1 +
>  3 files changed, 6 insertions(+), 1 deletion(-)

Sorry, bikeshed moment here, but why don't we just have a '--quiet' option
that shuts everything up unless it's fatal?

Will

Re: [PATCH kvmtool 00/10] Run kvm-unit-tests with --kernel

[Will Deacon]

On Thu, 23 Sep 2021 15:44:55 +0100, Alexandru Elisei wrote:
> What prompted this series (which I really hoped will turn out smaller than
> it did) is my attempt to add support for kvmtool to kvm-unit-tests
> automated test runner [1]. When working through the review comments for
> that series, I realized that kvmtool must be able to load an initrd when
> running a test to get all the features that tests rely on.
> 
> kvm-unit-tests uses the initrd, which is expected to be a text file in the
> format key=value, to pass parameters to a test. The initrd is by default
> generated by the runner script, but the location of a custom initrd file
> can also be set using the environment variable KVM_UNIT_TEST_ENV (many
> thanks to Andrew Jones for explaining that). Contained in the automatically
> generated initrd is information about the presence of certain commits in
> the host kernel.  These commits are important because they fix serious bugs
> in KVM, and running tests which are designed to exercise the fix on systems
> where it isn't present can cause the host kernel to crash. kvm-unit-tests
> calls these bug fixing commits erratas, and their presence is signalled by
> an entry ERRATA_<commit_id>=y in the initrd.
> 
> [...]

Applied patches 1-7, 9 and 10 to kvmtool (master), thanks!

[01/10] builtin-run: Treat specifying both --kernel and --firmware as an error
        https://git.kernel.org/will/kvmtool/c/6810e75ce9e0
[02/10] builtin-run: Warn when ignoring initrd because --firmware was specified
        https://git.kernel.org/will/kvmtool/c/6cbec43ef88d
[03/10] builtin-run: Do not attempt to find vmlinux if --firmware
        https://git.kernel.org/will/kvmtool/c/638630c9f7a3
[04/10] builtin-run: Abstract argument validation into a separate function
        https://git.kernel.org/will/kvmtool/c/cce9616484bd
[05/10] Use kvm->nr_disks instead of kvm->cfg.image_count
        https://git.kernel.org/will/kvmtool/c/39ab3a0b380c
[06/10] builtin-run: Move kernel command line generation to a separate function
        https://git.kernel.org/will/kvmtool/c/a5253f7cc810
[07/10] Add --nodefaults command line argument
        https://git.kernel.org/will/kvmtool/c/5613ae26b998
[09/10] arm64: Use the default offset when the kernel image magic is not found
        https://git.kernel.org/will/kvmtool/c/5303f0964ffd
[10/10] arm64: Be more permissive when parsing the kernel header
        https://git.kernel.org/will/kvmtool/c/dc6646192057

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH kvmtool 08/10] Add --nocompat option to disable compat warnings

[Alexandru Elisei]

Hi Will,

On Tue, Oct 12, 2021 at 09:34:53AM +0100, Will Deacon wrote:
> On Thu, Sep 23, 2021 at 03:45:03PM +0100, Alexandru Elisei wrote:
> > Commit e66942073035 ("kvm tools: Guest kernel compatability") added the
> > functionality that enables devices to print a warning message if the device
> > hasn't been initialized by the time the VM is destroyed. The purpose of
> > these messages is to let the user know if the kernel hasn't been built with
> > the correct Kconfig options to take advantage of the said devices (all
> > using virtio).
> > 
> > Since then, kvmtool has evolved and now supports loading different payloads
> > (like firmware images), and having those warnings even when it is entirely
> > intentional for the payload not to touch the devices can be confusing for
> > the user and makes the output unnecessarily verbose in those cases.
> > 
> > Add the --nocompat option to disable the warnings; the warnings are still
> > enabled by default.
> > 
> > Reported-by: Christoffer Dall <christoffer.dall@arm.com>
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  builtin-run.c            | 5 ++++-
> >  guest_compat.c           | 1 +
> >  include/kvm/kvm-config.h | 1 +
> >  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> Sorry, bikeshed moment here, but why don't we just have a '--quiet' option
> that shuts everything up unless it's fatal?

I can't figure out what is the criteria for deciding what is silenced by --quiet
and what is still shown. I chose --nocompat because it is clear what is supposed
to disable.

One possibility would be to hide pr_info() and the compat warnings. But that
still doesn't feel right to me - why hide *only* the compat warnings and leave
the other warnings unchanged? I could see having a --nocompat that hides the
compat warningis *and* a quiet option that hides pr_info() output (grepping for
pr_info reveals a number of places where it is used).

What do you think? Do you have something else in mind?

Thanks,
Alex

> 
> Will