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

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

[Julien Thierry]

Hi,

This series is based on the virtio reset series posted earlier:
https://marc.info/?l=kvm&m=154392174625966&w=2

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:

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

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).

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                                |  67 ++++++++--
 arm/include/arm-common/kvm-arch.h        |   5 +-
 arm/include/arm-common/kvm-config-arch.h |  21 +++-
 arm/kvm.c                                | 204 +++++++++++++++++++++++++++++++
 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, 323 insertions(+), 21 deletions(-)

--
1.9.1

[PATCH kvmtool 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>
---
 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

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

[Andre Przywara]

On Tue,  4 Dec 2018 11:14:28 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

> 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>

Checked against the data sheet.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

This is quite interesting: we build the RTC emulation unconditionally
for every architecture, but don't expose it in the DT (for arm/arm64).
The Linux driver can't even be configured for arm64.
Interestingly it works if one pokes 0x70 and 0x71 directly in memory
from a guest. Which sounds hackish (do we want that?), but fits more a
less the firmware use case. We would just need to make sure it actually
works correctly on ARM, since nobody tested this properly before.

I guess EDK2 would just hardcode the address?

Cheers,
Andre.

> ---
>  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);

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

[Sami Mujawar]

Hi Andre,

We are using the edk2 PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe RTC driver. This RTC driver derives the IO port values for the Index and Target register from Pcds (Platform Configuration Database), which can be considered as constants defined at build time, so sort of hardcoded.
We have also added MMIO capability to this driver (enabled by setting the PcdRtcUseMmio to TRUE). This change effectively performs MMIO accesses on 0x70 and 0x71.

Regards,

Sami Mujawar

-----Original Message-----
From: Andre Przywara <andre.przywara@arm.com>
Sent: 12 December 2018 06:16 PM
To: Julien Thierry <Julien.Thierry@arm.com>
Cc: kvm@vger.kernel.org; kvmarm@lists.cs.columbia.edu; Will Deacon <Will.Deacon@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Mark Rutland <Mark.Rutland@arm.com>
Subject: Re: [PATCH kvmtool 1/6] rtc: Initialize the Register D for MC146818 RTC

On Tue,  4 Dec 2018 11:14:28 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

> 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>

Checked against the data sheet.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

This is quite interesting: we build the RTC emulation unconditionally for every architecture, but don't expose it in the DT (for arm/arm64).
The Linux driver can't even be configured for arm64.
Interestingly it works if one pokes 0x70 and 0x71 directly in memory from a guest. Which sounds hackish (do we want that?), but fits more a less the firmware use case. We would just need to make sure it actually works correctly on ARM, since nobody tested this properly before.

I guess EDK2 would just hardcode the address?

Cheers,
Andre.

> ---
>  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_C0x0C
>  #define RTC_REG_D0x0D
>
> +/*
> + * Register D Bits
> + */
> +#define RTC_REG_D_VRT(1 << 7)
> +
>  struct rtc_device {
>  u8cmos_idx;
>  u8cmos_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);

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH kvmtool 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>
---
 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

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

[Andre Przywara]

On Tue,  4 Dec 2018 11:14:29 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

> 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>

Thanks,
Andre.

> ---
>  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;
> +}

[PATCH kvmtool 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>
---
 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

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

[Andre Przywara]

On Tue,  4 Dec 2018 11:14:30 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

Hi,

> 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>

Just wondering whether we actually need this unconditional printf in
the first place ...

Cheers,
Andre.

> ---
>  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");

[PATCH kvmtool 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                                | 58 +++++++++++++++++++++++++++++++-
 3 files changed, 74 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..d5bbbc3 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -169,9 +169,65 @@ 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 = kvm->cfg.arch.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;
+
+	if (!validate_fw_addr(kvm))
+		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 4/6] arm: Support firmware loading

[Andre Przywara]

On Tue,  4 Dec 2018 11:14:31 +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.

How is the user supposed to know this address? Will EDKII be linked
against a certain address, which has to be reflected in this parameter?

Wouldn't it be feasible to provide a default value, say the beginning
of DRAM? (See below)

> 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                                | 58
> +++++++++++++++++++++++++++++++- 3 files changed, 74 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..d5bbbc3 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -169,9 +169,65 @@ 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 = kvm->cfg.arch.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;

What about we check here for fw_addr being 0, which is the default
value and would be rejected by the next call.
So can't we set it to some sensible default value here?

	/* If not specified, use default load address at start of RAM */
	if (fw_addr == 0)
		fw_addr = ARM_MEMORY_AREA;

Cheers,
Andre.

> +	if (!validate_fw_addr(kvm))
> +		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)

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

