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

[Philippe Mathieu-Daudé]

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.
>
> Thanks,
> Sunil