[PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"

Sunil V L posted 1 patch 11 months, 2 weeks ago
Failed in applying to current master (apply log)
docs/system/riscv/virt.rst | 33 ++++++++++++++++++++++++
hw/riscv/virt.c            | 51 ++++++++++++++------------------------
2 files changed, 52 insertions(+), 32 deletions(-)
[PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Posted by Sunil V L 11 months, 2 weeks ago
Currently, virt machine supports two pflash instances each with
32MB size. However, the first pflash is always assumed to
contain M-mode firmware and reset vector is set to this if
enabled. Hence, for S-mode payloads like EDK2, only one pflash
instance is available for use. This means both code and NV variables
of EDK2 will need to use the same pflash.

The OS distros keep the EDK2 FW code as readonly. When non-volatile
variables also need to share the same pflash, it is not possible
to keep it as readonly since variables need write access.

To resolve this issue, the code and NV variables need to be separated.
But in that case we need an extra flash. Hence, modify the convention
such that pflash0 will contain the M-mode FW only when "-bios none"
option is used. Otherwise, pflash0 will contain the S-mode payload FW.
This enables both pflash instances available for EDK2 use.

Example usage:
1) pflash0 containing M-mode FW
qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt
or
qemu-system-riscv64 -bios none \
-drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt

2) pflash0 containing S-mode payload like EDK2
qemu-system-riscv64 -pflash <smode_fw_code> -pflash <smode_vars> -machine  virt
or
qemu-system-riscv64 -bios <opensbi_fw> \
-pflash <smode_fw_code> \
-pflash <smode_vars> \
-machine  virt
or
qemu-system-riscv64 -bios <opensbi_fw> \
-drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
-drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
-machine virt

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

The issue is reported at
https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6

The patch is based on Alistair's riscv-to-apply.next branch.

Changes since v2:
	1) Reverted v2 changes and used v1 approach so that pflash0 can be used
	   for code and pflash1 for variable store.
	2) Rebased to latest riscv-to-apply.next branch.
	3) Added documentation for pflash usage.

Changes since v1:
	1) Simplified the fix such that it doesn't break current EDK2.

 docs/system/riscv/virt.rst | 33 ++++++++++++++++++++++++
 hw/riscv/virt.c            | 51 ++++++++++++++------------------------
 2 files changed, 52 insertions(+), 32 deletions(-)

diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
index 4b16e41d7f..4ac0d38fdf 100644
--- a/docs/system/riscv/virt.rst
+++ b/docs/system/riscv/virt.rst
@@ -53,6 +53,39 @@ with the default OpenSBI firmware image as the -bios. It also supports
 the recommended RISC-V bootflow: U-Boot SPL (M-mode) loads OpenSBI fw_dynamic
 firmware and U-Boot proper (S-mode), using the standard -bios functionality.
 
+Using flash devices
+-------------------
+
+The first flash device (pflash0) can contain either ROM code like Oreboot
+or S-mode payload firmware code like EDK2. If the pflash0 contains the
+ROM code, -bios should be set to none. Otherwise, pflash0 is assumed to
+contain S-mode payload code.
+
+Firmware images used for pflash should be of size 32M.
+
+To boot as ROM code like Oreboot:
+
+.. code-block:: bash
+
+  $ qemu-system-riscv64 -bios none -pflash <rom_code_image> \
+       ... other args ....
+
+To boot as S-mode payload like EDK2:
+
+.. code-block:: bash
+
+  $ qemu-system-riscv64 -pflash <s-mode_fw_code> -pflash <smode_fw_vars> \
+       ... other args ....
+
+To boot as read-only S-mode payload like EDK2:
+
+.. code-block:: bash
+
+  $ qemu-system-riscv64 -bios <opensbi_fw> \
+       -drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
+       -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
+       ... other args ....
+
 Machine-specific options
 ------------------------
 
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4e3efbee16..1187a60d6e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1245,7 +1245,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
     target_ulong firmware_end_addr, kernel_start_addr;
     const char *firmware_name = riscv_default_firmware_name(&s->soc[0]);
     uint32_t fdt_load_addr;