[Julien Thierry]

Hi,

On 14/12/2018 18:08, Andre Przywara wrote:
> On Tue,  4 Dec 2018 11:14:31 +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.
> 
> How is the user supposed to know this address? Will EDKII be linked
> against a certain address, which has to be reflected in this parameter?
> 

For EDKII I believe any address in RAM is fine (although I'm unsure
whether there are other alignment properties).

For other firmwares/bootloaders, only the user can know depending on
what they are loading.

> Wouldn't it be feasible to provide a default value, say the beginning
> of DRAM? (See below)
> 
>> 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                                | 58
>> +++++++++++++++++++++++++++++++- 3 files changed, 74 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..d5bbbc3 100644
>> --- a/arm/kvm.c
>> +++ b/arm/kvm.c
>> @@ -169,9 +169,65 @@ 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 = kvm->cfg.arch.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;
> 
> What about we check here for fw_addr being 0, which is the default
> value and would be rejected by the next call.
> So can't we set it to some sensible default value here?
> 
> 	/* If not specified, use default load address at start of RAM */
> 	if (fw_addr == 0)
> 		fw_addr = ARM_MEMORY_AREA;
> 

Seems a bit random to me, but I guess we can. After all for the kernel
image we just to put it at the end of DRAM.

Thanks,

Julien

> 
>> +	if (!validate_fw_addr(kvm))
>> +		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)
> 

-- 
Julien Thierry

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

[André Przywara]

On 17/12/2018 10:05, Julien Thierry wrote:

Hi Julien,

> On 14/12/2018 18:08, Andre Przywara wrote:
>> On Tue,  4 Dec 2018 11:14:31 +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.
>>
>> How is the user supposed to know this address? Will EDKII be linked
>> against a certain address, which has to be reflected in this parameter?
>>
> 
> For EDKII I believe any address in RAM is fine (although I'm unsure
> whether there are other alignment properties).

Yes, speaking with Sami on Friday and actually trying it indeed EDKII
does not require a certain load address.

> For other firmwares/bootloaders, only the user can know depending on
> what they are loading.

Sure, but the majority of people will use EDKII, so let's make this
easier for them, without falling back to the old ARM habit of requiring
users to provide magic addresses just for getting something to boot ;-)

Thanks,
Andre.

>> Wouldn't it be feasible to provide a default value, say the beginning
>> of DRAM? (See below)
>>
>>> 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                                | 58
>>> +++++++++++++++++++++++++++++++- 3 files changed, 74 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..d5bbbc3 100644
>>> --- a/arm/kvm.c
>>> +++ b/arm/kvm.c
>>> @@ -169,9 +169,65 @@ 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 = kvm->cfg.arch.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;
>>
>> What about we check here for fw_addr being 0, which is the default
>> value and would be rejected by the next call.
>> So can't we set it to some sensible default value here?
>>
>> 	/* If not specified, use default load address at start of RAM */
>> 	if (fw_addr == 0)
>> 		fw_addr = ARM_MEMORY_AREA;
>>
> 
> Seems a bit random to me, but I guess we can. After all for the kernel
> image we just to put it at the end of DRAM.
> 
> Thanks,
> 
> Julien
> 
>>
>>> +	if (!validate_fw_addr(kvm))
>>> +		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)
>>
> 

[PATCH kvmtool 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 d5bbbc3..1a2cfdc 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 7de825a..05ad0b6 100644
--- a/kvm.c
+++ b/kvm.c
@@ -158,6 +158,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

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

[Andre Przywara]

On Tue,  4 Dec 2018 11:14:32 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

Hi,

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

So is this needed just for initialising the LIST_HEAD in the next
patch? Can't we use some dev_init() call for that purpose, or even add
the nonvolatile memory device to all architectures and init this in one
place?

Cheers,
Andre.

> 
> 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 d5bbbc3..1a2cfdc 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 7de825a..05ad0b6 100644
> --- a/kvm.c
> +++ b/kvm.c
> @@ -158,6 +158,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) {

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

[Julien Thierry]

Hi,

On 14/12/2018 18:11, Andre Przywara wrote:
> On Tue,  4 Dec 2018 11:14:32 +0000
> Julien Thierry <julien.thierry@arm.com> wrote:
> 
> Hi,
> 
>> Add a callback that allows to set arch specific default values when
>> creating fresh VM.
> 
> So is this needed just for initialising the LIST_HEAD in the next
> patch? Can't we use some dev_init() call for that purpose, or even add
> the nonvolatile memory device to all architectures and init this in one
> place?
> 

All the *_init() calls are done after option parsing, so this is not good.

And I'm really unsure about adding nvmem for all arch as:
1) I'm not familiar with the memory layouts of other archs and wouldn't
know where to map them.
2) I wouldn't know how to expose them as not all archs use DT (currently
only arm/arm64 and powerpc seem to really require it).

