[PATCH V4 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[PATCH V4 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Sunil V L]

This series adds the support to boot S-mode FW like EDK2 from the flash. The
S-mode firmware should be kept in pflash unit 1.

When -kernel (and -initrd) option is also provided along with the flash,
the kernel (and initrd) will be loaded into fw_cfg table and opensbi will
branch to the flash address which will be the entry point of the S-mode
firmware. The S-mode FW then loads and launches the kernel.

When only -pflash option is provided in the command line, the kernel
will be located and loaded in the usual way by the S-mode firmware.

These patches are available in below branch.
https://github.com/vlsunil/qemu/tree/pflash_v2

The first two patches in this series are refactor patches.

These changes are tested with a WIP EDK2 port for virt machine. Below
are the instructions to build and test this feature.

1) Get EDK2 sources from below branches.
https://github.com/vlsunil/edk2/tree/virt_refactor_smode_v1
https://github.com/vlsunil/edk2-platforms/tree/virt_refactor_smode_v1

2) Build EDK2 for RISC-V
	export WORKSPACE=`pwd`
        export GCC5_RISCV64_PREFIX=riscv64-linux-gnu-
        export PACKAGES_PATH=$WORKSPACE/edk2:$WORKSPACE/edk2-platforms
        export EDK_TOOLS_PATH=$WORKSPACE/edk2/BaseTools
        source edk2/edksetup.sh
        make -C edk2/BaseTools clean
        make -C edk2/BaseTools
        make -C edk2/BaseTools/Source/C
        source edk2/edksetup.sh BaseTools
        build -a RISCV64  -p Platform/Qemu/RiscVVirt/RiscVVirt.dsc -t GCC5

3)Make the EDK2 image size to match with what qemu flash expects
truncate -s 32M Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd

4) Run
a) Boot to EFI shell (no -kernel / -initrd option)
qemu-system-riscv64  -nographic   -drive file=Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1  -machine virt -M 2G

b) With -kernel, -initrd and -pflash
qemu-system-riscv64  -nographic   -drive file=Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1  -machine virt -M 2G -kernel arch/riscv/boot/Image.gz -initrd rootfs.cpio 


Changes since V3:
	1) White space and comment edits
	2) Added RB tag

Changes since V2:
	1) Moved the doc comment to .h file

Changes since V1:
	1) Modified code to support the use case when both -kernel and -pflash are configured.
	2) Refactor patches added to help (1) above.
	3) Cover letter added with test instructions.


Sunil V L (3):
  hw/arm,loongarch: Move load_image_to_fw_cfg() to common location
  hw/riscv: virt: Move create_fw_cfg() prior to loading kernel
  hw/riscv: virt: Enable booting S-mode firmware from pflash

 hw/arm/boot.c             | 49 ---------------------------------------
 hw/loongarch/virt.c       | 33 --------------------------
 hw/nvram/fw_cfg.c         | 32 +++++++++++++++++++++++++
 hw/riscv/boot.c           | 29 +++++++++++++++++++++++
 hw/riscv/virt.c           | 32 ++++++++++++++++++-------
 include/hw/nvram/fw_cfg.h | 21 +++++++++++++++++
 include/hw/riscv/boot.h   |  1 +
 7 files changed, 107 insertions(+), 90 deletions(-)

-- 
2.25.1

[PATCH V4 1/3] hw/arm, loongarch: Move load_image_to_fw_cfg() to common location

[Sunil V L]

load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
function will be required by riscv too. So, it's time to refactor and
move this function to a common path.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 hw/arm/boot.c             | 49 ---------------------------------------
 hw/loongarch/virt.c       | 33 --------------------------
 hw/nvram/fw_cfg.c         | 32 +++++++++++++++++++++++++
 include/hw/nvram/fw_cfg.h | 21 +++++++++++++++++
 4 files changed, 53 insertions(+), 82 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..704f368d9c 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -818,55 +818,6 @@ static void do_cpu_reset(void *opaque)
     }
 }
 
-/**
- * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
- *                          by key.
- * @fw_cfg:         The firmware config instance to store the data in.
- * @size_key:       The firmware config key to store the size of the loaded
- *                  data under, with fw_cfg_add_i32().
- * @data_key:       The firmware config key to store the loaded data under,
- *                  with fw_cfg_add_bytes().
- * @image_name:     The name of the image file to load. If it is NULL, the
- *                  function returns without doing anything.
- * @try_decompress: Whether the image should be decompressed (gunzipped) before
- *                  adding it to fw_cfg. If decompression fails, the image is
- *                  loaded as-is.
- *
- * In case of failure, the function prints an error message to stderr and the
- * process exits with status 1.
- */
-static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
-                                 uint16_t data_key, const char *image_name,
-                                 bool try_decompress)
-{
-    size_t size = -1;
-    uint8_t *data;
-
-    if (image_name == NULL) {
-        return;
-    }
-
-    if (try_decompress) {
-        size = load_image_gzipped_buffer(image_name,
-                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
-    }
-
-    if (size == (size_t)-1) {
-        gchar *contents;
-        gsize length;
-
-        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
-            error_report("failed to load \"%s\"", image_name);
-            exit(1);
-        }
-        size = length;
-        data = (uint8_t *)contents;
-    }
-
-    fw_cfg_add_i32(fw_cfg, size_key, size);
-    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
-}
-
 static int do_arm_linux_init(Object *obj, void *opaque)
 {
     if (object_dynamic_cast(obj, TYPE_ARM_LINUX_BOOT_IF)) {
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 5cc0b05538..eee2c193c0 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -542,39 +542,6 @@ static void reset_load_elf(void *opaque)
     }
 }
 