-    uint64_t kernel_entry;
+    uint64_t kernel_entry = 0;
 
     /*
      * Only direct boot kernel is currently supported for KVM VM,
@@ -1266,42 +1266,29 @@ static void virt_machine_done(Notifier *notifier, void *data)
     firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
                                                      start_addr, NULL);
 
-    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) {
+    if (drive_get(IF_PFLASH, 0, 0)) {
+        if (machine->firmware && !strcmp(machine->firmware, "none")) {
+            /*
+             * Pflash was supplied but bios is none, let's overwrite the
+             * address we jump to after reset to the base of the flash.
+             */
+            start_addr = virt_memmap[VIRT_FLASH].base;
+        } else {
+            /*
+             * Pflash was supplied but bios is not none. In this case,
+             * base of the flash would contain S-mode payload.
+             */
+            riscv_setup_firmware_boot(machine);
+            kernel_entry = virt_memmap[VIRT_FLASH].base;
+        }
+    }
+
+    if (machine->kernel_filename && !kernel_entry) {
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                          firmware_end_addr);
 
         kernel_entry = riscv_load_kernel(machine, &s->soc[0],
                                          kernel_start_addr, true, NULL);
-    } else {
-       /*
-        * If dynamic firmware is used, it doesn't know where is the next mode
-        * if kernel argument is not set.
-        */
-        kernel_entry = 0;
-    }
-
-    if (drive_get(IF_PFLASH, 0, 0)) {
-        /*
-         * Pflash was supplied, let's overwrite the address we jump to after
-         * reset to the base of the flash.
-         */
-        start_addr = virt_memmap[VIRT_FLASH].base;
     }
 
     fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
-- 
2.34.1
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Posted by Alistair Francis 11 months, 2 weeks ago
On Tue, May 23, 2023 at 8:29 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> Currently, virt machine supports two pflash instances each with
> 32MB size. However, the first pflash is always assumed to
> contain M-mode firmware and reset vector is set to this if
> enabled. Hence, for S-mode payloads like EDK2, only one pflash
> instance is available for use. This means both code and NV variables
> of EDK2 will need to use the same pflash.
>
> The OS distros keep the EDK2 FW code as readonly. When non-volatile
> variables also need to share the same pflash, it is not possible
> to keep it as readonly since variables need write access.
>
> To resolve this issue, the code and NV variables need to be separated.
> But in that case we need an extra flash. Hence, modify the convention
> such that pflash0 will contain the M-mode FW only when "-bios none"
> option is used. Otherwise, pflash0 will contain the S-mode payload FW.
> This enables both pflash instances available for EDK2 use.
>
> Example usage:
> 1) pflash0 containing M-mode FW
> qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt
> or
> qemu-system-riscv64 -bios none \
> -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt
>
> 2) pflash0 containing S-mode payload like EDK2
> qemu-system-riscv64 -pflash <smode_fw_code> -pflash <smode_vars> -machine  virt
> or
> qemu-system-riscv64 -bios <opensbi_fw> \
> -pflash <smode_fw_code> \
> -pflash <smode_vars> \
> -machine  virt
> or
> qemu-system-riscv64 -bios <opensbi_fw> \
> -drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
> -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
> -machine virt
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

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

Alistair

