[PATCH kvmtool 0/3] Misc fixes

[PATCH kvmtool 0/3] Misc fixes

[Alexandru Elisei]

These are a handful of small fixes for random stuff which I found while
using kvmtool.

Patch #1 is actually needed to use kvmtool as a test runner for
kvm-unit-tests (more detailed explanation in the commit message).

Patch #2 is more like a quality of life improvement for users.

Patch #3 is for something that I found when a Linux guest complained that
"msi-parent" was referencing an invalid phandle (I don't remember the
kernel version, defconfig and VM configuration that I used, couldn't
reproduce it with a v5.16 guest); it was also reported to me by Pierre, so
I figured it deserves a fix.

Alexandru Elisei (3):
  Remove initrd magic check
  arm: Use pr_debug() to print memory layout when loading a firmware
    image
  arm: pci: Generate "msi-parent" property only with a MSI controller

 arm/fdt.c                    |  2 +-
 arm/include/arm-common/pci.h |  3 ++-
 arm/kvm.c                    |  8 +++++---
 arm/pci.c                    |  8 ++++++--
 kvm.c                        | 22 ----------------------
 5 files changed, 14 insertions(+), 29 deletions(-)

-- 
2.31.1

[PATCH kvmtool 0/3] Misc fixes

[Alexandru Elisei]

These are a handful of small fixes for random stuff which I found while
using kvmtool.

Patch #1 is actually needed to use kvmtool as a test runner for
kvm-unit-tests (more detailed explanation in the commit message).

Patch #2 is more like a quality of life improvement for users.

Patch #3 is for something that I found when a Linux guest complained that
"msi-parent" was referencing an invalid phandle (I don't remember the
kernel version, defconfig and VM configuration that I used, couldn't
reproduce it with a v5.16 guest); it was also reported to me by Pierre, so
I figured it deserves a fix.

Alexandru Elisei (3):
  Remove initrd magic check
  arm: Use pr_debug() to print memory layout when loading a firmware
    image
  arm: pci: Generate "msi-parent" property only with a MSI controller

 arm/fdt.c                    |  2 +-
 arm/include/arm-common/pci.h |  3 ++-
 arm/kvm.c                    |  8 +++++---
 arm/pci.c                    |  8 ++++++--
 kvm.c                        | 22 ----------------------
 5 files changed, 14 insertions(+), 29 deletions(-)

-- 
2.31.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH kvmtool 1/3] Remove initrd magic check

[Alexandru Elisei]

Linux, besides CPIO, supports 7 different compressed formats for the initrd
(gzip, bzip2, LZMA, XZ, LZO, LZ4, ZSTD), but kvmtool only recognizes one of
them.

Remove the initrd magic check because:

1. It doesn't bring much to the end user, as the Linux kernel still
   complains if the initrd is in an unknown format.

2. --kernel can be used to load something that is not a Linux kernel (like
   a kvm-unit-tests test), in which case a format which is not supported by
   a Linux kernel can still be perfectly valid. For example, kvm-unit-tests
   load the test environment as an initrd in plain ASCII format.

3. It cuts down on the maintenance effort when new formats are added to
   the Linux kernel. Not a big deal, since that doesn't happen very often,
   but it's still an effort with very little gain (see point #1 above).

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 kvm.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/kvm.c b/kvm.c
index 5bc66c8be82a..952ef1fbb41c 100644
--- a/kvm.c
+++ b/kvm.c
@@ -512,25 +512,6 @@ err:
 }
 core_init(kvm__init);
 