-/* Load an image file into an fw_cfg entry identified by key. */
-static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
-                                 uint16_t data_key, const char *image_name,
-                                 bool try_decompress)
-{
-    size_t size = -1;
-    uint8_t *data;
-
-    if (image_name == NULL) {
-        return;
-    }
-
-    if (try_decompress) {
-        size = load_image_gzipped_buffer(image_name,
-                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
-    }
-
-    if (size == (size_t)-1) {
-        gchar *contents;
-        gsize length;
-
-        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
-            error_report("failed to load \"%s\"", image_name);
-            exit(1);
-        }
-        size = length;
-        data = (uint8_t *)contents;
-    }
-
-    fw_cfg_add_i32(fw_cfg, size_key, size);
-    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
-}
-
 static void fw_cfg_add_kernel_info(FWCfgState *fw_cfg)
 {
     /*
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d605f3f45a..371a45dfe2 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -41,6 +41,7 @@
 #include "qapi/error.h"
 #include "hw/acpi/aml-build.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/loader.h"
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
@@ -1221,6 +1222,37 @@ FWCfgState *fw_cfg_find(void)
     return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
 }
 
+void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
+                          uint16_t data_key, const char *image_name,
+                          bool try_decompress)
+{
+    size_t size = -1;
+    uint8_t *data;
+
+    if (image_name == NULL) {
+        return;
+    }
+
+    if (try_decompress) {
+        size = load_image_gzipped_buffer(image_name,
+                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
+    }
+
+    if (size == (size_t)-1) {
+        gchar *contents;
+        gsize length;
+
+        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
+            error_report("failed to load \"%s\"", image_name);
+            exit(1);
+        }
+        size = length;
+        data = (uint8_t *)contents;
+    }
+
+    fw_cfg_add_i32(fw_cfg, size_key, size);
+    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
+}
 
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 0e7a8bc7af..c1f81a5f13 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -342,4 +342,25 @@ bool fw_cfg_dma_enabled(void *opaque);
  */
 const char *fw_cfg_arch_key_name(uint16_t key);
 
+/**
+ * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
+ *                          by key.
+ * @fw_cfg:         The firmware config instance to store the data in.
+ * @size_key:       The firmware config key to store the size of the loaded
+ *                  data under, with fw_cfg_add_i32().
+ * @data_key:       The firmware config key to store the loaded data under,
+ *                  with fw_cfg_add_bytes().
+ * @image_name:     The name of the image file to load. If it is NULL, the
+ *                  function returns without doing anything.
+ * @try_decompress: Whether the image should be decompressed (gunzipped) before
+ *                  adding it to fw_cfg. If decompression fails, the image is
+ *                  loaded as-is.
+ *
+ * In case of failure, the function prints an error message to stderr and the
+ * process exits with status 1.
+ */
+void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
+                          uint16_t data_key, const char *image_name,
+                          bool try_decompress);
+
 #endif
-- 
2.25.1

[PATCH V4 2/3] hw/riscv: virt: Move create_fw_cfg() prior to loading kernel

[Sunil V L]

To enable both -kernel and -pflash options, the fw_cfg needs to be
created prior to loading the kernel.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 hw/riscv/virt.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ff8c0df5cd..b6bbf03f61 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1251,6 +1251,13 @@ static void virt_machine_done(Notifier *notifier, void *data)
                                     RISCV64_BIOS_BIN, start_addr, NULL);
     }
 
+    /*
+     * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
+     * tree cannot be altered and we get FDT_ERR_NOSPACE.
+     */
+    s->fw_cfg = create_fw_cfg(machine);
+    rom_set_fw(s->fw_cfg);
+
     if (machine->kernel_filename) {
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                          firmware_end_addr);
@@ -1284,13 +1291,6 @@ static void virt_machine_done(Notifier *notifier, void *data)
         start_addr = virt_memmap[VIRT_FLASH].base;
     }
 
-    /*
-     * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
-     * tree cannot be altered and we get FDT_ERR_NOSPACE.
-     */
-    s->fw_cfg = create_fw_cfg(machine);
-    rom_set_fw(s->fw_cfg);
-
     /* Compute the fdt load address in dram */
     fdt_load_addr = riscv_load_fdt(memmap[VIRT_DRAM].base,
                                    machine->ram_size, machine->fdt);
-- 
2.25.1

[PATCH V4 3/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Sunil V L]

To boot S-mode firmware payload like EDK2 from persistent
flash storage, qemu needs to pass the flash address as the
next_addr in fw_dynamic_info to the opensbi.

When both -kernel and -pflash options are provided in command line,
the kernel (and initrd if -initrd) will be copied to fw_cfg table.
The S-mode FW will load the kernel/initrd from fw_cfg table.

If only pflash is given but not -kernel, then it is the job of
of the S-mode firmware to locate and load the kernel.

In either case, update the kernel_entry with the flash address
so that the opensbi can jump to the entry point of the S-mode
firmware.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 hw/riscv/boot.c         | 29 +++++++++++++++++++++++++++++
 hw/riscv/virt.c         | 18 +++++++++++++++++-
 include/hw/riscv/boot.h |  1 +
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 1ae7596873..fa8ad27da2 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -338,3 +338,32 @@ void riscv_setup_direct_kernel(hwaddr kernel_addr, hwaddr fdt_addr)
         riscv_cpu->env.fdt_addr = fdt_addr;
     }
 }
+
+void riscv_setup_firmware_boot(MachineState *machine)
+{
+    if (machine->kernel_filename) {
+        FWCfgState *fw_cfg;
+        fw_cfg = fw_cfg_find();
+
+        assert(fw_cfg);
+        /*
+         * Expose the kernel, the command line, and the initrd in fw_cfg.
+         * We don't process them here at all, it's all left to the
+         * firmware.
+         */
+        load_image_to_fw_cfg(fw_cfg,
+                             FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
+                             machine->kernel_filename,
+                             true);
+        load_image_to_fw_cfg(fw_cfg,
+                             FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
+                             machine->initrd_filename, false);
+
+        if (machine->kernel_cmdline) {
+            fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
+                           strlen(machine->kernel_cmdline) + 1);
+            fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
+                              machine->kernel_cmdline);
+        }
+    }
+}
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index b6bbf03f61..40515a793d 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1258,7 +1258,23 @@ static void virt_machine_done(Notifier *notifier, void *data)
     s->fw_cfg = create_fw_cfg(machine);
     rom_set_fw(s->fw_cfg);
 