Or we could introduce an ARCH_WANT_NVMEM, and CONFIG_HAS_NVMEM. And when
this is defined, the ARCH is required to provide an NVMEM_AREA when they
want to start using ARCH_WANT_NVMEM.

This way there is a base for other achitectures to start supporting it
but avoids me breaking their memory layout or introducing an unusable
option for them.

Thanks,

Julien

> 
>>
>> 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 d5bbbc3..1a2cfdc 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 7de825a..05ad0b6 100644
>> --- a/kvm.c
>> +++ b/kvm.c
>> @@ -158,6 +158,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) {
> 

-- 
Julien Thierry

[PATCH kvmtool 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                                |  43 ++++++++++
 arm/include/arm-common/kvm-arch.h        |   5 +-
 arm/include/arm-common/kvm-config-arch.h |  18 ++++-
 arm/kvm.c                                | 134 +++++++++++++++++++++++++++++++
 4 files changed, 198 insertions(+), 2 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 2936986..fd482ce 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -72,6 +72,46 @@ 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;
+	int len;
+	const u64 reg_prop[] = {
+		cpu_to_fdt64(nvmem->map_addr),
+		cpu_to_fdt64(nvmem->size)
+	};
+
+	/* Name length + '@' + 8 hex characters + '\0' */
+	len = strlen(nvmem->name) + 10;
+	buf = malloc(len);
+	snprintf(buf, len, "%s@%llx", nvmem->name,
+		 nvmem->map_addr);
+	_FDT(fdt_begin_node(fdt, buf));
+	free(buf);
+
+	/* Name length + "kvmtool," + '\0' */
+	len = strlen(nvmem->name) + 9;
+	buf = malloc(len);
+	snprintf(buf, len, "kvmtool,%s", nvmem->name);
+	_FDT(fdt_property_string(fdt, "compatible", buf));
+	free(buf);
+
+	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
+
+	_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 +250,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..d5742cb 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>,<name>[,wb]",					\
+		     "Load <file> as non-volatile memory kvmtool,<name>",	\
+		     nvmem_parser, kvm),
 
 #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index 1a2cfdc..00675d9 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)
@@ -238,3 +286,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 6/6] arm: Support non-volatile memory

[Andre Przywara]

On Tue,  4 Dec 2018 11:14:33 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

Hi,

> 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                                |  43 ++++++++++
>  arm/include/arm-common/kvm-arch.h        |   5 +-
>  arm/include/arm-common/kvm-config-arch.h |  18 ++++-
>  arm/kvm.c                                | 134
> +++++++++++++++++++++++++++++++ 4 files changed, 198 insertions(+), 2
> deletions(-)
> 
> diff --git a/arm/fdt.c b/arm/fdt.c
> index 2936986..fd482ce 100644
> --- a/arm/fdt.c
> +++ b/arm/fdt.c
> @@ -72,6 +72,46 @@ 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;
> +	int len;
> +	const u64 reg_prop[] = {
> +		cpu_to_fdt64(nvmem->map_addr),
> +		cpu_to_fdt64(nvmem->size)
> +	};
> +
> +	/* Name length + '@' + 8 hex characters + '\0' */
> +	len = strlen(nvmem->name) + 10;
> +	buf = malloc(len);
> +	snprintf(buf, len, "%s@%llx", nvmem->name,
> +		 nvmem->map_addr);
> +	_FDT(fdt_begin_node(fdt, buf));
> +	free(buf);
> +
> +	/* Name length + "kvmtool," + '\0' */
> +	len = strlen(nvmem->name) + 9;
> +	buf = malloc(len);
> +	snprintf(buf, len, "kvmtool,%s", nvmem->name);
> +	_FDT(fdt_property_string(fdt, "compatible", buf));

As discussed in person, it doesn't sound right to (ab)use the
compatible name for this. This one should describe a device type, not
an instance of it.
So I would suggest to use a fixed compatible name (say:
"kvmtool,flash"), then put the designator in a property.
	flash@7f000000 {
		compatible = "kvmtool,flash";
		label = "environment";
		reg = <0 0x7f000000 0 0x10000>;
	};
So the label property would reflect the user provided name.
Also there is the generic "read-only" property, which we might want to
use in case "wb" is not provided.

I am not overly happy with inventing a new compatible name for such a
generic device, but couldn't find anything better (mtd-ram, mtd-rom,
cfi-flash were close, but not a complete fit). Also given that the idea
is to just communicate this from kvmtool to EDK II (without Linux ever
picking this up), I guess this solution works.