-/* RFC 1952 */
-#define GZIP_ID1		0x1f
-#define GZIP_ID2		0x8b
-#define CPIO_MAGIC		"0707"
-/* initrd may be gzipped, or a plain cpio */
-static bool initrd_check(int fd)
-{
-	unsigned char id[4];
-
-	if (read_in_full(fd, id, ARRAY_SIZE(id)) < 0)
-		return false;
-
-	if (lseek(fd, 0, SEEK_SET) < 0)
-		die_perror("lseek");
-
-	return (id[0] == GZIP_ID1 && id[1] == GZIP_ID2) ||
-		!memcmp(id, CPIO_MAGIC, 4);
-}
-
 bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
 		const char *initrd_filename, const char *kernel_cmdline)
 {
@@ -545,9 +526,6 @@ bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
 		fd_initrd = open(initrd_filename, O_RDONLY);
 		if (fd_initrd < 0)
 			die("Unable to open initrd %s", initrd_filename);
-
-		if (!initrd_check(fd_initrd))
-			die("%s is not an initrd", initrd_filename);
 	}
 
 	ret = kvm__arch_load_kernel_image(kvm, fd_kernel, fd_initrd,
-- 
2.31.1

[PATCH kvmtool 1/3] Remove initrd magic check

[Alexandru Elisei]

Linux, besides CPIO, supports 7 different compressed formats for the initrd
(gzip, bzip2, LZMA, XZ, LZO, LZ4, ZSTD), but kvmtool only recognizes one of
them.

Remove the initrd magic check because:

1. It doesn't bring much to the end user, as the Linux kernel still
   complains if the initrd is in an unknown format.

2. --kernel can be used to load something that is not a Linux kernel (like
   a kvm-unit-tests test), in which case a format which is not supported by
   a Linux kernel can still be perfectly valid. For example, kvm-unit-tests
   load the test environment as an initrd in plain ASCII format.

3. It cuts down on the maintenance effort when new formats are added to
   the Linux kernel. Not a big deal, since that doesn't happen very often,
   but it's still an effort with very little gain (see point #1 above).

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 kvm.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/kvm.c b/kvm.c
index 5bc66c8be82a..952ef1fbb41c 100644
--- a/kvm.c
+++ b/kvm.c
@@ -512,25 +512,6 @@ err:
 }
 core_init(kvm__init);
 
-/* RFC 1952 */
-#define GZIP_ID1		0x1f
-#define GZIP_ID2		0x8b
-#define CPIO_MAGIC		"0707"
-/* initrd may be gzipped, or a plain cpio */
-static bool initrd_check(int fd)
-{
-	unsigned char id[4];
-
-	if (read_in_full(fd, id, ARRAY_SIZE(id)) < 0)
-		return false;
-
-	if (lseek(fd, 0, SEEK_SET) < 0)
-		die_perror("lseek");
-
-	return (id[0] == GZIP_ID1 && id[1] == GZIP_ID2) ||
-		!memcmp(id, CPIO_MAGIC, 4);
-}
-
 bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
 		const char *initrd_filename, const char *kernel_cmdline)
 {
@@ -545,9 +526,6 @@ bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
 		fd_initrd = open(initrd_filename, O_RDONLY);
 		if (fd_initrd < 0)
 			die("Unable to open initrd %s", initrd_filename);
-
-		if (!initrd_check(fd_initrd))
-			die("%s is not an initrd", initrd_filename);
 	}
 
 	ret = kvm__arch_load_kernel_image(kvm, fd_kernel, fd_initrd,
-- 
2.31.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH kvmtool 2/3] arm: Use pr_debug() to print memory layout when loading a firmware image

[Alexandru Elisei]

When loading a kernel image, kvmtool is nice enough to print a message
informing the user where the file was loaded in guest memory, which is very
useful for debugging. Do the same for the firmware image.

Commit e1c7c62afc7b ("arm: turn pr_info() into pr_debug() messages")
changed various pr_info() into pr_debug() messages to stop kvmtool from
cluttering stdout. Do the same when printing where the FDT has been copied
when loading a firmware image.

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

diff --git a/arm/kvm.c b/arm/kvm.c
index 5aea18fedc3e..80d233f13d0b 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -218,6 +218,8 @@ bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
 
 	/* Kernel isn't loaded by kvm, point start address to firmware */
 	kvm->arch.kern_guest_start = fw_addr;