-    if (machine->kernel_filename) {
+    if (drive_get(IF_PFLASH, 0, 1)) {
+        /*
+         * S-mode FW like EDK2 will be kept in second plash (unit 1).
+         * When both kernel, initrd and pflash options are provided in the
+         * command line, the kernel and initrd will be copied to the fw_cfg
+         * table and opensbi will jump to the flash address which is the
+         * entry point of S-mode FW. It is the job of the S-mode FW to load
+         * the kernel and initrd using fw_cfg table.
+         *
+         * If only pflash is given but not -kernel, then it is the job of
+         * of the S-mode firmware to locate and load the kernel.
+         * In either case, the next_addr for opensbi will be the flash address.
+         */
+        riscv_setup_firmware_boot(machine);
+        kernel_entry = virt_memmap[VIRT_FLASH].base +
+                       virt_memmap[VIRT_FLASH].size / 2;
+    } else if (machine->kernel_filename) {
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                          firmware_end_addr);
 
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index a36f7618f5..93e5f8760d 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -57,5 +57,6 @@ void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
                                   uint32_t reset_vec_size,
                                   uint64_t kernel_entry);
 void riscv_setup_direct_kernel(hwaddr kernel_addr, hwaddr fdt_addr);
+void riscv_setup_firmware_boot(MachineState *machine);
 
 #endif /* RISCV_BOOT_H */
-- 
2.25.1

Re: [PATCH V4 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Gerd Hoffmann]

  Hi,

> 3)Make the EDK2 image size to match with what qemu flash expects
> truncate -s 32M Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd

Hmm, we have that kind of padding on arm too (64M for code and 64M for
vars) and only a fraction of the space is actually used, which isn't
exactly ideal.  So not sure it is a good plan to repeat that on riscv.

Also: Do you have support for persistent efi variables?  If that is the
case then it makes sense to have separate pflash devices for code and
variable store.  First because you can map the code part read-only then,
and second because decoupling code + vars to separate files allows easy
firmware code updates without loosing the variable store.

take care,
  Gerd

Re: [PATCH V4 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Sunil V L]

Hi Gerd,

On Tue, Sep 06, 2022 at 12:41:28PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > 3)Make the EDK2 image size to match with what qemu flash expects
> > truncate -s 32M Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd
> 
> Hmm, we have that kind of padding on arm too (64M for code and 64M for
> vars) and only a fraction of the space is actually used, which isn't
> exactly ideal.  So not sure it is a good plan to repeat that on riscv.

Yeah.. but it looks like limitation from qemu flash emulation. Do you mean
this limitation exists for arm in general on real flash also?

> 
> Also: Do you have support for persistent efi variables?  If that is the
> case then it makes sense to have separate pflash devices for code and
> variable store.  First because you can map the code part read-only then,
> and second because decoupling code + vars to separate files allows easy
> firmware code updates without loosing the variable store.

Yes, we have persistent variables in my WIP branch. We can easily make it
to create variables as separate file in EDK2. But we will need to
enhance qemu virt machine to create more than 2 flash since the first
one is currently reserved for machine mode firmware. This is a
good input to enhance it in future.

Thanks!
Sunil

> 
> take care,
>   Gerd
> 

Re: [PATCH V4 1/3] hw/arm,loongarch: Move load_image_to_fw_cfg() to common location

[Philippe Mathieu-Daudé via]

On 6/9/22 11:02, Sunil V L wrote:
> load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
> function will be required by riscv too. So, it's time to refactor and
> move this function to a common path.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   hw/arm/boot.c             | 49 ---------------------------------------
>   hw/loongarch/virt.c       | 33 --------------------------
>   hw/nvram/fw_cfg.c         | 32 +++++++++++++++++++++++++
>   include/hw/nvram/fw_cfg.h | 21 +++++++++++++++++
>   4 files changed, 53 insertions(+), 82 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH V4 1/3] hw/arm,loongarch: Move load_image_to_fw_cfg() to common location

[Philippe Mathieu-Daudé]

On 6/9/22 11:02, Sunil V L wrote:
> load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
> function will be required by riscv too. So, it's time to refactor and
> move this function to a common path.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   hw/arm/boot.c             | 49 ---------------------------------------
>   hw/loongarch/virt.c       | 33 --------------------------
>   hw/nvram/fw_cfg.c         | 32 +++++++++++++++++++++++++
>   include/hw/nvram/fw_cfg.h | 21 +++++++++++++++++
>   4 files changed, 53 insertions(+), 82 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH V4 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Gerd Hoffmann]

On Tue, Sep 06, 2022 at 06:02:00PM +0530, Sunil V L wrote:
> Hi Gerd,
> 
> On Tue, Sep 06, 2022 at 12:41:28PM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > 3)Make the EDK2 image size to match with what qemu flash expects
> > > truncate -s 32M Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd
> > 
> > Hmm, we have that kind of padding on arm too (64M for code and 64M for
> > vars) and only a fraction of the space is actually used, which isn't
> > exactly ideal.  So not sure it is a good plan to repeat that on riscv.
> 
> Yeah.. but it looks like limitation from qemu flash emulation. Do you mean
> this limitation exists for arm in general on real flash also?

Well, at least on x86 flash devices can have odd sizes.  I don't think
the qemu pflash emulation dictates anything here.

I think the underlying problem we actually have in qemu is that the
flash size indirectly dictates the memory layout.  We pack the flash
devices next to each other, on x86 downwards from 4G, on arm upwards
from zero, not sure what risc-v is dong here.