> ---
>
> The issue is reported at
> https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6
>
> The patch is based on Alistair's riscv-to-apply.next branch.
>
> Changes since v2:
>         1) Reverted v2 changes and used v1 approach so that pflash0 can be used
>            for code and pflash1 for variable store.
>         2) Rebased to latest riscv-to-apply.next branch.
>         3) Added documentation for pflash usage.
>
> Changes since v1:
>         1) Simplified the fix such that it doesn't break current EDK2.
>
>  docs/system/riscv/virt.rst | 33 ++++++++++++++++++++++++
>  hw/riscv/virt.c            | 51 ++++++++++++++------------------------
>  2 files changed, 52 insertions(+), 32 deletions(-)
>
> diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
> index 4b16e41d7f..4ac0d38fdf 100644
> --- a/docs/system/riscv/virt.rst
> +++ b/docs/system/riscv/virt.rst
> @@ -53,6 +53,39 @@ with the default OpenSBI firmware image as the -bios. It also supports
>  the recommended RISC-V bootflow: U-Boot SPL (M-mode) loads OpenSBI fw_dynamic
>  firmware and U-Boot proper (S-mode), using the standard -bios functionality.
>
> +Using flash devices
> +-------------------
> +
> +The first flash device (pflash0) can contain either ROM code like Oreboot
> +or S-mode payload firmware code like EDK2. If the pflash0 contains the
> +ROM code, -bios should be set to none. Otherwise, pflash0 is assumed to
> +contain S-mode payload code.
> +
> +Firmware images used for pflash should be of size 32M.
> +
> +To boot as ROM code like Oreboot:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 -bios none -pflash <rom_code_image> \
> +       ... other args ....
> +
> +To boot as S-mode payload like EDK2:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 -pflash <s-mode_fw_code> -pflash <smode_fw_vars> \
> +       ... other args ....
> +
> +To boot as read-only S-mode payload like EDK2:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 -bios <opensbi_fw> \
> +       -drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
> +       -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
> +       ... other args ....
> +
>  Machine-specific options
>  ------------------------
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4e3efbee16..1187a60d6e 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1245,7 +1245,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
>      target_ulong firmware_end_addr, kernel_start_addr;
>      const char *firmware_name = riscv_default_firmware_name(&s->soc[0]);
>      uint32_t fdt_load_addr;
> -    uint64_t kernel_entry;
> +    uint64_t kernel_entry = 0;
>
>      /*
>       * Only direct boot kernel is currently supported for KVM VM,
> @@ -1266,42 +1266,29 @@ static void virt_machine_done(Notifier *notifier, void *data)
>      firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
>                                                       start_addr, NULL);
>
> -    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) {
> +    if (drive_get(IF_PFLASH, 0, 0)) {
> +        if (machine->firmware && !strcmp(machine->firmware, "none")) {
> +            /*
> +             * Pflash was supplied but bios is none, let's overwrite the
> +             * address we jump to after reset to the base of the flash.
> +             */
> +            start_addr = virt_memmap[VIRT_FLASH].base;
> +        } else {
> +            /*
> +             * Pflash was supplied but bios is not none. In this case,
> +             * base of the flash would contain S-mode payload.
> +             */
> +            riscv_setup_firmware_boot(machine);
> +            kernel_entry = virt_memmap[VIRT_FLASH].base;
> +        }
> +    }
> +
> +    if (machine->kernel_filename && !kernel_entry) {
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
>                                                           firmware_end_addr);
>
>          kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>                                           kernel_start_addr, true, NULL);
> -    } else {
> -       /*
> -        * If dynamic firmware is used, it doesn't know where is the next mode
> -        * if kernel argument is not set.
> -        */
> -        kernel_entry = 0;
> -    }
> -
> -    if (drive_get(IF_PFLASH, 0, 0)) {
> -        /*
> -         * Pflash was supplied, let's overwrite the address we jump to after
> -         * reset to the base of the flash.
> -         */
> -        start_addr = virt_memmap[VIRT_FLASH].base;
>      }
>
>      fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
> --
> 2.34.1
>
>
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Posted by Alistair Francis 11 months, 2 weeks ago
On Tue, May 23, 2023 at 8:29 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> Currently, virt machine supports two pflash instances each with
> 32MB size. However, the first pflash is always assumed to
> contain M-mode firmware and reset vector is set to this if
> enabled. Hence, for S-mode payloads like EDK2, only one pflash
> instance is available for use. This means both code and NV variables
> of EDK2 will need to use the same pflash.
>
> The OS distros keep the EDK2 FW code as readonly. When non-volatile
> variables also need to share the same pflash, it is not possible
> to keep it as readonly since variables need write access.
>
> To resolve this issue, the code and NV variables need to be separated.
> But in that case we need an extra flash. Hence, modify the convention
> such that pflash0 will contain the M-mode FW only when "-bios none"
> option is used. Otherwise, pflash0 will contain the S-mode payload FW.
> This enables both pflash instances available for EDK2 use.
>
> Example usage:
> 1) pflash0 containing M-mode FW
> qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt
> or
> qemu-system-riscv64 -bios none \
> -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt
>
> 2) pflash0 containing S-mode payload like EDK2
> qemu-system-riscv64 -pflash <smode_fw_code> -pflash <smode_vars> -machine  virt
> or
> qemu-system-riscv64 -bios <opensbi_fw> \
> -pflash <smode_fw_code> \
> -pflash <smode_vars> \
> -machine  virt
> or
> qemu-system-riscv64 -bios <opensbi_fw> \
> -drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
> -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
> -machine virt
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

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