+	pr_debug("Loaded firmware to 0x%llx (%zd bytes)",
+		 kvm->arch.kern_guest_start, fw_sz);
 
 	/* Load dtb just after the firmware image*/
 	host_pos += fw_sz;
@@ -226,9 +228,9 @@ bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
 
 	kvm->arch.dtb_guest_start = ALIGN(host_to_guest_flat(kvm, host_pos),
 					  FDT_ALIGN);
-	pr_info("Placing fdt at 0x%llx - 0x%llx",
-		kvm->arch.dtb_guest_start,
-		kvm->arch.dtb_guest_start + FDT_MAX_SIZE);
+	pr_debug("Placing fdt at 0x%llx - 0x%llx",
+		 kvm->arch.dtb_guest_start,
+		 kvm->arch.dtb_guest_start + FDT_MAX_SIZE);
 
 	return true;
 }
-- 
2.31.1

[PATCH kvmtool 2/3] arm: Use pr_debug() to print memory layout when loading a firmware image

[Alexandru Elisei]

When loading a kernel image, kvmtool is nice enough to print a message
informing the user where the file was loaded in guest memory, which is very
useful for debugging. Do the same for the firmware image.

Commit e1c7c62afc7b ("arm: turn pr_info() into pr_debug() messages")
changed various pr_info() into pr_debug() messages to stop kvmtool from
cluttering stdout. Do the same when printing where the FDT has been copied
when loading a firmware image.

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

diff --git a/arm/kvm.c b/arm/kvm.c
index 5aea18fedc3e..80d233f13d0b 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -218,6 +218,8 @@ bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
 
 	/* Kernel isn't loaded by kvm, point start address to firmware */
 	kvm->arch.kern_guest_start = fw_addr;
+	pr_debug("Loaded firmware to 0x%llx (%zd bytes)",
+		 kvm->arch.kern_guest_start, fw_sz);
 
 	/* Load dtb just after the firmware image*/
 	host_pos += fw_sz;
@@ -226,9 +228,9 @@ bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
 
 	kvm->arch.dtb_guest_start = ALIGN(host_to_guest_flat(kvm, host_pos),
 					  FDT_ALIGN);
-	pr_info("Placing fdt at 0x%llx - 0x%llx",
-		kvm->arch.dtb_guest_start,
-		kvm->arch.dtb_guest_start + FDT_MAX_SIZE);
+	pr_debug("Placing fdt at 0x%llx - 0x%llx",
+		 kvm->arch.dtb_guest_start,
+		 kvm->arch.dtb_guest_start + FDT_MAX_SIZE);
 
 	return true;
 }
-- 
2.31.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH kvmtool 3/3] arm: pci: Generate "msi-parent" property only with a MSI controller

[Alexandru Elisei]

The "msi-parent" PCI root complex property describes the MSI parent of the
root complex. When the VM is created with a GICv2 or GICv3 irqchip
(--irqchip=gicv3 or --irqchip=gicv2), there is no MSI controller present on
the system and the corresponding phandle is not generated, leaving the
"msi-parent" property to point to a non-existing phandle. Skip creating the
"msi-parent" property when no MSI controller exists.

Reported-by: Pierre Gondois <pierre.gondois@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/fdt.c                    | 2 +-
 arm/include/arm-common/pci.h | 3 ++-
 arm/pci.c                    | 8 ++++++--
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 635de7f27fa5..286ccadf1311 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -190,7 +190,7 @@ static int setup_fdt(struct kvm *kvm)
 	}
 
 	/* PCI host controller */
-	pci__generate_fdt_nodes(fdt);
+	pci__generate_fdt_nodes(fdt, kvm);
 
 	/* PSCI firmware */
 	_FDT(fdt_begin_node(fdt, "psci"));
