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

[Philippe Mathieu-Daudé]

On 19/5/23 18:34, Philippe Mathieu-Daudé wrote:
> On 18/5/23 08:03, Sunil V L wrote:
> > 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.
>
> Personally I'd try to not use -bios at all, as I see it as a magic /
> confusing machine-specific command line option, doing some open-coded
> "load this blob somewhere in memory". I prefer using explicit hardware
> device model.
>
> -bios usually call the "hw/loader.h" API which eventually model a ROM,
> but do a lot of file format parsing magic. If you pass a plain ROM file,
> no need for all this magic.
>
> > 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.

BTW clarifying, I'm not rejecting this particular patch; I was just
trying to correct the idea than "doing what other architectures do"
is always the right approach ;)