Alistair

> ---
>
> The issue is reported at
> https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6
>
> The patch is based on Alistair's riscv-to-apply.next branch.
>
> Changes since v2:
>         1) Reverted v2 changes and used v1 approach so that pflash0 can be used
>            for code and pflash1 for variable store.
>         2) Rebased to latest riscv-to-apply.next branch.
>         3) Added documentation for pflash usage.
>
> Changes since v1:
>         1) Simplified the fix such that it doesn't break current EDK2.
>
>  docs/system/riscv/virt.rst | 33 ++++++++++++++++++++++++
>  hw/riscv/virt.c            | 51 ++++++++++++++------------------------
>  2 files changed, 52 insertions(+), 32 deletions(-)
>
> diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
> index 4b16e41d7f..4ac0d38fdf 100644
> --- a/docs/system/riscv/virt.rst
> +++ b/docs/system/riscv/virt.rst
> @@ -53,6 +53,39 @@ with the default OpenSBI firmware image as the -bios. It also supports
>  the recommended RISC-V bootflow: U-Boot SPL (M-mode) loads OpenSBI fw_dynamic
>  firmware and U-Boot proper (S-mode), using the standard -bios functionality.
>
> +Using flash devices
> +-------------------
> +
> +The first flash device (pflash0) can contain either ROM code like Oreboot
> +or S-mode payload firmware code like EDK2. If the pflash0 contains the
> +ROM code, -bios should be set to none. Otherwise, pflash0 is assumed to
> +contain S-mode payload code.
> +
> +Firmware images used for pflash should be of size 32M.
> +
> +To boot as ROM code like Oreboot:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 -bios none -pflash <rom_code_image> \
> +       ... other args ....
> +
> +To boot as S-mode payload like EDK2:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 -pflash <s-mode_fw_code> -pflash <smode_fw_vars> \
> +       ... other args ....
> +
> +To boot as read-only S-mode payload like EDK2:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 -bios <opensbi_fw> \
> +       -drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
> +       -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
> +       ... other args ....
> +
>  Machine-specific options
>  ------------------------
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4e3efbee16..1187a60d6e 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1245,7 +1245,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
>      target_ulong firmware_end_addr, kernel_start_addr;
>      const char *firmware_name = riscv_default_firmware_name(&s->soc[0]);
>      uint32_t fdt_load_addr;
> -    uint64_t kernel_entry;
> +    uint64_t kernel_entry = 0;
>
>      /*
>       * Only direct boot kernel is currently supported for KVM VM,
> @@ -1266,42 +1266,29 @@ static void virt_machine_done(Notifier *notifier, void *data)
>      firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
>                                                       start_addr, NULL);
>
> -    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) {
> +    if (drive_get(IF_PFLASH, 0, 0)) {
> +        if (machine->firmware && !strcmp(machine->firmware, "none")) {
> +            /*
> +             * Pflash was supplied but bios is none, let's overwrite the
> +             * address we jump to after reset to the base of the flash.
> +             */
> +            start_addr = virt_memmap[VIRT_FLASH].base;
> +        } else {
> +            /*
> +             * Pflash was supplied but bios is not none. In this case,
> +             * base of the flash would contain S-mode payload.
> +             */
> +            riscv_setup_firmware_boot(machine);
> +            kernel_entry = virt_memmap[VIRT_FLASH].base;
> +        }
> +    }
> +
> +    if (machine->kernel_filename && !kernel_entry) {
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
>                                                           firmware_end_addr);
>
>          kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>                                           kernel_start_addr, true, NULL);
> -    } else {
> -       /*
> -        * If dynamic firmware is used, it doesn't know where is the next mode
> -        * if kernel argument is not set.
> -        */
> -        kernel_entry = 0;
> -    }
> -
> -    if (drive_get(IF_PFLASH, 0, 0)) {
> -        /*
> -         * Pflash was supplied, let's overwrite the address we jump to after
> -         * reset to the base of the flash.
> -         */
> -        start_addr = virt_memmap[VIRT_FLASH].base;
>      }
>
>      fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
> --
> 2.34.1
>
>
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Posted by Andrea Bolognani 11 months, 2 weeks ago
On Tue, May 23, 2023 at 03:58:05PM +0530, Sunil V L wrote:
> Currently, virt machine supports two pflash instances each with
> 32MB size. However, the first pflash is always assumed to
> contain M-mode firmware and reset vector is set to this if
> enabled. Hence, for S-mode payloads like EDK2, only one pflash
> instance is available for use. This means both code and NV variables
> of EDK2 will need to use the same pflash.
>
> The OS distros keep the EDK2 FW code as readonly. When non-volatile
> variables also need to share the same pflash, it is not possible
> to keep it as readonly since variables need write access.
>
> To resolve this issue, the code and NV variables need to be separated.
> But in that case we need an extra flash. Hence, modify the convention
> such that pflash0 will contain the M-mode FW only when "-bios none"
> option is used. Otherwise, pflash0 will contain the S-mode payload FW.
> This enables both pflash instances available for EDK2 use.
>
> Example usage:
> 1) pflash0 containing M-mode FW
> qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt
> or
> qemu-system-riscv64 -bios none \
> -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt
>
> 2) pflash0 containing S-mode payload like EDK2
> qemu-system-riscv64 -pflash <smode_fw_code> -pflash <smode_vars> -machine  virt
> or
> qemu-system-riscv64 -bios <opensbi_fw> \
> -pflash <smode_fw_code> \
> -pflash <smode_vars> \
> -machine  virt
> or
> qemu-system-riscv64 -bios <opensbi_fw> \
> -drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
> -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
> -machine virt