edk2 arm code expects the variable store being mapped at 64m.  So
QEMU_EFI.fd (which is actually 2M in size) gets padded to 64m, which
has the desired effect that the next flash device (the varstore) is
mapped at 64m.  But also has the side effect that we map 62m of zeros
into the guest address space ...

The vars file is padded to 64m for consistency with the code.  Not
padding the vars file should have no bad side effects I think, except
for live migration where the flash size change might cause
compatibility problems.

Not padding the code file needs some alternative way to specify the
memory layout, to make sure the vars flash continues to be mapped at
64m even when the code flash is smaller.  Cc'ed Pawel who investigates
that right now.

One possible option is to just hard-code the flash memory layout per
machine type or per architecture.  Another option would be to add
some way to configure that on the command line.

take care,
  Gerd

Re: [PATCH V4 1/3] hw/arm, loongarch: Move load_image_to_fw_cfg() to common location

[Alistair Francis]

On Tue, Sep 6, 2022 at 11:38 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
> function will be required by riscv too. So, it's time to refactor and
> move this function to a common path.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/boot.c             | 49 ---------------------------------------
>  hw/loongarch/virt.c       | 33 --------------------------
>  hw/nvram/fw_cfg.c         | 32 +++++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h | 21 +++++++++++++++++
>  4 files changed, 53 insertions(+), 82 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index ada2717f76..704f368d9c 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -818,55 +818,6 @@ static void do_cpu_reset(void *opaque)
>      }
>  }
>
> -/**
> - * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
> - *                          by key.
> - * @fw_cfg:         The firmware config instance to store the data in.
> - * @size_key:       The firmware config key to store the size of the loaded
> - *                  data under, with fw_cfg_add_i32().
> - * @data_key:       The firmware config key to store the loaded data under,
> - *                  with fw_cfg_add_bytes().
> - * @image_name:     The name of the image file to load. If it is NULL, the
> - *                  function returns without doing anything.
> - * @try_decompress: Whether the image should be decompressed (gunzipped) before
> - *                  adding it to fw_cfg. If decompression fails, the image is
> - *                  loaded as-is.
> - *
> - * In case of failure, the function prints an error message to stderr and the
> - * process exits with status 1.
> - */
> -static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
> -                                 uint16_t data_key, const char *image_name,
> -                                 bool try_decompress)
> -{
> -    size_t size = -1;
> -    uint8_t *data;
> -
> -    if (image_name == NULL) {
> -        return;
> -    }
> -
> -    if (try_decompress) {
> -        size = load_image_gzipped_buffer(image_name,
> -                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
> -    }
> -
> -    if (size == (size_t)-1) {
> -        gchar *contents;
> -        gsize length;
> -
> -        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
> -            error_report("failed to load \"%s\"", image_name);
> -            exit(1);
> -        }
> -        size = length;
> -        data = (uint8_t *)contents;
> -    }
> -
> -    fw_cfg_add_i32(fw_cfg, size_key, size);
> -    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
> -}
> -
>  static int do_arm_linux_init(Object *obj, void *opaque)
>  {
>      if (object_dynamic_cast(obj, TYPE_ARM_LINUX_BOOT_IF)) {
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index 5cc0b05538..eee2c193c0 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -542,39 +542,6 @@ static void reset_load_elf(void *opaque)
>      }
>  }
>
> -/* Load an image file into an fw_cfg entry identified by key. */
> -static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
> -                                 uint16_t data_key, const char *image_name,
> -                                 bool try_decompress)
> -{
> -    size_t size = -1;
> -    uint8_t *data;
> -
> -    if (image_name == NULL) {
> -        return;
> -    }
> -
> -    if (try_decompress) {
> -        size = load_image_gzipped_buffer(image_name,
> -                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
> -    }
> -
> -    if (size == (size_t)-1) {
> -        gchar *contents;
> -        gsize length;
> -
> -        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
> -            error_report("failed to load \"%s\"", image_name);
> -            exit(1);
> -        }
> -        size = length;
> -        data = (uint8_t *)contents;
> -    }
> -
> -    fw_cfg_add_i32(fw_cfg, size_key, size);
> -    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
> -}
> -
>  static void fw_cfg_add_kernel_info(FWCfgState *fw_cfg)
>  {
>      /*
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index d605f3f45a..371a45dfe2 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -41,6 +41,7 @@
>  #include "qapi/error.h"
>  #include "hw/acpi/aml-build.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/loader.h"
>
>  #define FW_CFG_FILE_SLOTS_DFLT 0x20
>
> @@ -1221,6 +1222,37 @@ FWCfgState *fw_cfg_find(void)
>      return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
>  }
>
> +void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
> +                          uint16_t data_key, const char *image_name,
> +                          bool try_decompress)
> +{
> +    size_t size = -1;
> +    uint8_t *data;
> +
> +    if (image_name == NULL) {
> +        return;
> +    }
> +
> +    if (try_decompress) {
> +        size = load_image_gzipped_buffer(image_name,
> +                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
> +    }
> +
> +    if (size == (size_t)-1) {
> +        gchar *contents;
> +        gsize length;
> +
> +        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
> +            error_report("failed to load \"%s\"", image_name);
> +            exit(1);
> +        }
> +        size = length;
> +        data = (uint8_t *)contents;
> +    }
> +
> +    fw_cfg_add_i32(fw_cfg, size_key, size);
> +    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
> +}
>
>  static void fw_cfg_class_init(ObjectClass *klass, void *data)
>  {
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 0e7a8bc7af..c1f81a5f13 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -342,4 +342,25 @@ bool fw_cfg_dma_enabled(void *opaque);
>   */
>  const char *fw_cfg_arch_key_name(uint16_t key);
>
> +/**
> + * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
> + *                          by key.
> + * @fw_cfg:         The firmware config instance to store the data in.
> + * @size_key:       The firmware config key to store the size of the loaded
> + *                  data under, with fw_cfg_add_i32().
> + * @data_key:       The firmware config key to store the loaded data under,
> + *                  with fw_cfg_add_bytes().
> + * @image_name:     The name of the image file to load. If it is NULL, the
> + *                  function returns without doing anything.
> + * @try_decompress: Whether the image should be decompressed (gunzipped) before
> + *                  adding it to fw_cfg. If decompression fails, the image is
> + *                  loaded as-is.
> + *
> + * In case of failure, the function prints an error message to stderr and the
> + * process exits with status 1.
> + */
> +void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
> +                          uint16_t data_key, const char *image_name,
> +                          bool try_decompress);
> +
>  #endif
> --
> 2.25.1
>
>

