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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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 ;)