I wanted to test this, both directly with QEMU and through libvirt,
but I'm hitting two roadblocks.

First off, the only RISC-V edk2 build readily accessible to me (from
the edk2-riscv64 Fedora package) is configured to work off a R/W
pflash1. You said that you have edk2 patches making R/O CODE pflash0
and R/W VARS pflash1 ready. Any chance you could make either the
build output, or the patches and some hints on how to build edk2
after applying them, somewhere?

Going further and testing libvirt integration. After hacking around
other issues, I finally stumbled upon this error:

  qemu-system-riscv64: Property 'virt-machine.pflash0' not found

This is because a few years back libvirt has stopped using -drive
if=pflash for configuring pflash devices, and these days it generates
something along the lines of

  -blockdev '{"driver":"file","filename":"...","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}'
  -blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}'
  -machine virt,pflash0=libvirt-pflash0-format

instead, which both aarch64/virt and x86_64/q35 machine types are
perfectly happy with.

I've tried to patch hw/riscv/virt.c to get the above working, and
almost managed to succeed :) but eventually my unfamiliarity with the
internals of QEMU caught up with me. Can you please look into making
this work?

Thanks!

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Posted by Sunil V L 11 months, 2 weeks ago
Hi Andrea,

On Wed, May 24, 2023 at 03:50:34PM +0000, Andrea Bolognani wrote:
> On Tue, May 23, 2023 at 03:58:05PM +0530, Sunil V L wrote:
> > Currently, virt machine supports two pflash instances each with
> > 32MB size. However, the first pflash is always assumed to
> > contain M-mode firmware and reset vector is set to this if
> > enabled. Hence, for S-mode payloads like EDK2, only one pflash
> > instance is available for use. This means both code and NV variables
> > of EDK2 will need to use the same pflash.
> >
> > The OS distros keep the EDK2 FW code as readonly. When non-volatile
> > variables also need to share the same pflash, it is not possible
> > to keep it as readonly since variables need write access.
> >
> > To resolve this issue, the code and NV variables need to be separated.
> > But in that case we need an extra flash. Hence, modify the convention
> > such that pflash0 will contain the M-mode FW only when "-bios none"
> > option is used. Otherwise, pflash0 will contain the S-mode payload FW.
> > This enables both pflash instances available for EDK2 use.
> >
> > Example usage:
> > 1) pflash0 containing M-mode FW
> > qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt
> > or
> > qemu-system-riscv64 -bios none \
> > -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt
> >
> > 2) pflash0 containing S-mode payload like EDK2
> > qemu-system-riscv64 -pflash <smode_fw_code> -pflash <smode_vars> -machine  virt
> > or
> > qemu-system-riscv64 -bios <opensbi_fw> \
> > -pflash <smode_fw_code> \
> > -pflash <smode_vars> \
> > -machine  virt
> > or
> > qemu-system-riscv64 -bios <opensbi_fw> \
> > -drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
> > -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
> > -machine virt
> 
> I wanted to test this, both directly with QEMU and through libvirt,

