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

[Philippe Mathieu-Daudé]

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 [*].

Thankfully the Loongarch64 virt machine started cleanly avoiding the
previous design flaw. It provides a ROM for CODE and pflash for VARS.

Unfortunately the riscv virt machine copied the x86/arm virt machines.


[*] Having 2 distinct pflash is useful for non-virt machines where the
    firmware might want to (re)program the CODE region, in the "capsule
    update" scenario. This scenario is irrelevant for virt machines,
    since a guest will never update its CODE. CODE is updated by the
    host.

> > Is that a requirement from distros perspective? That was my original v1
> > design.
> >
> > But the reason why I kept unit0 for variables, it helps in keeping current
> > EDK2 usage model work. Otherwise, current EDK2 will break if we change
> > the code to unit 0.
>
> I think that it's more important to align with other architectures.
>
> The number of people currently running edk2 on RISC-V is probably
> vanishingly small, and in my opinion requiring them to tweak their
> command lines a bit is a fair price to pay to avoid having to carry a
> subtle difference between architectures for years to come.
>
> With that in mind, my preference would be to go back to v1.
>
> > Second, since unit 0 for RISC-V is currently assumed to start in M-mode fw
> > which is secure, I think it makes sense to keep variables also in unit
> > 0.
>
> If you're storing variables rather than code in pflash0, does it even
> make sense to talk about M-mode and S-mode?
>
>
> Taking a step back, what is even the use case for having M-mode code
> in pflash0? If you want to use an M-mode firmware, can't you just use
> -bios instead? In other words, can we change the behavior so that
> pflash being present always mean loading S-mode firmware off it?