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

[Alistair Francis]

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.

We would keep two pflash regions, hopefully in a way that doesn't
break any existing users.

Alistair

>
> 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?
> >
>
>