Re: [PATCH V4 2/3] hw/riscv: virt: Move create_fw_cfg() prior to loading kernel

[Alistair Francis]

On Tue, Sep 6, 2022 at 11:44 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> To enable both -kernel and -pflash options, the fw_cfg needs to be
> created prior to loading the kernel.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index ff8c0df5cd..b6bbf03f61 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1251,6 +1251,13 @@ static void virt_machine_done(Notifier *notifier, void *data)
>                                      RISCV64_BIOS_BIN, start_addr, NULL);
>      }
>
> +    /*
> +     * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
> +     * tree cannot be altered and we get FDT_ERR_NOSPACE.
> +     */
> +    s->fw_cfg = create_fw_cfg(machine);
> +    rom_set_fw(s->fw_cfg);
> +
>      if (machine->kernel_filename) {
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
>                                                           firmware_end_addr);
> @@ -1284,13 +1291,6 @@ static void virt_machine_done(Notifier *notifier, void *data)
>          start_addr = virt_memmap[VIRT_FLASH].base;
>      }
>
> -    /*
> -     * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
> -     * tree cannot be altered and we get FDT_ERR_NOSPACE.
> -     */
> -    s->fw_cfg = create_fw_cfg(machine);
> -    rom_set_fw(s->fw_cfg);
> -
>      /* Compute the fdt load address in dram */
>      fdt_load_addr = riscv_load_fdt(memmap[VIRT_DRAM].base,
>                                     machine->ram_size, machine->fdt);
> --
> 2.25.1
>
>

Re: [PATCH V4 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflashy

[Sunil V L]

On Wed, Sep 07, 2022 at 09:10:37AM +0200, Gerd Hoffmann wrote:
> On Tue, Sep 06, 2022 at 06:02:00PM +0530, Sunil V L wrote:
> > Hi Gerd,
> > 
> > On Tue, Sep 06, 2022 at 12:41:28PM +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > 3)Make the EDK2 image size to match with what qemu flash expects
> > > > truncate -s 32M Build/RiscVVirt/DEBUG_GCC5/FV/RISCV_VIRT.fd
> > > 
> > > Hmm, we have that kind of padding on arm too (64M for code and 64M for
> > > vars) and only a fraction of the space is actually used, which isn't
> > > exactly ideal.  So not sure it is a good plan to repeat that on riscv.
> > 
> > Yeah.. but it looks like limitation from qemu flash emulation. Do you mean
> > this limitation exists for arm in general on real flash also?
> 
> Well, at least on x86 flash devices can have odd sizes.  I don't think
> the qemu pflash emulation dictates anything here.
> 
> I think the underlying problem we actually have in qemu is that the
> flash size indirectly dictates the memory layout.  We pack the flash
> devices next to each other, on x86 downwards from 4G, on arm upwards
> from zero, not sure what risc-v is dong here.
> 
> edk2 arm code expects the variable store being mapped at 64m.  So
> QEMU_EFI.fd (which is actually 2M in size) gets padded to 64m, which
> has the desired effect that the next flash device (the varstore) is
> mapped at 64m.  But also has the side effect that we map 62m of zeros
> into the guest address space ...
> 
> The vars file is padded to 64m for consistency with the code.  Not
> padding the vars file should have no bad side effects I think, except
> for live migration where the flash size change might cause
> compatibility problems.
> 
> Not padding the code file needs some alternative way to specify the
> memory layout, to make sure the vars flash continues to be mapped at
> 64m even when the code flash is smaller.  Cc'ed Pawel who investigates
> that right now.
> 
> One possible option is to just hard-code the flash memory layout per
> machine type or per architecture.  Another option would be to add
> some way to configure that on the command line.

Thanks Gerd. One question: Is it possible to have separate code + vars
even when there is TF-A? My understanding is, TF-A will take one drive
and will be hidden from the non-secure word. So, there is only one flash
left for edk2. Is that correct?

In RISC-V, I think we have the this situation always since M-mode is
mandatory. The first flash is always reserved for secure fw. So, we will
have to increase the number of flash supported to 3 to support edk2 use
case.

I have a fix RISC-V which resolves truncate issue leveraging logic from
x86. It also creates 2 flash drives within non-secure space.
EDK2 also needs to be modified to work with smaller code flash. But
because the patch takes care of the actual size, it allows to have
bigger code and smaller var images.
Same thing can be adopted to arm also since both seem to follow the same
logic. But I think that will be a separate patch than this series. I
will run that as a separate RFC patch. Is that fine?

Thanks!
Sunil
> 
> take care,
>   Gerd
> 