Thank you very much for helping with this!

> but I'm hitting two roadblocks.
> 
> First off, the only RISC-V edk2 build readily accessible to me (from
> the edk2-riscv64 Fedora package) is configured to work off a R/W
> pflash1. You said that you have edk2 patches making R/O CODE pflash0
> and R/W VARS pflash1 ready. Any chance you could make either the
> build output, or the patches and some hints on how to build edk2
> after applying them, somewhere?
> 
Please build EDK2 using the branch
https://github.com/vlsunil/edk2/tree/separate_code_vars.

The instructions to build is in
https://github.com/vlsunil/riscv-uefi-edk2-docs/wiki/RISC-V-Qemu-Virt-support#build-edk2

However, now it will create two images for code and vars.

> Going further and testing libvirt integration. After hacking around
> other issues, I finally stumbled upon this error:
> 
>   qemu-system-riscv64: Property 'virt-machine.pflash0' not found
> 
> This is because a few years back libvirt has stopped using -drive
> if=pflash for configuring pflash devices, and these days it generates
> something along the lines of
> 
>   -blockdev '{"driver":"file","filename":"...","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}'
>   -blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}'
>   -machine virt,pflash0=libvirt-pflash0-format
> 
> instead, which both aarch64/virt and x86_64/q35 machine types are
> perfectly happy with.
> 
> I've tried to patch hw/riscv/virt.c to get the above working, and
> almost managed to succeed :) but eventually my unfamiliarity with the
> internals of QEMU caught up with me. Can you please look into making
> this work?
> 
Thanks!. This needs some investigation. Let me look into supporting
this.

Thanks!
Sunil
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Posted by Andrea Bolognani 11 months, 2 weeks ago
On Wed, May 24, 2023 at 11:03:52PM +0530, Sunil V L wrote:
> On Wed, May 24, 2023 at 03:50:34PM +0000, Andrea Bolognani wrote:
> > First off, the only RISC-V edk2 build readily accessible to me (from
> > the edk2-riscv64 Fedora package) is configured to work off a R/W
> > pflash1. You said that you have edk2 patches making R/O CODE pflash0
> > and R/W VARS pflash1 ready. Any chance you could make either the
> > build output, or the patches and some hints on how to build edk2
> > after applying them, somewhere?
>
> Please build EDK2 using the branch
> https://github.com/vlsunil/edk2/tree/separate_code_vars.
>
> The instructions to build is in
> https://github.com/vlsunil/riscv-uefi-edk2-docs/wiki/RISC-V-Qemu-Virt-support#build-edk2
>
> However, now it will create two images for code and vars.

Following your pointers, I was able to build suitable edk2 images and
verify that they work with a patched QEMU, so

  Tested-by: Andrea Bolognani <abologna@redhat.com>

Note, however, that said testing was limited to verifying that edk2
would come up.

I have also tried booting an openSUSE Tumbleweed "JeOS" image, since
that's the only distro I'm aware of that uses UEFI boot on RISC-V at
this point - though they use U-Boot's UEFI support rather than edk2.