Cheers,
Andre,

> +	free(buf);
> +
> +	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
> +
> +	_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 +250,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..d5742cb
> 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>,<name>[,wb]",					\
> +		     "Load <file> as non-volatile memory
> kvmtool,<name>",	\
> +		     nvmem_parser, kvm),
>  
>  #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index 1a2cfdc..00675d9 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) @@ -238,3 +286,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);

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

[Julien Thierry]

Hi,

On 14/12/2018 18:09, Andre Przywara wrote:
> On Tue,  4 Dec 2018 11:14:33 +0000
> Julien Thierry <julien.thierry@arm.com> wrote:
> 
> Hi,
> 
>> 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                                |  43 ++++++++++
>>  arm/include/arm-common/kvm-arch.h        |   5 +-
>>  arm/include/arm-common/kvm-config-arch.h |  18 ++++-
>>  arm/kvm.c                                | 134
>> +++++++++++++++++++++++++++++++ 4 files changed, 198 insertions(+), 2
>> deletions(-)
>>
>> diff --git a/arm/fdt.c b/arm/fdt.c
>> index 2936986..fd482ce 100644
>> --- a/arm/fdt.c
>> +++ b/arm/fdt.c
>> @@ -72,6 +72,46 @@ 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;
>> +	int len;
>> +	const u64 reg_prop[] = {
>> +		cpu_to_fdt64(nvmem->map_addr),
>> +		cpu_to_fdt64(nvmem->size)
>> +	};
>> +
>> +	/* Name length + '@' + 8 hex characters + '\0' */
>> +	len = strlen(nvmem->name) + 10;
>> +	buf = malloc(len);
>> +	snprintf(buf, len, "%s@%llx", nvmem->name,
>> +		 nvmem->map_addr);
>> +	_FDT(fdt_begin_node(fdt, buf));
>> +	free(buf);
>> +
>> +	/* Name length + "kvmtool," + '\0' */
>> +	len = strlen(nvmem->name) + 9;
>> +	buf = malloc(len);
>> +	snprintf(buf, len, "kvmtool,%s", nvmem->name);
>> +	_FDT(fdt_property_string(fdt, "compatible", buf));
> 
> As discussed in person, it doesn't sound right to (ab)use the
> compatible name for this. This one should describe a device type, not
> an instance of it.
> So I would suggest to use a fixed compatible name (say:
> "kvmtool,flash"), then put the designator in a property.
> 	flash@7f000000 {
> 		compatible = "kvmtool,flash";
> 		label = "environment";
> 		reg = <0 0x7f000000 0 0x10000>;
> 	};

Yes, that sounds better than the current situation. Thanks for the
suggestion.

> So the label property would reflect the user provided name.
> Also there is the generic "read-only" property, which we might want to
> use in case "wb" is not provided.
> 

So, I was not really seeing the "wb" as "the guest shouldn't write to
this", but rather "do you want the file to be updated by what the guest
writes?".

I was more thinking about the case where you want to start guests with
the same starting data multiple times, without having to back up your
data files.

Does that make sense?

Thanks,

Julien

