|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none" |
Date: | Fri, 19 May 2023 18:40:37 +0200 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 |
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 pflashinstance is available for use. This means both code and NV variablesof 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 conventionsuch 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 EDK2qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virtor 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 ;)
[Prev in Thread] | Current Thread | [Next in Thread] |