[PATCH v6 0/3] hw/riscv/virt: pflash improvements

[PATCH v6 0/3] hw/riscv/virt: pflash improvements

[Sunil V L]

This series improves the pflash usage in RISC-V virt machine with solutions to
below issues.

1) Currently the first pflash is reserved for ROM/M-mode firmware code. But S-mode
payload firmware like EDK2 need both pflash devices to have separate code and variable
store so that OS distros can keep the FW code as read-only. 

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

2) The latest way of using pflash devices in other architectures and libvirt
is by using -blockdev and machine options. However, currently this method is
not working in RISC-V.

With above issues fixed, added documentation on how to use pflash devices
in RISC-V virt machine.

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

Changes since v5:
	1) Added KVM use case as per feedback from Anup. Updated the documentation
	   patch that only S-mode payload is supported for KVM guests. Tested with
	   KVM enabled.
	2) Updated tags.

Changes since v4:
	1) Updated patch 2 to avoid accessing private field as per feedback from Philippe.
	2) Updated documentation patch to add read-only for ROM usage.
	3) Rebased to latest riscv-to-apply.next branch and updated tags.

Changes since v3:
	1) Converted single patch to a series with a cover letter since there are
	   multiple patches now.
	2) Added a new patch to enable pflash usage via -blockdev option.
	3) Separated the documentation change into new patch and updated the
	   documentation to mention only -blockdev option which seems to be the
	   recommended way of using pflash.

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.


Sunil V L (3):
  hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
  riscv/virt: Support using pflash via -blockdev option
  docs/system: riscv: Add pflash usage details

 docs/system/riscv/virt.rst | 43 +++++++++++++++++++++++++++
 hw/riscv/virt.c            | 59 ++++++++++++++++----------------------
 2 files changed, 68 insertions(+), 34 deletions(-)

-- 
2.34.1

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

[Sunil V L]

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
for non-KVM guests 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.