During that attempt, I ended up in the edk2 shell. Running

  fs0:\efi\boot\bootriscv64.efi

brings up GRUB just fine, but selecting the default boot entry
results in

  Loading Linux 6.3.2-1-default ...
  Loading initial ramdisk ...
  EFI stub: Booting Linux Kernel...
  EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
  EFI stub: Generating empty DTB
  EFI stub: Exiting boot services...

being printed, after which it's back to OpenSBI and from there to
edk2 again.

I feel that this has probably more to do with the way the openSUSE
image has been built than edk2, but I thought I'd report my
experience nonetheless in case there's any useful information that
can be gathered from it :)

> > Going further and testing libvirt integration. After hacking around
> > other issues, I finally stumbled upon this error:
> >
> >   qemu-system-riscv64: Property 'virt-machine.pflash0' not found
>
> Thanks!. This needs some investigation. Let me look into supporting
> this.

Yes please! It's critical to libvirt integration. Feel free to CC me
when you post patches and I'll gladly test them.

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Posted by Sunil V L 11 months, 2 weeks ago
Hi Andrea,

On Thu, May 25, 2023 at 01:43:28PM +0000, Andrea Bolognani wrote:
> On Wed, May 24, 2023 at 11:03:52PM +0530, Sunil V L wrote:
> > On Wed, May 24, 2023 at 03:50:34PM +0000, Andrea Bolognani wrote:
> > > First off, the only RISC-V edk2 build readily accessible to me (from
> > > the edk2-riscv64 Fedora package) is configured to work off a R/W
> > > pflash1. You said that you have edk2 patches making R/O CODE pflash0
> > > and R/W VARS pflash1 ready. Any chance you could make either the
> > > build output, or the patches and some hints on how to build edk2
> > > after applying them, somewhere?
> >
> > Please build EDK2 using the branch
> > https://github.com/vlsunil/edk2/tree/separate_code_vars.
> >
> > The instructions to build is in
> > https://github.com/vlsunil/riscv-uefi-edk2-docs/wiki/RISC-V-Qemu-Virt-support#build-edk2
> >
> > However, now it will create two images for code and vars.
> 
> Following your pointers, I was able to build suitable edk2 images and
> verify that they work with a patched QEMU, so
> 
>   Tested-by: Andrea Bolognani <abologna@redhat.com>
> 
> Note, however, that said testing was limited to verifying that edk2
> would come up.
> 
> I have also tried booting an openSUSE Tumbleweed "JeOS" image, since
> that's the only distro I'm aware of that uses UEFI boot on RISC-V at
> this point - though they use U-Boot's UEFI support rather than edk2.
> 
> During that attempt, I ended up in the edk2 shell. Running
> 
>   fs0:\efi\boot\bootriscv64.efi
> 
> brings up GRUB just fine, but selecting the default boot entry
> results in
> 
>   Loading Linux 6.3.2-1-default ...
>   Loading initial ramdisk ...
>   EFI stub: Booting Linux Kernel...
>   EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
>   EFI stub: Generating empty DTB
>   EFI stub: Exiting boot services...
> 
> being printed, after which it's back to OpenSBI and from there to
> edk2 again.
> 
> I feel that this has probably more to do with the way the openSUSE
> image has been built than edk2, but I thought I'd report my
> experience nonetheless in case there's any useful information that
> can be gathered from it :)
> 
Thanks!. Please add -machine acpi=off in qemu command to boot the
kernel.

> > > Going further and testing libvirt integration. After hacking around
> > > other issues, I finally stumbled upon this error:
> > >
> > >   qemu-system-riscv64: Property 'virt-machine.pflash0' not found
> >
> > Thanks!. This needs some investigation. Let me look into supporting
> > this.
> 
> Yes please! It's critical to libvirt integration. Feel free to CC me
> when you post patches and I'll gladly test them.
> 
Sure, I have the fix ready. I need to convert this into a patch series
now. Will send it soon and thanks in advance for helping with testing.

