[PATCH kvmtool v2 0/6] arm: Add support for firmware booting

[PATCH kvmtool v2 0/6] arm: Add support for firmware booting

[Julien Thierry]

Hi,

This series is based on the virtio reset series[1] posted earlier.

We would like to be able to load firmwares like UEFI in kvmtool.

The series contains:
A way to load the firmware into RAM and an option to be able to create
non-volatile memory zones and load data into them.

Those non-volatile memory are presented throught the DT with a node:

	<flash>@<addr> {
		compatible = "kvmtool,flash";
		reg = < <addr> <size> >;
		label = <name>;
	}

These are expected to be dealt with by specific kvmtool driver and not
to be picked up by existing drivers (although technically it is just
plain memory, mapped in the guest).


Changes since v1[2]:
* Added Andre's Reviewed-by tags.
* Changed the DT binding for nvmem.
* removed the requirement to provide firmware load address (defaults to
  the start of RAM).

[1] https://marc.info/?l=kvm&m=154712959125764&w=2
[2] https://marc.info/?l=kvm&m=154392208726108&w=2

Cheers,

Julien

-->

Julien Thierry (5):
  arm: Move firmware function
  builtin-run: Do not look for default kernel when firmware is provided
  arm: Support firmware loading
  kvm: Add arch specific reset
  arm: Support non-volatile memory

Sami Mujawar (1):
  rtc: Initialize the Register D for MC146818 RTC

 arm/fdt.c                                |  58 +++++++--
 arm/include/arm-common/kvm-arch.h        |   5 +-
 arm/include/arm-common/kvm-config-arch.h |  21 +++-
 arm/kvm.c                                | 207 +++++++++++++++++++++++++++++++
 builtin-run.c                            |  24 ++--
 hw/rtc.c                                 |   8 ++
 include/kvm/kvm.h                        |   1 +
 kvm.c                                    |   2 +
 mips/kvm.c                               |   4 +
 powerpc/kvm.c                            |   4 +
 x86/kvm.c                                |   4 +
 11 files changed, 317 insertions(+), 21 deletions(-)

--
1.9.1

[PATCH kvmtool v2 1/6] rtc: Initialize the Register D for MC146818 RTC

[Julien Thierry]

From: Sami Mujawar <sami.mujawar@arm.com>

Some software drivers check the VRT bit (BIT7) of Register D before
using the MC146818 RTC. Initialized the VRT bit in rtc__init() to
indicate that the RAM and time contents are valid.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 hw/rtc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/rtc.c b/hw/rtc.c
index 0649b5d..c1fa72f 100644
--- a/hw/rtc.c
+++ b/hw/rtc.c
@@ -25,6 +25,11 @@
 #define RTC_REG_C			0x0C
 #define RTC_REG_D			0x0D
 
+/*
+ * Register D Bits
+ */
+#define RTC_REG_D_VRT			(1 << 7)
+
 struct rtc_device {
 	u8			cmos_idx;
 	u8			cmos_data[128];
@@ -140,6 +145,9 @@ int rtc__init(struct kvm *kvm)
 		return r;
 	}
 
+	/* Set the VRT bit in Register D to indicate valid RAM and time */
+	rtc.cmos_data[RTC_REG_D] = RTC_REG_D_VRT;
+
 	return r;
 }
 dev_init(rtc__init);
-- 
1.9.1

[PATCH kvmtool v2 2/6] arm: Move firmware function

[Julien Thierry]

Firmware loading/setup function are in fdt file while it is not very
related to this.

Move them to the file that does the main init/setup for memory.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/fdt.c | 10 ----------
 arm/kvm.c | 10 ++++++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 980015b..664bb62 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -14,16 +14,6 @@
 #include <linux/sizes.h>
 #include <linux/psci.h>
 
-bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
-{
-	return false;
-}
-
-int kvm__arch_setup_firmware(struct kvm *kvm)
-{
-	return 0;
-}
-
 static void dump_fdt(const char *dtb_file, void *fdt)
 {
 	int count, fd;
diff --git a/arm/kvm.c b/arm/kvm.c
index b824f63..c6843e5 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -168,3 +168,13 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 
 	return true;
 }
+
+bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
+{
+	return false;
+}
+
+int kvm__arch_setup_firmware(struct kvm *kvm)
+{
+	return 0;
+}
-- 
1.9.1

[PATCH kvmtool v2 3/6] builtin-run: Do not look for default kernel when firmware is provided

[Julien Thierry]