diff --git a/arm/include/arm-common/pci.h b/arm/include/arm-common/pci.h
index 9008a0ed072e..afa7f7f42bcb 100644
--- a/arm/include/arm-common/pci.h
+++ b/arm/include/arm-common/pci.h
@@ -1,6 +1,7 @@
 #ifndef ARM_COMMON__PCI_H
 #define ARM_COMMON__PCI_H
 
-void pci__generate_fdt_nodes(void *fdt);
+struct kvm;
+void pci__generate_fdt_nodes(void *fdt, struct kvm *kvm);
 
 #endif /* ARM_COMMON__PCI_H */
diff --git a/arm/pci.c b/arm/pci.c
index e44e45343c6a..5bd82d438807 100644
--- a/arm/pci.c
+++ b/arm/pci.c
@@ -6,6 +6,7 @@
 #include "kvm/util.h"
 
 #include "arm-common/pci.h"
+#include "arm-common/gic.h"
 
 /*
  * An entry in the interrupt-map table looks like:
@@ -24,8 +25,9 @@ struct of_interrupt_map_entry {
 	struct of_gic_irq		gic_irq;
 } __attribute__((packed));
 
-void pci__generate_fdt_nodes(void *fdt)
+void pci__generate_fdt_nodes(void *fdt, struct kvm *kvm)
 {
+	enum irqchip_type irqchip = kvm->cfg.arch.irqchip;
 	struct device_header *dev_hdr;
 	struct of_interrupt_map_entry irq_map[OF_PCI_IRQ_MAP_MAX];
 	unsigned nentries = 0;
@@ -68,7 +70,9 @@ void pci__generate_fdt_nodes(void *fdt)
 	_FDT(fdt_property(fdt, "bus-range", bus_range, sizeof(bus_range)));
 	_FDT(fdt_property(fdt, "reg", &cfg_reg_prop, sizeof(cfg_reg_prop)));
 	_FDT(fdt_property(fdt, "ranges", ranges, sizeof(ranges)));
-	_FDT(fdt_property_cell(fdt, "msi-parent", PHANDLE_MSI));
+
+	if (irqchip == IRQCHIP_GICV2M || irqchip == IRQCHIP_GICV3_ITS)
+		_FDT(fdt_property_cell(fdt, "msi-parent", PHANDLE_MSI));
 
 	/* Generate the interrupt map ... */
 	dev_hdr = device__first_dev(DEVICE_BUS_PCI);
-- 
2.31.1

[PATCH kvmtool 3/3] arm: pci: Generate "msi-parent" property only with a MSI controller

[Alexandru Elisei]

The "msi-parent" PCI root complex property describes the MSI parent of the
root complex. When the VM is created with a GICv2 or GICv3 irqchip
(--irqchip=gicv3 or --irqchip=gicv2), there is no MSI controller present on
the system and the corresponding phandle is not generated, leaving the
"msi-parent" property to point to a non-existing phandle. Skip creating the
"msi-parent" property when no MSI controller exists.

Reported-by: Pierre Gondois <pierre.gondois@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/fdt.c                    | 2 +-
 arm/include/arm-common/pci.h | 3 ++-
 arm/pci.c                    | 8 ++++++--
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 635de7f27fa5..286ccadf1311 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -190,7 +190,7 @@ static int setup_fdt(struct kvm *kvm)
 	}
 
 	/* PCI host controller */
-	pci__generate_fdt_nodes(fdt);
+	pci__generate_fdt_nodes(fdt, kvm);
 
 	/* PSCI firmware */
 	_FDT(fdt_begin_node(fdt, "psci"));
diff --git a/arm/include/arm-common/pci.h b/arm/include/arm-common/pci.h
index 9008a0ed072e..afa7f7f42bcb 100644
--- a/arm/include/arm-common/pci.h
+++ b/arm/include/arm-common/pci.h
@@ -1,6 +1,7 @@
 #ifndef ARM_COMMON__PCI_H
 #define ARM_COMMON__PCI_H
 