> I am not overly happy with inventing a new compatible name for such a
> generic device, but couldn't find anything better (mtd-ram, mtd-rom,
> cfi-flash were close, but not a complete fit). Also given that the idea
> is to just communicate this from kvmtool to EDK II (without Linux ever
> picking this up), I guess this solution works.
> 
> Cheers,
> Andre,
> 
>> +	free(buf);
>> +
>> +	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
>> +
>> +	_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 +250,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..d5742cb
>> 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>,<name>[,wb]",					\
>> +		     "Load <file> as non-volatile memory
>> kvmtool,<name>",	\
>> +		     nvmem_parser, kvm),
>>  
>>  #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
>> diff --git a/arm/kvm.c b/arm/kvm.c
>> index 1a2cfdc..00675d9 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) @@ -238,3 +286,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);
> 

-- 
Julien Thierry

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

[André Przywara]

On 17/12/2018 10:31, Julien Thierry wrote:
> Hi,
> 
> On 14/12/2018 18:09, Andre Przywara wrote:
>> On Tue,  4 Dec 2018 11:14:33 +0000
>> Julien Thierry <julien.thierry@arm.com> wrote:
>>
>> Hi,
>>
>>> 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                                |  43 ++++++++++
>>>  arm/include/arm-common/kvm-arch.h        |   5 +-
>>>  arm/include/arm-common/kvm-config-arch.h |  18 ++++-
>>>  arm/kvm.c                                | 134
>>> +++++++++++++++++++++++++++++++ 4 files changed, 198 insertions(+), 2
>>> deletions(-)
>>>
>>> diff --git a/arm/fdt.c b/arm/fdt.c
>>> index 2936986..fd482ce 100644
>>> --- a/arm/fdt.c
>>> +++ b/arm/fdt.c
>>> @@ -72,6 +72,46 @@ 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;
>>> +	int len;
>>> +	const u64 reg_prop[] = {
>>> +		cpu_to_fdt64(nvmem->map_addr),
>>> +		cpu_to_fdt64(nvmem->size)
>>> +	};
>>> +
>>> +	/* Name length + '@' + 8 hex characters + '\0' */
>>> +	len = strlen(nvmem->name) + 10;
>>> +	buf = malloc(len);
>>> +	snprintf(buf, len, "%s@%llx", nvmem->name,
>>> +		 nvmem->map_addr);
>>> +	_FDT(fdt_begin_node(fdt, buf));
>>> +	free(buf);
>>> +
>>> +	/* Name length + "kvmtool," + '\0' */
>>> +	len = strlen(nvmem->name) + 9;
>>> +	buf = malloc(len);
>>> +	snprintf(buf, len, "kvmtool,%s", nvmem->name);
>>> +	_FDT(fdt_property_string(fdt, "compatible", buf));
>>
>> As discussed in person, it doesn't sound right to (ab)use the
>> compatible name for this. This one should describe a device type, not
>> an instance of it.
>> So I would suggest to use a fixed compatible name (say:
>> "kvmtool,flash"), then put the designator in a property.
>> 	flash@7f000000 {
>> 		compatible = "kvmtool,flash";
>> 		label = "environment";
>> 		reg = <0 0x7f000000 0 0x10000>;
>> 	};
> 
> Yes, that sounds better than the current situation. Thanks for the
> suggestion.
> 
>> So the label property would reflect the user provided name.
>> Also there is the generic "read-only" property, which we might want to
>> use in case "wb" is not provided.
>>
> 
> So, I was not really seeing the "wb" as "the guest shouldn't write to
> this", but rather "do you want the file to be updated by what the guest
> writes?".
> 
> I was more thinking about the case where you want to start guests with
> the same starting data multiple times, without having to back up your
> data files.
> 
> Does that make sense?

Yes, you are right, those are indeed two separate concepts.

Cheers,
Andre

>> I am not overly happy with inventing a new compatible name for such a
>> generic device, but couldn't find anything better (mtd-ram, mtd-rom,
>> cfi-flash were close, but not a complete fit). Also given that the idea
>> is to just communicate this from kvmtool to EDK II (without Linux ever
>> picking this up), I guess this solution works.
>>
>> Cheers,
>> Andre,
>>
>>> +	free(buf);
>>> +
>>> +	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
>>> +
>>> +	_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 +250,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..d5742cb
>>> 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>,<name>[,wb]",					\
>>> +		     "Load <file> as non-volatile memory
>>> kvmtool,<name>",	\
>>> +		     nvmem_parser, kvm),
>>>  
>>>  #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
>>> diff --git a/arm/kvm.c b/arm/kvm.c
>>> index 1a2cfdc..00675d9 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) @@ -238,3 +286,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);
>>
>