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

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

On Mon, May 08, 2023 at 03:00:02AM -0700, 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!
> 
> > 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.
> 
It is not just tweaking the command line. The current EDK2 will not work
anymore if code is moved to plfash 0 since EDK2 assumed its entry point
is in pflash1. I agree there may not be too many users but if we have
to align with other archs, there will be combinations of qemu and
edk2 versions which won't work.

> With that in mind, my preference would be to go back to v1.
> 
Thanks!. If this is the preference,  we can request people to use proper
versions of EDK2 with different qemu versions.

> > 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?
>
TBH, I don't know. I am sure Alistair would know since it was added in
https://github.com/qemu/qemu/commit/1c20d3ff6004b600336c52cbef9f134fad3ccd94
I don't think opensbi can be launched from pflash. So, it may be some
other use case which I am now aware of.

I will be happy if this can be avoided by using -bios.

Thanks,
Sunil