When KVM is enabled, pflash0 is always assumed to contain the
S-mode payload firmware only.

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>
Tested-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/virt.c | 53 ++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 32 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4e3efbee16..3e5dc649c3 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,31 @@ 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") &&
+            !kvm_enabled()) {
+            /*
+             * Pflash was supplied but bios is none and not KVM guest,
+             * 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 either KVM guest or 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

[PATCH v6 2/3] riscv/virt: Support using pflash via -blockdev option

[Sunil V L]

Currently, pflash devices can be configured only via -pflash
or -drive options. This is the legacy way and the
better way is to use -blockdev as in other architectures.
libvirt also has moved to use -blockdev method.

To support -blockdev option, pflash devices need to be
created in instance_init itself. So, update the code to
move the virt_flash_create() to instance_init. Also, use
standard interfaces to detect whether pflash0 is
configured or not.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reported-by: Andrea Bolognani <abologna@redhat.com>
Tested-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/virt.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 3e5dc649c3..76c7a3ba3b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1246,6 +1246,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
     const char *firmware_name = riscv_default_firmware_name(&s->soc[0]);
     uint32_t fdt_load_addr;
     uint64_t kernel_entry = 0;
+    BlockBackend *pflash_blk0;
 
     /*
      * Only direct boot kernel is currently supported for KVM VM,
@@ -1266,7 +1267,8 @@ 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, 0)) {
+    pflash_blk0 = pflash_cfi01_get_blk(s->flash[0]);
+    if (pflash_blk0) {
         if (machine->firmware && !strcmp(machine->firmware, "none") &&
             !kvm_enabled()) {
             /*
@@ -1499,8 +1501,6 @@ static void virt_machine_init(MachineState *machine)
     sysbus_create_simple("goldfish_rtc", memmap[VIRT_RTC].base,
         qdev_get_gpio_in(DEVICE(mmio_irqchip), RTC_IRQ));
 
-    virt_flash_create(s);
-
     for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
         /* Map legacy -drive if=pflash to machine properties */
         pflash_cfi01_legacy_drive(s->flash[i],
@@ -1527,6 +1527,8 @@ static void virt_machine_instance_init(Object *obj)
 {
     RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
 
+    virt_flash_create(s);
+
     s->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
     s->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
     s->acpi = ON_OFF_AUTO_AUTO;
-- 
2.34.1

[PATCH v6 3/3] docs/system: riscv: Add pflash usage details

[Sunil V L]

pflash devices can be used in virt machine for different
purposes like for ROM code or S-mode FW payload. Add a
section in the documentation on how to use pflash devices
for different purposes.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/system/riscv/virt.rst | 43 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
index 4b16e41d7f..6c1d4fed3e 100644
--- a/docs/system/riscv/virt.rst
+++ b/docs/system/riscv/virt.rst
@@ -53,6 +53,49 @@ 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
+-------------------
+
+When KVM is not enabled, the first flash device (pflash0) can contain either
+the ROM code or S-mode payload firmware code. If the pflash0 contains the
+ROM code, -bios should be set to none. If -bios is not set to
+none, pflash0 is assumed to contain S-mode payload code.
+
+When KVM is enabled, pflash0 is always assumed to contain the S-mode payload
+firmware.
+
+Firmware images used for pflash should be of size 32 MiB.
+
+To boot as ROM code:
+
+.. code-block:: bash
+
+  $ qemu-system-riscv64 -bios none \
+     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<rom_code> \
+     -M virt,pflash0=pflash0 \
+     ... other args ....
+
+To boot as read-only S-mode payload:
+
+.. code-block:: bash
+
+  $ qemu-system-riscv64 \
+     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<s-mode_fw_code> \
+     -blockdev node-name=pflash1,driver=file,filename=<s-mode_fw_vars> \
+     -M virt,pflash0=pflash0,pflash1=pflash1 \
+     ... other args ....
+
+To boot as read-only S-mode payload in KVM guest:
+
+.. code-block:: bash
+
+  $ qemu-system-riscv64 \
+     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<s-mode_fw_code> \
+     -blockdev node-name=pflash1,driver=file,filename=<s-mode_fw_vars> \
+     -M virt,pflash0=pflash0,pflash1=pflash1 \
+     --enable-kvm \
+     ... other args ....
+
 Machine-specific options
 ------------------------
 
-- 
2.34.1

Re: [PATCH v6 3/3] docs/system: riscv: Add pflash usage details

[Andrea Bolognani]

On Wed, May 31, 2023 at 07:53:00PM +0530, Sunil V L wrote:
> +Using flash devices
> +-------------------
> +
> +When KVM is not enabled, the first flash device (pflash0) can contain either
> +the ROM code or S-mode payload firmware code. If the pflash0 contains the
> +ROM code, -bios should be set to none. If -bios is not set to
> +none, pflash0 is assumed to contain S-mode payload code.
> +
> +When KVM is enabled, pflash0 is always assumed to contain the S-mode payload
> +firmware.
> +
> +Firmware images used for pflash should be of size 32 MiB.
> +
> +To boot as ROM code:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 -bios none \
> +     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<rom_code> \
> +     -M virt,pflash0=pflash0 \
> +     ... other args ....
> +
> +To boot as read-only S-mode payload:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 \
> +     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<s-mode_fw_code> \
> +     -blockdev node-name=pflash1,driver=file,filename=<s-mode_fw_vars> \
> +     -M virt,pflash0=pflash0,pflash1=pflash1 \
> +     ... other args ....
> +
> +To boot as read-only S-mode payload in KVM guest:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 \
> +     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<s-mode_fw_code> \
> +     -blockdev node-name=pflash1,driver=file,filename=<s-mode_fw_vars> \
> +     -M virt,pflash0=pflash0,pflash1=pflash1 \
> +     --enable-kvm \
> +     ... other args ....

I feel that this, while accurate, has gotten more complicated than it
needs to be. We're also putting the least common scenario front and
center instead of opening with the one that most people are going to
be using.

Below is how I suggest reworking it. What do you think?



Using flash devices
-------------------

By default, the first flash device (pflash0) is expected to contain
S-mode firmware code. It can be configured as read-only, with the
second flash device (pflash1) available to store configuration data.

For example, booting edk2 looks like

..code-block:: bash

  $ qemu-system-riscv64 \
     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<edk2_code> \
     -blockdev node-name=pflash1,driver=file,filename=<edk2_vars> \
     -M virt,pflash0=pflash0,pflash1=pflash1 \
     ... other args ....

For TCG guests only, it is also possible to boot M-mode firmware from
the first flash device (pflash0) by additionally passing ``-bios
none``, as in

..code-block:: bash

  $ qemu-system-riscv64 \
     -bios none \
     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<m_mode_code>
\
     -M virt,pflash0=pflash0 \
     ... other args ....

Firmware images used for pflash must be exactly 32 MiB in size.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v6 3/3] docs/system: riscv: Add pflash usage details

[Sunil V L]

On Wed, May 31, 2023 at 09:43:39AM -0700, Andrea Bolognani wrote:
> On Wed, May 31, 2023 at 07:53:00PM +0530, Sunil V L wrote:
> > +Using flash devices
> > +-------------------
> > +
> > +When KVM is not enabled, the first flash device (pflash0) can contain either
> > +the ROM code or S-mode payload firmware code. If the pflash0 contains the
> > +ROM code, -bios should be set to none. If -bios is not set to
> > +none, pflash0 is assumed to contain S-mode payload code.
> > +
> > +When KVM is enabled, pflash0 is always assumed to contain the S-mode payload
> > +firmware.
> > +
> > +Firmware images used for pflash should be of size 32 MiB.
> > +
> > +To boot as ROM code:
> > +
> > +.. code-block:: bash
> > +
> > +  $ qemu-system-riscv64 -bios none \
> > +     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<rom_code> \
> > +     -M virt,pflash0=pflash0 \
> > +     ... other args ....
> > +
> > +To boot as read-only S-mode payload:
> > +
> > +.. code-block:: bash
> > +
> > +  $ qemu-system-riscv64 \
> > +     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<s-mode_fw_code> \
> > +     -blockdev node-name=pflash1,driver=file,filename=<s-mode_fw_vars> \
> > +     -M virt,pflash0=pflash0,pflash1=pflash1 \
> > +     ... other args ....
> > +
> > +To boot as read-only S-mode payload in KVM guest:
> > +
> > +.. code-block:: bash
> > +
> > +  $ qemu-system-riscv64 \
> > +     -blockdev node-name=pflash0,driver=file,read-only=on,filename=<s-mode_fw_code> \
> > +     -blockdev node-name=pflash1,driver=file,filename=<s-mode_fw_vars> \
> > +     -M virt,pflash0=pflash0,pflash1=pflash1 \
> > +     --enable-kvm \
> > +     ... other args ....
> 
> I feel that this, while accurate, has gotten more complicated than it
> needs to be. We're also putting the least common scenario front and
> center instead of opening with the one that most people are going to
> be using.
> 
> Below is how I suggest reworking it. What do you think?
> 
> 
> 
> Using flash devices
> -------------------
> 
> By default, the first flash device (pflash0) is expected to contain
> S-mode firmware code. It can be configured as read-only, with the
> second flash device (pflash1) available to store configuration data.
> 
> For example, booting edk2 looks like
> 
> ..code-block:: bash
> 
>   $ qemu-system-riscv64 \
>      -blockdev node-name=pflash0,driver=file,read-only=on,filename=<edk2_code> \
>      -blockdev node-name=pflash1,driver=file,filename=<edk2_vars> \
>      -M virt,pflash0=pflash0,pflash1=pflash1 \
>      ... other args ....
> 
> For TCG guests only, it is also possible to boot M-mode firmware from
> the first flash device (pflash0) by additionally passing ``-bios
> none``, as in
> 
> ..code-block:: bash
> 
>   $ qemu-system-riscv64 \
>      -bios none \
>      -blockdev node-name=pflash0,driver=file,read-only=on,filename=<m_mode_code>
> \
>      -M virt,pflash0=pflash0 \
>      ... other args ....
> 
> Firmware images used for pflash must be exactly 32 MiB in size.
> 
Hi Andrea,

This looks great! Thank you very much. Unless I see an objection, I will
use this text in the next revision of the series tomorrow.

Thanks!
Sunil