When a firmware file is provided, kvmtool is not responsible for loading
a kernel image.

There is no reason for looking for a default kernel image when loading
a firmware.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 builtin-run.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index 443c10b..82e2b2e 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -512,12 +512,13 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 
 	kvm->nr_disks = kvm->cfg.image_count;
 
-	if (!kvm->cfg.kernel_filename)
+	if (!kvm->cfg.kernel_filename && !kvm->cfg.firmware_filename) {
 		kvm->cfg.kernel_filename = find_kernel();
 
-	if (!kvm->cfg.kernel_filename) {
-		kernel_usage_with_options();
-		return ERR_PTR(-EINVAL);
+		if (!kvm->cfg.kernel_filename) {
+			kernel_usage_with_options();
+			return ERR_PTR(-EINVAL);
+		}
 	}
 
 	kvm->cfg.vmlinux_filename = find_vmlinux();
@@ -639,10 +640,17 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 
 	kvm->cfg.real_cmdline = real_cmdline;
 
-	printf("  # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
-		kvm->cfg.kernel_filename,
-		(unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
-		kvm->cfg.nrcpus, kvm->cfg.guest_name);
+	if (kvm->cfg.kernel_filename) {
+		printf("  # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
+		       kvm->cfg.kernel_filename,
+		       (unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
+		       kvm->cfg.nrcpus, kvm->cfg.guest_name);
+	} else if (kvm->cfg.firmware_filename) {
+		printf("  # %s run --firmware %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
+		       kvm->cfg.firmware_filename,
+		       (unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
+		       kvm->cfg.nrcpus, kvm->cfg.guest_name);
+	}
 
 	if (init_list__init(kvm) < 0)
 		die ("Initialisation failed");
-- 
1.9.1

[PATCH kvmtool v2 4/6] arm: Support firmware loading

[Julien Thierry]

Implement firmware image loading for arm and set the boot start address
to the firmware address.

Add an option for the user to specify where to map the firmware.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arm/fdt.c                                | 14 +++++++-
 arm/include/arm-common/kvm-config-arch.h |  5 ++-
 arm/kvm.c                                | 61 +++++++++++++++++++++++++++++++-
 3 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 664bb62..2936986 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -131,7 +131,19 @@ static int setup_fdt(struct kvm *kvm)
 	/* /chosen */
 	_FDT(fdt_begin_node(fdt, "chosen"));
 	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
-	_FDT(fdt_property_string(fdt, "bootargs", kvm->cfg.real_cmdline));
+
+	if (kvm->cfg.firmware_filename) {
+		/*
+		 * When using a firmware, command line is not passed through DT,
+		 * or the firmware can add it itself
+		 */
+		if (kvm->cfg.kernel_cmdline)
+			pr_warning("Ignoring custom bootargs: %s\n",
+				   kvm->cfg.kernel_cmdline);
+	} else
+		_FDT(fdt_property_string(fdt, "bootargs",
+					 kvm->cfg.real_cmdline));
+
 	_FDT(fdt_property_u64(fdt, "kaslr-seed", kvm->cfg.arch.kaslr_seed));
 
 	/* Initrd */
diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
index 6a196f1..5734c46 100644
--- a/arm/include/arm-common/kvm-config-arch.h
+++ b/arm/include/arm-common/kvm-config-arch.h
@@ -11,6 +11,7 @@ struct kvm_config_arch {
 	bool		has_pmuv3;
 	u64		kaslr_seed;
 	enum irqchip_type irqchip;
+	u64		fw_addr;
 };
 
 int irqchip_parser(const struct option *opt, const char *arg, int unset);
@@ -30,6 +31,8 @@ int irqchip_parser(const struct option *opt, const char *arg, int unset);
         OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,				\
 		     "[gicv2|gicv2m|gicv3|gicv3-its]",				\
 		     "Type of interrupt controller to emulate in the guest",	\
-		     irqchip_parser, NULL),
+		     irqchip_parser, NULL),					\
+	OPT_U64('\0', "firmware-address", &(cfg)->fw_addr,			\
+		"Address where firmware should be loaded"),
 
 #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index c6843e5..d3f8f5d 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -169,9 +169,68 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 	return true;
 }
 
+static bool validate_fw_addr(struct kvm *kvm, u64 fw_addr)
+{
+	u64 ram_phys;
+
+	ram_phys = host_to_guest_flat(kvm, kvm->ram_start);
+
+	if (fw_addr < ram_phys || fw_addr >= ram_phys + kvm->ram_size) {
+		pr_err("Provide --firmware-address an address in RAM: "
+		       "0x%016llx - 0x%016llx",
+		       ram_phys, ram_phys + kvm->ram_size);
+
+		return false;
+	}
+
+	return true;
+}
+
 bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
 {
-	return false;
+	u64 fw_addr = kvm->cfg.arch.fw_addr;
+	void *host_pos;
+	void *limit;
+	ssize_t fw_sz;
+	int fd;
+
+	limit = kvm->ram_start + kvm->ram_size;
+
+	/* For default firmware address, lets load it at the begining of RAM */
+	if (fw_addr == 0)
+		fw_addr = ARM_MEMORY_AREA;
+
+	if (!validate_fw_addr(kvm, fw_addr))
+		die("Bad firmware destination: 0x%016llx", fw_addr);
+
+	fd = open(firmware_filename, O_RDONLY);
+	if (fd < 0)
+		return false;
+
+	host_pos = guest_flat_to_host(kvm, fw_addr);
+	if (!host_pos || host_pos < kvm->ram_start)
+		return false;
+
+	fw_sz = read_file(fd, host_pos, limit - host_pos);
+	if (fw_sz < 0)
+		die("failed to load firmware");
+	close(fd);
+
+	/* Kernel isn't loaded by kvm, point start address to firmware */
+	kvm->arch.kern_guest_start = fw_addr;
+
+	/* Load dtb just after the firmware image*/
+	host_pos += fw_sz;
+	if (host_pos + FDT_MAX_SIZE > limit)
+		die("not enough space to load fdt");
+
+	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);
+
+	return true;
 }
 
 int kvm__arch_setup_firmware(struct kvm *kvm)