-void pci__generate_fdt_nodes(void *fdt);
+struct kvm;
+void pci__generate_fdt_nodes(void *fdt, struct kvm *kvm);
 
 #endif /* ARM_COMMON__PCI_H */
diff --git a/arm/pci.c b/arm/pci.c
index e44e45343c6a..5bd82d438807 100644
--- a/arm/pci.c
+++ b/arm/pci.c
@@ -6,6 +6,7 @@
 #include "kvm/util.h"
 
 #include "arm-common/pci.h"
+#include "arm-common/gic.h"
 
 /*
  * An entry in the interrupt-map table looks like:
@@ -24,8 +25,9 @@ struct of_interrupt_map_entry {
 	struct of_gic_irq		gic_irq;
 } __attribute__((packed));
 
-void pci__generate_fdt_nodes(void *fdt)
+void pci__generate_fdt_nodes(void *fdt, struct kvm *kvm)
 {
+	enum irqchip_type irqchip = kvm->cfg.arch.irqchip;
 	struct device_header *dev_hdr;
 	struct of_interrupt_map_entry irq_map[OF_PCI_IRQ_MAP_MAX];
 	unsigned nentries = 0;
@@ -68,7 +70,9 @@ void pci__generate_fdt_nodes(void *fdt)
 	_FDT(fdt_property(fdt, "bus-range", bus_range, sizeof(bus_range)));
 	_FDT(fdt_property(fdt, "reg", &cfg_reg_prop, sizeof(cfg_reg_prop)));
 	_FDT(fdt_property(fdt, "ranges", ranges, sizeof(ranges)));
-	_FDT(fdt_property_cell(fdt, "msi-parent", PHANDLE_MSI));
+
+	if (irqchip == IRQCHIP_GICV2M || irqchip == IRQCHIP_GICV3_ITS)
+		_FDT(fdt_property_cell(fdt, "msi-parent", PHANDLE_MSI));
 
 	/* Generate the interrupt map ... */
 	dev_hdr = device__first_dev(DEVICE_BUS_PCI);
-- 
2.31.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH kvmtool 0/3] Misc fixes

[Will Deacon]

On Mon, 14 Feb 2022 16:58:27 +0000, Alexandru Elisei wrote:
> These are a handful of small fixes for random stuff which I found while
> using kvmtool.
> 
> Patch #1 is actually needed to use kvmtool as a test runner for
> kvm-unit-tests (more detailed explanation in the commit message).
> 
> Patch #2 is more like a quality of life improvement for users.
> 
> [...]

Applied to kvmtool (master), thanks!

[1/3] Remove initrd magic check
      https://git.kernel.org/will/kvmtool/c/9b681b0827d7
[2/3] arm: Use pr_debug() to print memory layout when loading a firmware image
      https://git.kernel.org/will/kvmtool/c/c334a68e202e
[3/3] arm: pci: Generate "msi-parent" property only with a MSI controller
      https://git.kernel.org/will/kvmtool/c/95f47968a1d3

Cheers,
-- 
Will

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

Re: [PATCH kvmtool 0/3] Misc fixes

[Will Deacon]

On Mon, 14 Feb 2022 16:58:27 +0000, Alexandru Elisei wrote:
> These are a handful of small fixes for random stuff which I found while
> using kvmtool.
> 
> Patch #1 is actually needed to use kvmtool as a test runner for
> kvm-unit-tests (more detailed explanation in the commit message).
> 
> Patch #2 is more like a quality of life improvement for users.
> 
> [...]

Applied to kvmtool (master), thanks!

[1/3] Remove initrd magic check
      https://git.kernel.org/will/kvmtool/c/9b681b0827d7
[2/3] arm: Use pr_debug() to print memory layout when loading a firmware image
      https://git.kernel.org/will/kvmtool/c/c334a68e202e
[3/3] arm: pci: Generate "msi-parent" property only with a MSI controller
      https://git.kernel.org/will/kvmtool/c/95f47968a1d3

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm