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

[Sunil V L <sunilvl@ventanamicro.com>]

On Thu, May 18, 2023 at 02:55:16PM +1000, Alistair Francis wrote:
> On Wed, May 17, 2023 at 10:48 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> >
> > On 8/5/23 12:00, Andrea Bolognani wrote:
> > > On Mon, May 08, 2023 at 11:37:43AM +0530, Sunil V L wrote:
> > >> On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote:
> > >>> On 4/25/23 12:25, 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_vars> -pflash <smode_fw_code> -machine  virt
> > >>>> or
> > >>>> qemu-system-riscv64 -bios <opensbi_fw> \
> > >>>> -pflash <smode_fw_vars> \
> > >>>> -pflash <smode_fw_code> \
> > >>>
> > >>> On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables.
> > >>> Shouldn't riscv64 do the same?
> > >
> > > Good catch, I had missed that!
> >
> > This is a design mistake spreading.
> >
> > What EDK2 maintainers want is one Read-Only + Exec region for CODE and
> > one Read-Write + NoExec region for VARS.
> >
> > QEMU never implemented correctly pflash bank (multiple sectors) write
> > protected.
> >
> > When EDK2 (x86, OVMF) was tried on QEMU, QEMU was using a single pflash.
> > To separate CODE/VARS, a second pflash was added, the first one being
> > "locked" into Read-Only mode. Using a pflash allowed the firmware to
> > identify device size using pflash CFI commands.
> >
> > Then this design was copied to the ARM virt board for EDK2 needs.
> >
> > In retrospective, this design was declared a mistake, since a simple
> > ROM region for the CODE is sufficient, and much simpler [*].
> 
> It seems like we are making changes to the whole flash setup. Is it
> worth adding the ROM region now as well, so we can migrate to the
> simpler approach.
> 

ROM is used for FW image used for -bios option in RISC-V. It appears
that loongarch uses -bios to boot EDK2 also as per
docs/system/loongarch/virt.rst. But in RISC-V, EDK2 is S-mode payload
and hence -bios can not be used. If we have to create ROM for S-mode
payload, do we need to add a new qemu option? Also, I don't see
enough space in the memmap of RISC-V virt machine for PAYLOAD_ROM
region. So, I am not sure how to keep 2 flashes and add ROM region for
S-mode payload. Let me know if I am missing some thing.

> We would keep two pflash regions, hopefully in a way that doesn't
> break any existing users.
> 
Agree. While I agree with Philippe, I think we better solve the current
problem by going back to v1 of the patch.

Thanks,
Sunil