Re: [PATCH V4 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflash

[Sunil V L]

> Same thing can be adopted to arm also since both seem to follow the same
> logic. But I think that will be a separate patch than this series. I
> will run that as a separate RFC patch. Is that fine?

Just to be clear, I meant RISC-V fix as separate RFC patch. Not the ARM.

Thanks
Sunil

Re: [PATCH V4 0/3] hw/riscv: virt: Enable booting S-mode firmware from pflashy

[Gerd Hoffmann]

  Hi,

> Thanks Gerd. One question: Is it possible to have separate code + vars
> even when there is TF-A? My understanding is, TF-A will take one drive
> and will be hidden from the non-secure word. So, there is only one flash
> left for edk2. Is that correct?

Yes.

> In RISC-V, I think we have the this situation always since M-mode is
> mandatory. The first flash is always reserved for secure fw. So, we will
> have to increase the number of flash supported to 3 to support edk2 use
> case.

Well.  Adding one more flash is certainly the easiest approach.  Another
possible option would be to have the secure world store the efi variables.
That might be needed anyway for secure boot support.

> I have a fix RISC-V which resolves truncate issue leveraging logic from
> x86. It also creates 2 flash drives within non-secure space.
> EDK2 also needs to be modified to work with smaller code flash. But
> because the patch takes care of the actual size, it allows to have
> bigger code and smaller var images.

The size of the code flash and thereby the address of the varstore is
known at compile time too, so yes, that approach should work fine.

Note that changing the flash size can lead to compatibility problems
(for example when live migrating), so I'd suggest to be generous with
code/vars sizes.  On x64 the upstream default flash size is 2M and when
all compile-time options are enabled things don't fit any more.

So, assuming size figures are roughly the same for risc-v, I'd suggest
to go for 4M or 8M code flash and 1M vars flash.

> Same thing can be adopted to arm also since both seem to follow the same
> logic.

Yes.  The tricky part here is dealing with backward compatibility and
the transition from padded to non-padded images.  I suspect at the end
of the day keeping the vars flash mapping fixed at 64M (and have a hole
between code and vars) will be easier.

> But I think that will be a separate patch than this series. I
> will run that as a separate RFC patch. Is that fine?

No objections.

take care,
  Gerd

Re: [PATCH V4 1/3] hw/arm, loongarch: Move load_image_to_fw_cfg() to common location

[Sunil V L]

Hi,

Could maintainers of hw/arm and hw/loongarch to take a look at this? I
have addressed Peter's earlier comment.

Thanks
Sunil

On Thu, Sep 08, 2022 at 11:17:52AM +0200, Alistair Francis wrote:
> On Tue, Sep 6, 2022 at 11:38 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
> > function will be required by riscv too. So, it's time to refactor and
> > move this function to a common path.
> >
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> 
> Alistair
> 
> > ---
> >  hw/arm/boot.c             | 49 ---------------------------------------
> >  hw/loongarch/virt.c       | 33 --------------------------
> >  hw/nvram/fw_cfg.c         | 32 +++++++++++++++++++++++++
> >  include/hw/nvram/fw_cfg.h | 21 +++++++++++++++++
> >  4 files changed, 53 insertions(+), 82 deletions(-)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index ada2717f76..704f368d9c 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -818,55 +818,6 @@ static void do_cpu_reset(void *opaque)
> >      }
> >  }
> >
> > -/**
> > - * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
> > - *                          by key.
> > - * @fw_cfg:         The firmware config instance to store the data in.
> > - * @size_key:       The firmware config key to store the size of the loaded
> > - *                  data under, with fw_cfg_add_i32().
> > - * @data_key:       The firmware config key to store the loaded data under,
> > - *                  with fw_cfg_add_bytes().
> > - * @image_name:     The name of the image file to load. If it is NULL, the
> > - *                  function returns without doing anything.
> > - * @try_decompress: Whether the image should be decompressed (gunzipped) before
> > - *                  adding it to fw_cfg. If decompression fails, the image is
> > - *                  loaded as-is.
> > - *
> > - * In case of failure, the function prints an error message to stderr and the
> > - * process exits with status 1.
> > - */
> > -static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
> > -                                 uint16_t data_key, const char *image_name,
> > -                                 bool try_decompress)
> > -{
> > -    size_t size = -1;
> > -    uint8_t *data;
> > -
> > -    if (image_name == NULL) {
> > -        return;
> > -    }
> > -
> > -    if (try_decompress) {
> > -        size = load_image_gzipped_buffer(image_name,
> > -                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
> > -    }
> > -
> > -    if (size == (size_t)-1) {
> > -        gchar *contents;
> > -        gsize length;
> > -
> > -        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
> > -            error_report("failed to load \"%s\"", image_name);
> > -            exit(1);
> > -        }
> > -        size = length;
> > -        data = (uint8_t *)contents;
> > -    }
> > -
> > -    fw_cfg_add_i32(fw_cfg, size_key, size);
> > -    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
> > -}
> > -
> >  static int do_arm_linux_init(Object *obj, void *opaque)
> >  {
> >      if (object_dynamic_cast(obj, TYPE_ARM_LINUX_BOOT_IF)) {
> > diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> > index 5cc0b05538..eee2c193c0 100644
> > --- a/hw/loongarch/virt.c
> > +++ b/hw/loongarch/virt.c
> > @@ -542,39 +542,6 @@ static void reset_load_elf(void *opaque)
> >      }
> >  }
> >
> > -/* Load an image file into an fw_cfg entry identified by key. */
> > -static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
> > -                                 uint16_t data_key, const char *image_name,
> > -                                 bool try_decompress)
> > -{
> > -    size_t size = -1;
> > -    uint8_t *data;
> > -
> > -    if (image_name == NULL) {
> > -        return;
> > -    }
> > -
> > -    if (try_decompress) {
> > -        size = load_image_gzipped_buffer(image_name,
> > -                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
> > -    }
> > -
> > -    if (size == (size_t)-1) {
> > -        gchar *contents;
> > -        gsize length;
> > -
> > -        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
> > -            error_report("failed to load \"%s\"", image_name);
> > -            exit(1);
> > -        }
> > -        size = length;
> > -        data = (uint8_t *)contents;
> > -    }
> > -
> > -    fw_cfg_add_i32(fw_cfg, size_key, size);
> > -    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
> > -}
> > -
> >  static void fw_cfg_add_kernel_info(FWCfgState *fw_cfg)
> >  {
> >      /*
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index d605f3f45a..371a45dfe2 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -41,6 +41,7 @@
> >  #include "qapi/error.h"
> >  #include "hw/acpi/aml-build.h"
> >  #include "hw/pci/pci_bus.h"
> > +#include "hw/loader.h"
> >
> >  #define FW_CFG_FILE_SLOTS_DFLT 0x20
> >
> > @@ -1221,6 +1222,37 @@ FWCfgState *fw_cfg_find(void)
> >      return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
> >  }
> >
> > +void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
> > +                          uint16_t data_key, const char *image_name,
> > +                          bool try_decompress)
> > +{
> > +    size_t size = -1;
> > +    uint8_t *data;
> > +
> > +    if (image_name == NULL) {
> > +        return;
> > +    }
> > +
> > +    if (try_decompress) {
> > +        size = load_image_gzipped_buffer(image_name,
> > +                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
> > +    }
> > +
> > +    if (size == (size_t)-1) {
> > +        gchar *contents;
> > +        gsize length;
> > +
> > +        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
> > +            error_report("failed to load \"%s\"", image_name);
> > +            exit(1);
> > +        }
> > +        size = length;
> > +        data = (uint8_t *)contents;
> > +    }
> > +
> > +    fw_cfg_add_i32(fw_cfg, size_key, size);
> > +    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
> > +}
> >
> >  static void fw_cfg_class_init(ObjectClass *klass, void *data)
> >  {
> > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> > index 0e7a8bc7af..c1f81a5f13 100644
> > --- a/include/hw/nvram/fw_cfg.h
> > +++ b/include/hw/nvram/fw_cfg.h
> > @@ -342,4 +342,25 @@ bool fw_cfg_dma_enabled(void *opaque);
> >   */
> >  const char *fw_cfg_arch_key_name(uint16_t key);
> >
> > +/**
> > + * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
> > + *                          by key.
> > + * @fw_cfg:         The firmware config instance to store the data in.
> > + * @size_key:       The firmware config key to store the size of the loaded
> > + *                  data under, with fw_cfg_add_i32().
> > + * @data_key:       The firmware config key to store the loaded data under,
> > + *                  with fw_cfg_add_bytes().
> > + * @image_name:     The name of the image file to load. If it is NULL, the
> > + *                  function returns without doing anything.
> > + * @try_decompress: Whether the image should be decompressed (gunzipped) before
> > + *                  adding it to fw_cfg. If decompression fails, the image is
> > + *                  loaded as-is.
> > + *
> > + * In case of failure, the function prints an error message to stderr and the
> > + * process exits with status 1.
> > + */
> > +void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
> > +                          uint16_t data_key, const char *image_name,
> > +                          bool try_decompress);
> > +
> >  #endif
> > --
> > 2.25.1
> >
> >