-- 
1.9.1

Re: [PATCH kvmtool v2 4/6] arm: Support firmware loading

[Andre Przywara]

On Thu, 10 Jan 2019 14:20:44 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

Hi,

> Implement firmware image loading for arm and set the boot start
> address to the firmware address.
> 
> Add an option for the user to specify where to map the firmware.

Many thanks for making this optional!

> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> ---
>  arm/fdt.c                                | 14 +++++++-
>  arm/include/arm-common/kvm-config-arch.h |  5 ++-
>  arm/kvm.c                                | 61
> +++++++++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 3
> deletions(-)
> 
> diff --git a/arm/fdt.c b/arm/fdt.c
> index 664bb62..2936986 100644
> --- a/arm/fdt.c
> +++ b/arm/fdt.c
> @@ -131,7 +131,19 @@ static int setup_fdt(struct kvm *kvm)
>  	/* /chosen */
>  	_FDT(fdt_begin_node(fdt, "chosen"));
>  	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
> -	_FDT(fdt_property_string(fdt, "bootargs",
> kvm->cfg.real_cmdline)); +
> +	if (kvm->cfg.firmware_filename) {
> +		/*
> +		 * When using a firmware, command line is not passed
> through DT,
> +		 * or the firmware can add it itself
> +		 */
> +		if (kvm->cfg.kernel_cmdline)
> +			pr_warning("Ignoring custom bootargs: %s\n",
> +				   kvm->cfg.kernel_cmdline);
> +	} else
> +		_FDT(fdt_property_string(fdt, "bootargs",
> +					 kvm->cfg.real_cmdline));
> +
>  	_FDT(fdt_property_u64(fdt, "kaslr-seed",
> kvm->cfg.arch.kaslr_seed)); 
>  	/* Initrd */
> diff --git a/arm/include/arm-common/kvm-config-arch.h
> b/arm/include/arm-common/kvm-config-arch.h index 6a196f1..5734c46
> 100644 --- a/arm/include/arm-common/kvm-config-arch.h
> +++ b/arm/include/arm-common/kvm-config-arch.h
> @@ -11,6 +11,7 @@ struct kvm_config_arch {
>  	bool		has_pmuv3;
>  	u64		kaslr_seed;
>  	enum irqchip_type irqchip;
> +	u64		fw_addr;
>  };
>  
>  int irqchip_parser(const struct option *opt, const char *arg, int
> unset); @@ -30,6 +31,8 @@ int irqchip_parser(const struct option
> *opt, const char *arg, int unset); OPT_CALLBACK('\0', "irqchip",
> &(cfg)->irqchip,				\
> "[gicv2|gicv2m|gicv3|gicv3-its]",				\
> "Type of interrupt controller to emulate in the guest",	\
> -		     irqchip_parser, NULL),
> +		     irqchip_parser,
> NULL),					\
> +	OPT_U64('\0', "firmware-address",
> &(cfg)->fw_addr,			\
> +		"Address where firmware should be loaded"),
>  
>  #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index c6843e5..d3f8f5d 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -169,9 +169,68 @@ bool kvm__arch_load_kernel_image(struct kvm
> *kvm, int fd_kernel, int fd_initrd, return true;
>  }
>  
> +static bool validate_fw_addr(struct kvm *kvm, u64 fw_addr)
> +{
> +	u64 ram_phys;
> +
> +	ram_phys = host_to_guest_flat(kvm, kvm->ram_start);
> +
> +	if (fw_addr < ram_phys || fw_addr >= ram_phys +
> kvm->ram_size) {

I am OK with this check for now, but was wondering if we really *need*
to have the firmware in RAM? Couldn't we possibly put it into some
ROM/flash/nvmem device memory area and utilise XIP, for instance? I
understand that EDKII does not support this at the moment, but was just
thinking whether this would be an artificial restriction for other
firmware ports.

But I guess we can relax this later on, possibly when we support this
properly.

> +		pr_err("Provide --firmware-address an address in
> RAM: "
> +		       "0x%016llx - 0x%016llx",
> +		       ram_phys, ram_phys + kvm->ram_size);
> +
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  bool kvm__load_firmware(struct kvm *kvm, const char
> *firmware_filename) {
> -	return false;
> +	u64 fw_addr = kvm->cfg.arch.fw_addr;
> +	void *host_pos;
> +	void *limit;
> +	ssize_t fw_sz;
> +	int fd;
> +
> +	limit = kvm->ram_start + kvm->ram_size;
> +
> +	/* For default firmware address, lets load it at the
> begining of RAM */
> +	if (fw_addr == 0)
> +		fw_addr = ARM_MEMORY_AREA;
> +
> +	if (!validate_fw_addr(kvm, fw_addr))
> +		die("Bad firmware destination: 0x%016llx", fw_addr);
> +
> +	fd = open(firmware_filename, O_RDONLY);
> +	if (fd < 0)
> +		return false;
> +
> +	host_pos = guest_flat_to_host(kvm, fw_addr);
> +	if (!host_pos || host_pos < kvm->ram_start)
> +		return false;
> +
> +	fw_sz = read_file(fd, host_pos, limit - host_pos);
> +	if (fw_sz < 0)
> +		die("failed to load firmware");
> +	close(fd);
> +
> +	/* Kernel isn't loaded by kvm, point start address to
> firmware */
> +	kvm->arch.kern_guest_start = fw_addr;
> +
> +	/* Load dtb just after the firmware image*/
> +	host_pos += fw_sz;
> +	if (host_pos + FDT_MAX_SIZE > limit)
> +		die("not enough space to load fdt");
> +
> +	kvm->arch.dtb_guest_start = ALIGN(host_to_guest_flat(kvm,
> host_pos),
> +					  FDT_ALIGN);

Does anyone know why we force a 2MB alignment (FDT_ALIGN) here? Will?
The DT spec and the ARM and arm64 kernel boot protocols just require an
8 byte alignment. So far this didn't really matter, but I guess the idea
is that the firmware picks up its dtb by looking just after its image
end (as U-Boot does, for instance). Forcing an additional 2MB alignment
sounds surprising and fragile here.

So is there any reason we can't reduce the alignment to the spec'ed 8
bytes?

On a related matter: I see that we still pass the DTB address in x0 (as
in the kernel boot protocol) and the EDKII port relies on this. Should
this be documented somewhere, then?


Apart from that the patch looks good to me.

Cheers,
Andre

> +	pr_info("Placing fdt at 0x%llx - 0x%llx",
> +		kvm->arch.dtb_guest_start,
> +		kvm->arch.dtb_guest_start + FDT_MAX_SIZE);
> +
> +	return true;
>  }
>  
>  int kvm__arch_setup_firmware(struct kvm *kvm)

Re: [PATCH kvmtool v2 4/6] arm: Support firmware loading

[Vladimir Murzin]

On 1/11/19 6:07 PM, Andre Przywara wrote:
> Does anyone know why we force a 2MB alignment (FDT_ALIGN) here? Will?

Is not it because early days fdt was mapped with a single 2M block mapping?

Vladimir

[PATCH kvmtool v2 5/6] kvm: Add arch specific reset

[Julien Thierry]

Add a callback that allows to set arch specific default values when
creating fresh VM.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arm/kvm.c         | 4 ++++
 include/kvm/kvm.h | 1 +
 kvm.c             | 2 ++
 mips/kvm.c        | 4 ++++
 powerpc/kvm.c     | 4 ++++
 x86/kvm.c         | 4 ++++
 6 files changed, 19 insertions(+)

diff --git a/arm/kvm.c b/arm/kvm.c
index d3f8f5d..50faab5 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -57,6 +57,10 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 {
 }
 
+void kvm__arch_reset(struct kvm *kvm)
+{
+}
+
 void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 {
 	/*
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 1edacfd..eeeb10c 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -136,6 +136,7 @@ int kvm__enumerate_instances(int (*callback)(const char *name, int pid));
 void kvm__remove_socket(const char *name);
 
 void kvm__arch_set_cmdline(char *cmdline, bool video);
+void kvm__arch_reset(struct kvm *kvm);
 void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size);
 void kvm__arch_delete_ram(struct kvm *kvm);
 int kvm__arch_setup_firmware(struct kvm *kvm);
diff --git a/kvm.c b/kvm.c
index d5249a0..052c6e8 100644
--- a/kvm.c
+++ b/kvm.c
@@ -160,6 +160,8 @@ struct kvm *kvm__new(void)
 	kvm->sys_fd = -1;
 	kvm->vm_fd = -1;
 
+	kvm__arch_reset(kvm);
+
 	return kvm;
 }
 
diff --git a/mips/kvm.c b/mips/kvm.c
index 211770d..3f6fd20 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -56,6 +56,10 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 
 }
 
+void kvm__arch_reset(struct kvm *kvm)
+{
+}
+
 /* Architecture-specific KVM init */
 void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 {
diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index 702d67d..5bb721b 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -87,6 +87,10 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 	/* We don't need anything unusual in here. */
 }
 
+void kvm__arch_reset(struct kvm *kvm)
+{
+}
+
 /* Architecture-specific KVM init */
 void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 {
diff --git a/x86/kvm.c b/x86/kvm.c
index 3e0f0b7..e017d36 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -129,6 +129,10 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 		strcat(cmdline, " earlyprintk=serial i8042.noaux=1");
 }
 
+void kvm__arch_reset(struct kvm *kvm)
+{
+}
+
 /* Architecture-specific KVM init */
 void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 {
-- 
1.9.1

[PATCH kvmtool v2 6/6] arm: Support non-volatile memory

[Julien Thierry]

Add an option to let user load files as non-volatile memory.

An additional range of addresses is reserved for nv memory.
Loaded files must be a multiple of the system page size.

Users can chose whether the data written by the guest modifies the original
file.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arm/fdt.c                                |  34 ++++++++
 arm/include/arm-common/kvm-arch.h        |   5 +-
 arm/include/arm-common/kvm-config-arch.h |  18 ++++-
 arm/kvm.c                                | 134 +++++++++++++++++++++++++++++++
 4 files changed, 189 insertions(+), 2 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 2936986..e3a7a64 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -72,6 +72,37 @@ static void generate_irq_prop(void *fdt, u8 irq, enum irq_type irq_type)
 	_FDT(fdt_property(fdt, "interrupts", irq_prop, sizeof(irq_prop)));
 }
 
+static void generate_nvmem_node(void *fdt, struct kvm_nv_mem *nvmem)
+{
+	char buf[22]; // 22 >= len("flash@" + 16 hex char + '\0')
+	const u64 reg_prop[] = {
+		cpu_to_fdt64(nvmem->map_addr),
+		cpu_to_fdt64(nvmem->size)
+	};
+
+	snprintf(buf, sizeof(buf), "flash@%llx", nvmem->map_addr);
+	_FDT(fdt_begin_node(fdt, buf));
+
+	_FDT(fdt_property_string(fdt, "compatible", "kvmtool,flash"));
+
+	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
+
+	_FDT(fdt_property_string(fdt, "label", nvmem->name));
+
+	_FDT(fdt_end_node(fdt));
+}
+
+static void generate_nvmem_nodes(void *fdt, struct kvm *kvm)
+{
+	struct list_head *nvmem_node;
+
+	list_for_each(nvmem_node, &kvm->cfg.arch.nvmem_list)
+		generate_nvmem_node(fdt,
+				    container_of(nvmem_node,
+					         struct kvm_nv_mem,
+					         node));
+}
+
 struct psci_fns {
 	u32 cpu_suspend;
 	u32 cpu_off;
@@ -210,6 +241,9 @@ static int setup_fdt(struct kvm *kvm)
 	_FDT(fdt_property_cell(fdt, "migrate", fns->migrate));
 	_FDT(fdt_end_node(fdt));
 
+	/* Non volatile memories */
+	generate_nvmem_nodes(fdt, kvm);
+
 	/* Finalise. */
 	_FDT(fdt_end_node(fdt));
 	_FDT(fdt_finish(fdt));
diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index b9d486d..f425d86 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -10,6 +10,7 @@
 #define ARM_IOPORT_AREA		_AC(0x0000000000000000, UL)
 #define ARM_MMIO_AREA		_AC(0x0000000000010000, UL)
 #define ARM_AXI_AREA		_AC(0x0000000040000000, UL)
+#define ARM_NVRAM_AREA		_AC(0x000000007f000000, UL)
 #define ARM_MEMORY_AREA		_AC(0x0000000080000000, UL)
 
 #define ARM_LOMAP_MAX_MEMORY	((1ULL << 32) - ARM_MEMORY_AREA)
@@ -24,9 +25,11 @@
 #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
 #define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
 #define ARM_PCI_CFG_SIZE	(1ULL << 24)
-#define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
+#define ARM_PCI_MMIO_SIZE	(ARM_NVRAM_AREA - \
 				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
 
+#define ARM_NVRAM_SIZE		(ARM_MEMORY_AREA - ARM_NVRAM_AREA)
+
 #define KVM_IOPORT_AREA		ARM_IOPORT_AREA
 #define KVM_PCI_CFG_AREA	ARM_AXI_AREA
 #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
index 5734c46..0f1fa11 100644
--- a/arm/include/arm-common/kvm-config-arch.h
+++ b/arm/include/arm-common/kvm-config-arch.h
@@ -1,8 +1,18 @@
 #ifndef ARM_COMMON__KVM_CONFIG_ARCH_H
 #define ARM_COMMON__KVM_CONFIG_ARCH_H
 
+#include <linux/list.h>
 #include "kvm/parse-options.h"
 
+struct kvm_nv_mem {
+	char			*data_file;
+	char			*name;
+	ssize_t			size;
+	u64			map_addr;
+	bool			write_back;
+	struct list_head	node;
+};
+
 struct kvm_config_arch {
 	const char	*dump_dtb_filename;
 	unsigned int	force_cntfrq;
@@ -12,9 +22,11 @@ struct kvm_config_arch {
 	u64		kaslr_seed;
 	enum irqchip_type irqchip;
 	u64		fw_addr;
+	struct list_head nvmem_list;
 };
 
 int irqchip_parser(const struct option *opt, const char *arg, int unset);
+int nvmem_parser(const struct option *opt, const char *arg, int unset);
 
 #define OPT_ARCH_RUN(pfx, cfg)							\
 	pfx,									\
@@ -33,6 +45,10 @@ int irqchip_parser(const struct option *opt, const char *arg, int unset);
 		     "Type of interrupt controller to emulate in the guest",	\
 		     irqchip_parser, NULL),					\
 	OPT_U64('\0', "firmware-address", &(cfg)->fw_addr,			\
-		"Address where firmware should be loaded"),
+		"Address where firmware should be loaded"),			\
+	OPT_CALLBACK('\0', "nvmem", NULL,					\
+		     "<file>,<label>[,wb]",					\
+		     "Load <file> as non-volatile memory",			\
+		     nvmem_parser, kvm),
 
 #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index 50faab5..eaa7005 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -18,6 +18,53 @@ struct kvm_ext kvm_req_ext[] = {
 	{ 0, 0 },
 };
 
+int nvmem_parser(const struct option *opt, const char *arg, int unset)
+{
+	struct kvm *kvm = (struct kvm*) opt->ptr;
+	struct kvm_nv_mem *nvmem;
+	struct stat st;
+	const char *ptr;
+	uint32_t len;
+
+	nvmem = calloc(sizeof (*nvmem), 1);
+
+	if (!nvmem)
+		die("nvmem: cannot add non-volatile memory");
+
+	ptr = strstr(arg, ",");
+
+	if (!ptr)
+		die("nvmem: missing name for non-volatile memory");
+
+	len = ptr - arg + 1;
+	nvmem->data_file = malloc(len);
+	strncpy(nvmem->data_file, arg, len);
+	nvmem->data_file[len - 1] = '\0';
+
+	if (stat(nvmem->data_file, &st))
+		die("nvmem: failed to stat data file");
+	nvmem->size = st.st_size;
+
+	arg = arg + len;
+	for (ptr = arg; *ptr != '\0' && *ptr != ','; ++ptr)
+		;
+	len = ptr - arg + 1;
+	nvmem->name = malloc(len);
+	strncpy(nvmem->name, arg, len);
+	nvmem->name[len - 1] = '\0';
+
+	if (*ptr == ',') {
+		if (!strcmp(ptr + 1, "wb"))
+			nvmem->write_back = true;
+		else
+			die("firmware-data: invalid option %s", ptr + 1);
+	}
+
+	list_add(&nvmem->node, &kvm->cfg.arch.nvmem_list);
+
+	return 0;
+}
+
 bool kvm__arch_cpu_supports_vm(void)
 {
 	/* The KVM capability check is enough. */
@@ -59,6 +106,7 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 
 void kvm__arch_reset(struct kvm *kvm)
 {
+	INIT_LIST_HEAD(&kvm->cfg.arch.nvmem_list);
 }
 
 void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
@@ -241,3 +289,89 @@ int kvm__arch_setup_firmware(struct kvm *kvm)
 {
 	return 0;
 }
+
+static int setup_nvmem(struct kvm *kvm)
+{
+	u64 map_address = ARM_NVRAM_AREA;
+	const u64 limit = ARM_NVRAM_AREA + ARM_NVRAM_SIZE;
+	struct list_head *nvmem_node;
+	const int pagesize = getpagesize();
+
+	list_for_each(nvmem_node, &kvm->cfg.arch.nvmem_list) {
+		struct kvm_nv_mem *nvmem = container_of(nvmem_node,
+							struct kvm_nv_mem,
+							node);
+		void *user_addr;
+		int fd;
+
+		if (map_address + nvmem->size > limit)
+			die("cannot map file %s in non-volatile memory, no space left",
+			    nvmem->data_file);
+
+		if (nvmem->size & (pagesize - 1))
+			die("size of non-volatile memory files must be a multiple of system page size (= %d)",
+			    pagesize);
+
+		user_addr = mmap(NULL, nvmem->size, PROT_RW, MAP_ANON_NORESERVE, -1, 0);
+		if (user_addr == MAP_FAILED)
+			die("cannot create mapping for file %s",
+			     nvmem->data_file);
+
+		fd = open(nvmem->data_file, O_RDONLY);
+		if (fd < 0)
+			die("cannot read file %s", nvmem->data_file);
+		if (read_file(fd, user_addr, nvmem->size) < 0)
+			die("failed to map nv memory data %s",
+			    nvmem->data_file);
+		close(fd);
+
+		if (kvm__register_dev_mem(kvm, map_address, nvmem->size,
+					  user_addr))
+			die("failed to register nv memory mapping for guest");
+
+		nvmem->map_addr = map_address;
+		map_address += nvmem->size;
+	}
+
+	return 0;
+}
+firmware_init(setup_nvmem);
+
+static int flush_nv_mem(struct kvm *kvm)
+{
+	struct list_head *nvmem_node;
+	int err = 0;
+
+	list_for_each(nvmem_node, &kvm->cfg.arch.nvmem_list) {
+		struct kvm_nv_mem *nvmem = container_of(nvmem_node,
+							struct kvm_nv_mem,
+							node);
+		void *host_pos;
+
+		host_pos = guest_flat_to_host(kvm,
+					      nvmem->map_addr);
+
+		if (nvmem->write_back) {
+			int fd;
+
+			fd = open(nvmem->data_file, O_WRONLY);
+			if (fd < 0) {
+				pr_err("failed to open firmware data file for writting");
+				err = -1;
+				continue;
+			}
+
+			if (write_in_full(fd, host_pos, nvmem->size) < 0) {
+				pr_err("failed to flush firmware data to file");
+				err = -1;
+			}
+			close(fd);
+		}
+
+		if (munmap(host_pos, nvmem->size))
+			err = -1;
+	}
+
+	return err;
+}
+firmware_exit(flush_nv_mem);
-- 
1.9.1

Re: [PATCH kvmtool v2 0/6] arm: Add support for firmware booting

[Will Deacon]

Hi Julien,

On Thu, Jan 10, 2019 at 02:20:40PM +0000, Julien Thierry wrote:
> This series is based on the virtio reset series[1] posted earlier.
> 
> We would like to be able to load firmwares like UEFI in kvmtool.
> 
> The series contains:
> A way to load the firmware into RAM and an option to be able to create
> non-volatile memory zones and load data into them.
> 
> Those non-volatile memory are presented throught the DT with a node:
> 
> 	<flash>@<addr> {
> 		compatible = "kvmtool,flash";
> 		reg = < <addr> <size> >;
> 		label = <name>;
> 	}
> 
> These are expected to be dealt with by specific kvmtool driver and not
> to be picked up by existing drivers (although technically it is just
> plain memory, mapped in the guest).

I've picked up the first four patches of this series, but I don't really
understand where you're going with the non-volatile memory part and whether
it's nvram, flash or something completely different. Given that Linux
doesn't support your binding, is this something that UEFI currently uses?

Will

Re: [PATCH kvmtool v2 0/6] arm: Add support for firmware booting

[Julien Thierry]

Hi Will,

On 22/01/2019 07:10, Will Deacon wrote:
> Hi Julien,
> 
> On Thu, Jan 10, 2019 at 02:20:40PM +0000, Julien Thierry wrote:
>> This series is based on the virtio reset series[1] posted earlier.
>>
>> We would like to be able to load firmwares like UEFI in kvmtool.
>>
>> The series contains:
>> A way to load the firmware into RAM and an option to be able to create
>> non-volatile memory zones and load data into them.
>>
>> Those non-volatile memory are presented throught the DT with a node:
>>
>> 	<flash>@<addr> {
>> 		compatible = "kvmtool,flash";
>> 		reg = < <addr> <size> >;
>> 		label = <name>;
>> 	}
>>
>> These are expected to be dealt with by specific kvmtool driver and not
>> to be picked up by existing drivers (although technically it is just
>> plain memory, mapped in the guest).
> 
> I've picked up the first four patches of this series, but I don't really

Thanks!

> understand where you're going with the non-volatile memory part and whether
> it's nvram, flash or something completely different. Given that Linux
> doesn't support your binding, is this something that UEFI currently uses?
> 

So the thing is that UEFI/EDK2 needs some kind of readable and writable
non-volatile memory (flash or nv-ram, not sure what's the difference) to
load/store its environment. For kvmtool, we can just provide memory
region where the environment is mapped and EDK2 can just directly read
from or write to it.

The thing is, when looking at the flash binding in Linux, each platform
seems to have its own binding of their flash device (Which I guess makes
sense since they don't necessarily work the same way). I couldn't find
any existing generic bindings that seemed to fit what I'm presenting to
EDK2 (which could also be used by another firmware if they needed a
binary blob in flash or something).

Another thing I wanted to avoid would be an existing Linux driver
picking up on the "flash device" kvmtool is presenting (don't want it
directly messing up with EFI environment). The idea is that you would
use this memory only if you explicitly added the support in the software
for this platform.

EDK2 doesn't already use it (not upstream), but it is part of the
support to boot EDK2 under kvmtool series (on the EDK2 side). So, this
is not set in stone yet and if you suggestion I can still check with
Sami if that works on his side and implement it.

Thanks,

-- 
Julien Thierry

Re: [PATCH kvmtool v2 0/6] arm: Add support for firmware booting

[Will Deacon]

On Tue, Jan 22, 2019 at 09:00:01AM +0000, Julien Thierry wrote:
> On 22/01/2019 07:10, Will Deacon wrote:
> > I've picked up the first four patches of this series, but I don't really
> 
> Thanks!
> 
> > understand where you're going with the non-volatile memory part and whether
> > it's nvram, flash or something completely different. Given that Linux
> > doesn't support your binding, is this something that UEFI currently uses?
> > 
> 
> So the thing is that UEFI/EDK2 needs some kind of readable and writable
> non-volatile memory (flash or nv-ram, not sure what's the difference) to
> load/store its environment. For kvmtool, we can just provide memory
> region where the environment is mapped and EDK2 can just directly read
> from or write to it.
> 
> The thing is, when looking at the flash binding in Linux, each platform
> seems to have its own binding of their flash device (Which I guess makes
> sense since they don't necessarily work the same way). I couldn't find
> any existing generic bindings that seemed to fit what I'm presenting to
> EDK2 (which could also be used by another firmware if they needed a
> binary blob in flash or something).

We could probably abuse memreserve for this, which I think happens in the
wild already for non-memory regions. However, if the non-volatile region is
outside of ram, then why do we care at all? Is UEFI parsing the dts to
locate it? If so, I'd have thought that they would need to get a binding
sorted for that purpose, in which case we can just use that.

Will