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

[PATCH v4 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 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 | 29 +++++++++++++++++++
 hw/riscv/virt.c            | 58 ++++++++++++++++----------------------
 2 files changed, 53 insertions(+), 34 deletions(-)

-- 
2.34.1

[PATCH v4 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
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>
Tested-by: Andrea Bolognani <abologna@redhat.com>
---
 hw/riscv/virt.c | 51 ++++++++++++++++++-------------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

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

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

[Sunil V L]

Currently, pflash devices can be configured only via -pflash
or if=pflash options. This is the legacy way and the
better way is to use -blockdev as in other architectures.
libvirt also has moved to -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>
---
 hw/riscv/virt.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 1187a60d6e..48fcbdbf06 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1246,6 +1246,8 @@ 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;
+    MemoryRegion *flash_mem;
 
     /*
      * Only direct boot kernel is currently supported for KVM VM,
@@ -1265,21 +1267,22 @@ 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) {
+        flash_mem = pflash_cfi01_get_memory(s->flash[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;
+            start_addr = flash_mem->addr;
         } 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;
+            kernel_entry = flash_mem->addr;
         }
     }
 
@@ -1497,8 +1500,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],
@@ -1525,6 +1526,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 v4 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>
---
 docs/system/riscv/virt.rst | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
index 4b16e41d7f..c3e0b7a9c3 100644
--- a/docs/system/riscv/virt.rst
+++ b/docs/system/riscv/virt.rst
@@ -53,6 +53,35 @@ 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
+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.
+
+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,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 ....
+
 Machine-specific options
 ------------------------
 
-- 
2.34.1

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

[Andrea Bolognani]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

On Thu, May 25, 2023 at 10:18:00PM +0530, Sunil V L wrote:
> 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 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.

Success! \o/

With these patches applied, libvirt built from the master branch,
edk2 built from your branch and a JSON firmware descriptor for it
installed (attached), it's finally possible to boot an unmodified
openSUSE Tumbleweed RISC-V disk image by simply including

  <os firmware='efi'>

in the domain XML, which is the same experience you'd have with
x86_64 or aarch64. That's a *massive* leap forward when it comes
to giving developers convenient access to a reasonable RISC-V
environment they can play around with!

Thanks a lot for your work on this, and I can't wait to see it
all merged :)


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

-- 
Andrea Bolognani / Red Hat / Virtualization

[-- Attachment #2: 50-edk2-riscv64-raw.json --]
[-- Type: application/json, Size: 592 bytes --]

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

[Andrew Jones]

On Thu, May 25, 2023 at 11:03:52AM -0700, Andrea Bolognani wrote:
> On Thu, May 25, 2023 at 10:18:00PM +0530, Sunil V L wrote:
> > 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 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.
> 
> Success! \o/
> 
> With these patches applied, libvirt built from the master branch,
> edk2 built from your branch and a JSON firmware descriptor for it
> installed (attached), it's finally possible to boot an unmodified
> openSUSE Tumbleweed RISC-V disk image by simply including
> 
>   <os firmware='efi'>

Hi Andrea,

I'm a bit concerned that we don't also need to add some XML in order to
disable ACPI right now. RISC-V guest kernels will support ACPI in the
near future. Ideally a default libvirt VM using edk2 will also use ACPI.
Will there be a problem with changing that default later? If so, then
I'd change it now and continue burdening developers a bit longer by
requiring them to explicitly disable it.

Thanks,
drew

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

[Andrea Bolognani]

On Fri, May 26, 2023 at 08:39:07AM +0200, Andrew Jones wrote:
> On Thu, May 25, 2023 at 11:03:52AM -0700, Andrea Bolognani wrote:
> > With these patches applied, libvirt built from the master branch,
> > edk2 built from your branch and a JSON firmware descriptor for it
> > installed (attached), it's finally possible to boot an unmodified
> > openSUSE Tumbleweed RISC-V disk image by simply including
> >
> >   <os firmware='efi'>
>
> Hi Andrea,
>
> I'm a bit concerned that we don't also need to add some XML in order to
> disable ACPI right now. RISC-V guest kernels will support ACPI in the
> near future. Ideally a default libvirt VM using edk2 will also use ACPI.
> Will there be a problem with changing that default later? If so, then
> I'd change it now and continue burdening developers a bit longer by
> requiring them to explicitly disable it.

libvirt doesn't enable ACPI by default on any architecture, not even
x86_64. virt-manager will enable it by default if it's advertised as
available on the architecture in the capabilities XML.

However, it looks like the corresponding code in libvirt is not as
dynamic as I would have assumed: instead, we hardcode the list of
architectures that advertise ACPI support available, and at the
moment that list does *not* include RISC-V :)

I think it would make sense to fix this, but I want to make sure I
understand the impact. Is this just an UEFI thing? All my other
RISC-V guests (Fedora, Ubuntu, FreeBSD) boot just fine when I turn
ACPI on. In fact, even the openSUSE one works with ACPI on, as long
as the UEFI implementation used is the U-Boot one rather than edk2.

So, are edk2 users the only ones who would (temporarily) need to
manually turn ACPI off if virt-manager started enabling it by
default?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Sunil V L]

On Fri, May 26, 2023 at 03:49:11AM -0400, Andrea Bolognani wrote:
> On Fri, May 26, 2023 at 08:39:07AM +0200, Andrew Jones wrote:
> > On Thu, May 25, 2023 at 11:03:52AM -0700, Andrea Bolognani wrote:
> > > With these patches applied, libvirt built from the master branch,
> > > edk2 built from your branch and a JSON firmware descriptor for it
> > > installed (attached), it's finally possible to boot an unmodified
> > > openSUSE Tumbleweed RISC-V disk image by simply including
> > >
> > >   <os firmware='efi'>
> >
> > Hi Andrea,
> >
> > I'm a bit concerned that we don't also need to add some XML in order to
> > disable ACPI right now. RISC-V guest kernels will support ACPI in the
> > near future. Ideally a default libvirt VM using edk2 will also use ACPI.
> > Will there be a problem with changing that default later? If so, then
> > I'd change it now and continue burdening developers a bit longer by
> > requiring them to explicitly disable it.
> 
> libvirt doesn't enable ACPI by default on any architecture, not even
> x86_64. virt-manager will enable it by default if it's advertised as
> available on the architecture in the capabilities XML.
> 
> However, it looks like the corresponding code in libvirt is not as
> dynamic as I would have assumed: instead, we hardcode the list of
> architectures that advertise ACPI support available, and at the
> moment that list does *not* include RISC-V :)
> 
> I think it would make sense to fix this, but I want to make sure I
> understand the impact. Is this just an UEFI thing? All my other
> RISC-V guests (Fedora, Ubuntu, FreeBSD) boot just fine when I turn
> ACPI on. In fact, even the openSUSE one works with ACPI on, as long
> as the UEFI implementation used is the U-Boot one rather than edk2.
> 
> So, are edk2 users the only ones who would (temporarily) need to
> manually turn ACPI off if virt-manager started enabling it by
> default?
> 

Right. Currently only EDK2 supports passing ACPI information to the
kernel in RISC-V. I think it is true even in aarch64. Hence, we need
this temporary solution to turn acpi off only for EDK2 users.

Is there an issue with delaying the fix in virt-manager to add RISC-V
to ACPI supported list? I think there is no point enabling it until
kernel also supports it.

Thanks,
Sunil

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

[Andrew Jones]

On Fri, May 26, 2023 at 03:49:11AM -0400, Andrea Bolognani wrote:
> On Fri, May 26, 2023 at 08:39:07AM +0200, Andrew Jones wrote:
> > On Thu, May 25, 2023 at 11:03:52AM -0700, Andrea Bolognani wrote:
> > > With these patches applied, libvirt built from the master branch,
> > > edk2 built from your branch and a JSON firmware descriptor for it
> > > installed (attached), it's finally possible to boot an unmodified
> > > openSUSE Tumbleweed RISC-V disk image by simply including
> > >
> > >   <os firmware='efi'>
> >
> > Hi Andrea,
> >
> > I'm a bit concerned that we don't also need to add some XML in order to
> > disable ACPI right now. RISC-V guest kernels will support ACPI in the
> > near future. Ideally a default libvirt VM using edk2 will also use ACPI.
> > Will there be a problem with changing that default later? If so, then
> > I'd change it now and continue burdening developers a bit longer by
> > requiring them to explicitly disable it.
> 
> libvirt doesn't enable ACPI by default on any architecture, not even
> x86_64. virt-manager will enable it by default if it's advertised as
> available on the architecture in the capabilities XML.
> 
> However, it looks like the corresponding code in libvirt is not as
> dynamic as I would have assumed: instead, we hardcode the list of
> architectures that advertise ACPI support available, and at the
> moment that list does *not* include RISC-V :)
> 
> I think it would make sense to fix this, but I want to make sure I
> understand the impact. Is this just an UEFI thing? All my other
> RISC-V guests (Fedora, Ubuntu, FreeBSD) boot just fine when I turn
> ACPI on. In fact, even the openSUSE one works with ACPI on, as long
> as the UEFI implementation used is the U-Boot one rather than edk2.
> 
> So, are edk2 users the only ones who would (temporarily) need to
> manually turn ACPI off if virt-manager started enabling it by
> default?

I assume so, but I'm not tracking firmware status. If the firmware
doesn't extract the ACPI tables from QEMU and present them to the
guest (afaik only edk2 does that), then the guest kernel falls back
to DT, which is why it's working for you.

I suppose we should wait until Linux merges the ACPI patches, before
adding RISC-V to the libvirt capabilities ACPI list. Then, is it
possible to use something like libosinfo to inform virt-manager
when it should enable ACPI and when not? Later distro images, with
later kernels, will want to use ACPI by default, but older images
will still need to use DT.

Thanks,
drew

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

[Andrea Bolognani]

On Fri, May 26, 2023 at 10:34:36AM +0200, Andrew Jones wrote:
> On Fri, May 26, 2023 at 03:49:11AM -0400, Andrea Bolognani wrote:
> > So, are edk2 users the only ones who would (temporarily) need to
> > manually turn ACPI off if virt-manager started enabling it by
> > default?
>
> I assume so, but I'm not tracking firmware status. If the firmware
> doesn't extract the ACPI tables from QEMU and present them to the
> guest (afaik only edk2 does that), then the guest kernel falls back
> to DT, which is why it's working for you.
>
> I suppose we should wait until Linux merges the ACPI patches, before
> adding RISC-V to the libvirt capabilities ACPI list.

That sounds reasonable to me, but note that 1) the libvirt change
might take a while to propagate to distros and 2) someone will have
to remind me to prepare such a patch when the time comes ;)

> Then, is it
> possible to use something like libosinfo to inform virt-manager
> when it should enable ACPI and when not? Later distro images, with
> later kernels, will want to use ACPI by default, but older images
> will still need to use DT.

Something like that would definitely be possible, but I don't think
the scaffolding for it exists at the moment, so someone would have to
wire it up across the stack. Given how relatively immature the RISC-V
distro ecosystem is at the moment, I think it's fine to do nothing
and wait for the problem to go away on its own :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Andrew Jones]

On Fri, May 26, 2023 at 04:42:57AM -0400, Andrea Bolognani wrote:
> On Fri, May 26, 2023 at 10:34:36AM +0200, Andrew Jones wrote:
> > On Fri, May 26, 2023 at 03:49:11AM -0400, Andrea Bolognani wrote:
> > > So, are edk2 users the only ones who would (temporarily) need to
> > > manually turn ACPI off if virt-manager started enabling it by
> > > default?
> >
> > I assume so, but I'm not tracking firmware status. If the firmware
> > doesn't extract the ACPI tables from QEMU and present them to the
> > guest (afaik only edk2 does that), then the guest kernel falls back
> > to DT, which is why it's working for you.
> >
> > I suppose we should wait until Linux merges the ACPI patches, before
> > adding RISC-V to the libvirt capabilities ACPI list.
> 
> That sounds reasonable to me, but note that 1) the libvirt change
> might take a while to propagate to distros and 2) someone will have
> to remind me to prepare such a patch when the time comes ;)

Initial ACPI support will probably be merged for 6.4. So maybe it is
time to get the libvirt side of things going.

> 
> > Then, is it
> > possible to use something like libosinfo to inform virt-manager
> > when it should enable ACPI and when not? Later distro images, with
> > later kernels, will want to use ACPI by default, but older images
> > will still need to use DT.
> 
> Something like that would definitely be possible, but I don't think
> the scaffolding for it exists at the moment, so someone would have to
> wire it up across the stack. Given how relatively immature the RISC-V
> distro ecosystem is at the moment, I think it's fine to do nothing
> and wait for the problem to go away on its own :)

WFM

Thanks,
drew

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

[Philippe Mathieu-Daudé]

On 25/5/23 18:48, Sunil V L wrote:
> 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>
> ---
>   docs/system/riscv/virt.rst | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
> index 4b16e41d7f..c3e0b7a9c3 100644
> --- a/docs/system/riscv/virt.rst
> +++ b/docs/system/riscv/virt.rst
> @@ -53,6 +53,35 @@ 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
> +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.
> +
> +Firmware images used for pflash should be of size 32 MiB.
> +
> +To boot as ROM code:

If "ROM" ...

> +
> +.. code-block:: bash
> +
> +  $ qemu-system-riscv64 -bios none \
> +     -blockdev node-name=pflash0,driver=file,filename=<rom_code> \

... shouldn't we also enforce 'read-only=on,' ...

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

... like in S-mode?

> +     -blockdev node-name=pflash1,driver=file,filename=<s-mode_fw_vars> \
> +     -M virt,pflash0=pflash0,pflash1=pflash1 \
> +     ... other args ....
> +
>   Machine-specific options
>   ------------------------
>   

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

[Philippe Mathieu-Daudé]

On 25/5/23 18:48, Sunil V L wrote:
> Currently, pflash devices can be configured only via -pflash
> or if=pflash options. This is the legacy way and the
> better way is to use -blockdev as in other architectures.
> libvirt also has moved to -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>
> ---
>   hw/riscv/virt.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)


> @@ -1265,21 +1267,22 @@ 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) {
> +        flash_mem = pflash_cfi01_get_memory(s->flash[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;
> +            start_addr = flash_mem->addr;

I don't understand this change. Besides you access MemoryRegion::addr
which is an internal API field.

>           } 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;
> +            kernel_entry = flash_mem->addr;
>           }
>       }

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

[Sunil V L]

On Fri, May 26, 2023 at 11:53:18AM +0200, Philippe Mathieu-Daudé wrote:
> On 25/5/23 18:48, Sunil V L wrote:
> > Currently, pflash devices can be configured only via -pflash
> > or if=pflash options. This is the legacy way and the
> > better way is to use -blockdev as in other architectures.
> > libvirt also has moved to -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>
> > ---
> >   hw/riscv/virt.c | 15 +++++++++------
> >   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> 
> > @@ -1265,21 +1267,22 @@ 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) {
> > +        flash_mem = pflash_cfi01_get_memory(s->flash[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;
> > +            start_addr = flash_mem->addr;
> 
> I don't understand this change. Besides you access MemoryRegion::addr
> which is an internal API field.
> 
Thanks Philip. This is not really required. I was trying to avoid
depending on memmap base addresses and dividing the memmap
range by 2 to get the second flash memory base etc. Since we have
created MemoryRegion already which has the actual base address for each
pflash, I was thinking to avoid this hardcoding. But I didn't realize I
am accessing the internal field.

Let me revert this particular change.

Thanks,
Sunil

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

[Andrea Bolognani]

On Fri, May 26, 2023 at 11:10:12AM +0200, Andrew Jones wrote:
> On Fri, May 26, 2023 at 04:42:57AM -0400, Andrea Bolognani wrote:
> > On Fri, May 26, 2023 at 10:34:36AM +0200, Andrew Jones wrote:
> > > On Fri, May 26, 2023 at 03:49:11AM -0400, Andrea Bolognani wrote:
> > > > So, are edk2 users the only ones who would (temporarily) need to
> > > > manually turn ACPI off if virt-manager started enabling it by
> > > > default?
> > >
> > > I assume so, but I'm not tracking firmware status. If the firmware
> > > doesn't extract the ACPI tables from QEMU and present them to the
> > > guest (afaik only edk2 does that), then the guest kernel falls back
> > > to DT, which is why it's working for you.
> > >
> > > I suppose we should wait until Linux merges the ACPI patches, before
> > > adding RISC-V to the libvirt capabilities ACPI list.
> >
> > That sounds reasonable to me, but note that 1) the libvirt change
> > might take a while to propagate to distros and 2) someone will have
> > to remind me to prepare such a patch when the time comes ;)
>
> Initial ACPI support will probably be merged for 6.4. So maybe it is
> time to get the libvirt side of things going.

Randomly remembered about this. Did ACPI support make it into 6.4
after all? Is now a good time to change libvirt?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Sunil V L]

On Mon, Nov 20, 2023 at 02:29:28PM +0000, Andrea Bolognani wrote:
> On Fri, May 26, 2023 at 11:10:12AM +0200, Andrew Jones wrote:
> > On Fri, May 26, 2023 at 04:42:57AM -0400, Andrea Bolognani wrote:
> > > On Fri, May 26, 2023 at 10:34:36AM +0200, Andrew Jones wrote:
> > > > On Fri, May 26, 2023 at 03:49:11AM -0400, Andrea Bolognani wrote:
> > > > > So, are edk2 users the only ones who would (temporarily) need to
> > > > > manually turn ACPI off if virt-manager started enabling it by
> > > > > default?
> > > >
> > > > I assume so, but I'm not tracking firmware status. If the firmware
> > > > doesn't extract the ACPI tables from QEMU and present them to the
> > > > guest (afaik only edk2 does that), then the guest kernel falls back
> > > > to DT, which is why it's working for you.
> > > >
> > > > I suppose we should wait until Linux merges the ACPI patches, before
> > > > adding RISC-V to the libvirt capabilities ACPI list.
> > >
> > > That sounds reasonable to me, but note that 1) the libvirt change
> > > might take a while to propagate to distros and 2) someone will have
> > > to remind me to prepare such a patch when the time comes ;)
> >
> > Initial ACPI support will probably be merged for 6.4. So maybe it is
> > time to get the libvirt side of things going.
> 
> Randomly remembered about this. Did ACPI support make it into 6.4
> after all? Is now a good time to change libvirt?
> 
Hi Andrea,

Not yet. While basic ACPI changes are merged, the interrupt controller
support is still going on. Looks like it will take few merge windows to
get ACPI fully supported. So, we still need to wait for libvirt change.

Thanks!
Sunil