Re: [PATCH V4 1/3] hw/arm, loongarch: Move load_image_to_fw_cfg() to common location

[gaosong]

在 2022/9/15 下午7:59, Sunil V L 写道:
> Hi,
>
> Could maintainers of hw/arm and hw/loongarch to take a look at this? I
> have addressed Peter's earlier comment.
>
> Thanks
> Sunil
>
> On Thu, Sep 08, 2022 at 11:17:52AM +0200, Alistair Francis wrote:
>> On Tue, Sep 6, 2022 at 11:38 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
>>> load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
>>> function will be required by riscv too. So, it's time to refactor and
>>> move this function to a common path.
>>>
>>> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>
>> Alistair
Reviewed-by: Song Gao <gaosong@loongson.cn>

Thanks
Song Gao
>>> ---
>>>   hw/arm/boot.c             | 49 ---------------------------------------
>>>   hw/loongarch/virt.c       | 33 --------------------------
>>>   hw/nvram/fw_cfg.c         | 32 +++++++++++++++++++++++++
>>>   include/hw/nvram/fw_cfg.h | 21 +++++++++++++++++
>>>   4 files changed, 53 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>> index ada2717f76..704f368d9c 100644
>>> --- a/hw/arm/boot.c
>>> +++ b/hw/arm/boot.c
>>> @@ -818,55 +818,6 @@ static void do_cpu_reset(void *opaque)
>>>       }
>>>   }
>>>
>>> -/**
>>> - * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
>>> - *                          by key.
>>> - * @fw_cfg:         The firmware config instance to store the data in.
>>> - * @size_key:       The firmware config key to store the size of the loaded
>>> - *                  data under, with fw_cfg_add_i32().
>>> - * @data_key:       The firmware config key to store the loaded data under,
>>> - *                  with fw_cfg_add_bytes().
>>> - * @image_name:     The name of the image file to load. If it is NULL, the
>>> - *                  function returns without doing anything.
>>> - * @try_decompress: Whether the image should be decompressed (gunzipped) before
>>> - *                  adding it to fw_cfg. If decompression fails, the image is
>>> - *                  loaded as-is.
>>> - *
>>> - * In case of failure, the function prints an error message to stderr and the
>>> - * process exits with status 1.
>>> - */
>>> -static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>>> -                                 uint16_t data_key, const char *image_name,
>>> -                                 bool try_decompress)
>>> -{
>>> -    size_t size = -1;
>>> -    uint8_t *data;
>>> -
>>> -    if (image_name == NULL) {
>>> -        return;
>>> -    }
>>> -
>>> -    if (try_decompress) {
>>> -        size = load_image_gzipped_buffer(image_name,
>>> -                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
>>> -    }
>>> -
>>> -    if (size == (size_t)-1) {
>>> -        gchar *contents;
>>> -        gsize length;
>>> -
>>> -        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
>>> -            error_report("failed to load \"%s\"", image_name);
>>> -            exit(1);
>>> -        }
>>> -        size = length;
>>> -        data = (uint8_t *)contents;
>>> -    }
>>> -
>>> -    fw_cfg_add_i32(fw_cfg, size_key, size);
>>> -    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
>>> -}
>>> -
>>>   static int do_arm_linux_init(Object *obj, void *opaque)
>>>   {
>>>       if (object_dynamic_cast(obj, TYPE_ARM_LINUX_BOOT_IF)) {
>>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>>> index 5cc0b05538..eee2c193c0 100644
>>> --- a/hw/loongarch/virt.c
>>> +++ b/hw/loongarch/virt.c
>>> @@ -542,39 +542,6 @@ static void reset_load_elf(void *opaque)
>>>       }
>>>   }
>>>
>>> -/* Load an image file into an fw_cfg entry identified by key. */
>>> -static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>>> -                                 uint16_t data_key, const char *image_name,
>>> -                                 bool try_decompress)
>>> -{
>>> -    size_t size = -1;
>>> -    uint8_t *data;
>>> -
>>> -    if (image_name == NULL) {
>>> -        return;
>>> -    }
>>> -
>>> -    if (try_decompress) {
>>> -        size = load_image_gzipped_buffer(image_name,
>>> -                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
>>> -    }
>>> -
>>> -    if (size == (size_t)-1) {
>>> -        gchar *contents;
>>> -        gsize length;
>>> -
>>> -        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
>>> -            error_report("failed to load \"%s\"", image_name);
>>> -            exit(1);
>>> -        }
>>> -        size = length;
>>> -        data = (uint8_t *)contents;
>>> -    }
>>> -
>>> -    fw_cfg_add_i32(fw_cfg, size_key, size);
>>> -    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
>>> -}
>>> -
>>>   static void fw_cfg_add_kernel_info(FWCfgState *fw_cfg)
>>>   {
>>>       /*
>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>> index d605f3f45a..371a45dfe2 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -41,6 +41,7 @@
>>>   #include "qapi/error.h"
>>>   #include "hw/acpi/aml-build.h"
>>>   #include "hw/pci/pci_bus.h"
>>> +#include "hw/loader.h"
>>>
>>>   #define FW_CFG_FILE_SLOTS_DFLT 0x20
>>>
>>> @@ -1221,6 +1222,37 @@ FWCfgState *fw_cfg_find(void)
>>>       return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
>>>   }
>>>
>>> +void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>>> +                          uint16_t data_key, const char *image_name,
>>> +                          bool try_decompress)
>>> +{
>>> +    size_t size = -1;
>>> +    uint8_t *data;
>>> +
>>> +    if (image_name == NULL) {
>>> +        return;
>>> +    }
>>> +
>>> +    if (try_decompress) {
>>> +        size = load_image_gzipped_buffer(image_name,
>>> +                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
>>> +    }
>>> +
>>> +    if (size == (size_t)-1) {
>>> +        gchar *contents;
>>> +        gsize length;
>>> +
>>> +        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
>>> +            error_report("failed to load \"%s\"", image_name);
>>> +            exit(1);
>>> +        }
>>> +        size = length;
>>> +        data = (uint8_t *)contents;
>>> +    }
>>> +
>>> +    fw_cfg_add_i32(fw_cfg, size_key, size);
>>> +    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
>>> +}
>>>
>>>   static void fw_cfg_class_init(ObjectClass *klass, void *data)
>>>   {
>>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>>> index 0e7a8bc7af..c1f81a5f13 100644
>>> --- a/include/hw/nvram/fw_cfg.h
>>> +++ b/include/hw/nvram/fw_cfg.h
>>> @@ -342,4 +342,25 @@ bool fw_cfg_dma_enabled(void *opaque);
>>>    */
>>>   const char *fw_cfg_arch_key_name(uint16_t key);
>>>
>>> +/**
>>> + * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
>>> + *                          by key.
>>> + * @fw_cfg:         The firmware config instance to store the data in.
>>> + * @size_key:       The firmware config key to store the size of the loaded
>>> + *                  data under, with fw_cfg_add_i32().
>>> + * @data_key:       The firmware config key to store the loaded data under,
>>> + *                  with fw_cfg_add_bytes().
>>> + * @image_name:     The name of the image file to load. If it is NULL, the
>>> + *                  function returns without doing anything.
>>> + * @try_decompress: Whether the image should be decompressed (gunzipped) before
>>> + *                  adding it to fw_cfg. If decompression fails, the image is
>>> + *                  loaded as-is.
>>> + *
>>> + * In case of failure, the function prints an error message to stderr and the
>>> + * process exits with status 1.
>>> + */
>>> +void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>>> +                          uint16_t data_key, const char *image_name,
>>> +                          bool try_decompress);
>>> +
>>>   #endif
>>> --
>>> 2.25.1
>>>
>>>

Re: [PATCH V4 1/3] hw/arm, loongarch: Move load_image_to_fw_cfg() to common location

[Sunil V L]

On Thu, Sep 15, 2022 at 08:56:01PM +0800, gaosong wrote:
> 
> 在 2022/9/15 下午7:59, Sunil V L 写道:
> > Hi,
> > 
> > Could maintainers of hw/arm and hw/loongarch to take a look at this? I
> > have addressed Peter's earlier comment.
> > 
> > Thanks
> > Sunil
> > 
> > On Thu, Sep 08, 2022 at 11:17:52AM +0200, Alistair Francis wrote:
> > > On Tue, Sep 6, 2022 at 11:38 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
> > > > load_image_to_fw_cfg() is duplicated by both arm and loongarch. The same
> > > > function will be required by riscv too. So, it's time to refactor and
> > > > move this function to a common path.
> > > > 
> > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > 
> > > Alistair
> Reviewed-by: Song Gao <gaosong@loongson.cn>
> 
> Thanks
> Song Gao
Thanks Song Gao.

Hi Peter,

Could you please provide RB or ACK if things are fine with this patch?

Thanks
Sunil