Thanks,
Sunil
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Posted by Andrea Bolognani 11 months, 2 weeks ago
On Thu, May 25, 2023 at 08:32:46PM +0530, Sunil V L wrote:
> On Thu, May 25, 2023 at 01:43:28PM +0000, Andrea Bolognani wrote:
> > I have also tried booting an openSUSE Tumbleweed "JeOS" image, since
> > that's the only distro I'm aware of that uses UEFI boot on RISC-V at
> > this point - though they use U-Boot's UEFI support rather than edk2.
> >
> > During that attempt, I ended up in the edk2 shell. Running
> >
> >   fs0:\efi\boot\bootriscv64.efi
> >
> > brings up GRUB just fine, but selecting the default boot entry
> > results in
> >
> >   Loading Linux 6.3.2-1-default ...
> >   Loading initial ramdisk ...
> >   EFI stub: Booting Linux Kernel...
> >   EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
> >   EFI stub: Generating empty DTB
> >   EFI stub: Exiting boot services...
> >
> > being printed, after which it's back to OpenSBI and from there to
> > edk2 again.
>
> Thanks!. Please add -machine acpi=off in qemu command to boot the
> kernel.

Yup, that worked! It booted all the way to the login prompt :)

Note that libvirt automatically adds acpi=off, so that won't be a
concern for libvirt users.

> > > > Going further and testing libvirt integration. After hacking around
> > > > other issues, I finally stumbled upon this error:
> > > >
> > > >   qemu-system-riscv64: Property 'virt-machine.pflash0' not found
> > >
> > > Thanks!. This needs some investigation. Let me look into supporting
> > > this.
> >
> > Yes please! It's critical to libvirt integration. Feel free to CC me
> > when you post patches and I'll gladly test them.
>
> Sure, I have the fix ready. I need to convert this into a patch series
> now. Will send it soon and thanks in advance for helping with testing.

Excellent! Looking forward to it :)

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Posted by Sunil V L 11 months, 2 weeks ago
On Thu, May 25, 2023 at 04:19:12PM +0000, Andrea Bolognani wrote:
> On Thu, May 25, 2023 at 08:32:46PM +0530, Sunil V L wrote:
> > On Thu, May 25, 2023 at 01:43:28PM +0000, Andrea Bolognani wrote:
> > > I have also tried booting an openSUSE Tumbleweed "JeOS" image, since
> > > that's the only distro I'm aware of that uses UEFI boot on RISC-V at
> > > this point - though they use U-Boot's UEFI support rather than edk2.
> > >
> > > During that attempt, I ended up in the edk2 shell. Running
> > >
> > >   fs0:\efi\boot\bootriscv64.efi
> > >
> > > brings up GRUB just fine, but selecting the default boot entry
> > > results in
> > >
> > >   Loading Linux 6.3.2-1-default ...
> > >   Loading initial ramdisk ...
> > >   EFI stub: Booting Linux Kernel...
> > >   EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
> > >   EFI stub: Generating empty DTB
> > >   EFI stub: Exiting boot services...
> > >
> > > being printed, after which it's back to OpenSBI and from there to
> > > edk2 again.
> >
> > Thanks!. Please add -machine acpi=off in qemu command to boot the
> > kernel.
> 
> Yup, that worked! It booted all the way to the login prompt :)
> 
> Note that libvirt automatically adds acpi=off, so that won't be a
> concern for libvirt users.
> 
Great!. Thanks. Since we decided to keep acpi enabled by default in
qemu similar to other architectures, we need to turn this off until
complete ACPI support is merged in linux kernel. Thanks for the
information that libvirt turns it off by default.

> > > > > Going further and testing libvirt integration. After hacking around
> > > > > other issues, I finally stumbled upon this error:
> > > > >
> > > > >   qemu-system-riscv64: Property 'virt-machine.pflash0' not found
> > > >
> > > > Thanks!. This needs some investigation. Let me look into supporting
> > > > this.
> > >
> > > Yes please! It's critical to libvirt integration. Feel free to CC me
> > > when you post patches and I'll gladly test them.
> >
> > Sure, I have the fix ready. I need to convert this into a patch series
> > now. Will send it soon and thanks in advance for helping with testing.
> 
> Excellent! Looking forward to it :)
> 
Sent.

Thanks,
Sunil