From: Alistair Francis <alistair23@gmail.com>
To: Sunil V L <sunilvl@ventanamicro.com>
Cc: qemu-riscv@nongnu.org, qemu-devel@nongnu.org,
Palmer Dabbelt <palmer@dabbelt.com>,
Alistair Francis <alistair.francis@wdc.com>,
Bin Meng <bin.meng@windriver.com>,
Weiwei Li <liweiwei@iscas.ac.cn>,
Daniel Henrique Barboza <dbarboza@ventanamicro.com>,
Liu Zhiwei <zhiwei_liu@linux.alibaba.com>,
Heinrich Schuchardt <xypron.glpk@gmx.de>
Subject: Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
Date: Fri, 26 May 2023 13:37:57 +1000 [thread overview]
Message-ID: <CAKmqyKNOU65R3Yc=K=xHZKb00_e88HkGG8jwO=qfng1fg78aAw@mail.gmail.com> (raw)
In-Reply-To: <20230523102805.100160-1-sunilvl@ventanamicro.com>
On Tue, May 23, 2023 at 8:29 PM Sunil V L <sunilvl@ventanamicro.com> 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_code> -pflash <smode_vars> -machine virt
> or
> qemu-system-riscv64 -bios <opensbi_fw> \
> -pflash <smode_fw_code> \
> -pflash <smode_vars> \
> -machine virt
> or
> qemu-system-riscv64 -bios <opensbi_fw> \
> -drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
> -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
> -machine virt
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
>
> The issue is reported at
> https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6
>
> The patch is based on Alistair's riscv-to-apply.next branch.
>
> Changes since v2:
> 1) Reverted v2 changes and used v1 approach so that pflash0 can be used
> for code and pflash1 for variable store.
> 2) Rebased to latest riscv-to-apply.next branch.
> 3) Added documentation for pflash usage.
>
> Changes since v1:
> 1) Simplified the fix such that it doesn't break current EDK2.
>
> docs/system/riscv/virt.rst | 33 ++++++++++++++++++++++++
> hw/riscv/virt.c | 51 ++++++++++++++------------------------
> 2 files changed, 52 insertions(+), 32 deletions(-)
>
> diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
> index 4b16e41d7f..4ac0d38fdf 100644
> --- a/docs/system/riscv/virt.rst
> +++ b/docs/system/riscv/virt.rst
> @@ -53,6 +53,39 @@ with the default OpenSBI firmware image as the -bios. It also supports
> the recommended RISC-V bootflow: U-Boot SPL (M-mode) loads OpenSBI fw_dynamic
> firmware and U-Boot proper (S-mode), using the standard -bios functionality.
>
> +Using flash devices
> +-------------------
> +
> +The first flash device (pflash0) can contain either ROM code like Oreboot
> +or S-mode payload firmware code like EDK2. If the pflash0 contains the
> +ROM code, -bios should be set to none. Otherwise, pflash0 is assumed to
> +contain S-mode payload code.
> +
> +Firmware images used for pflash should be of size 32M.
> +
> +To boot as ROM code like Oreboot:
> +
> +.. code-block:: bash
> +
> + $ qemu-system-riscv64 -bios none -pflash <rom_code_image> \
> + ... other args ....
> +
> +To boot as S-mode payload like EDK2:
> +
> +.. code-block:: bash
> +
> + $ qemu-system-riscv64 -pflash <s-mode_fw_code> -pflash <smode_fw_vars> \
> + ... other args ....
> +
> +To boot as read-only S-mode payload like EDK2:
> +
> +.. code-block:: bash
> +
> + $ qemu-system-riscv64 -bios <opensbi_fw> \
> + -drive file=<smode_fw_code>,if=pflash,format=raw,unit=0,readonly=on \
> + -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=1 \
> + ... other args ....
> +
> Machine-specific options
> ------------------------
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4e3efbee16..1187a60d6e 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1245,7 +1245,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
> target_ulong firmware_end_addr, kernel_start_addr;
> const char *firmware_name = riscv_default_firmware_name(&s->soc[0]);
> uint32_t fdt_load_addr;
> - uint64_t kernel_entry;
> + uint64_t kernel_entry = 0;
>
> /*
> * Only direct boot kernel is currently supported for KVM VM,
> @@ -1266,42 +1266,29 @@ static void virt_machine_done(Notifier *notifier, void *data)
> firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
> start_addr, NULL);
>
> - if (drive_get(IF_PFLASH, 0, 1)) {
> - /*
> - * S-mode FW like EDK2 will be kept in second plash (unit 1).
> - * When both kernel, initrd and pflash options are provided in the
> - * command line, the kernel and initrd will be copied to the fw_cfg
> - * table and opensbi will jump to the flash address which is the
> - * entry point of S-mode FW. It is the job of the S-mode FW to load
> - * the kernel and initrd using fw_cfg table.
> - *
> - * If only pflash is given but not -kernel, then it is the job of
> - * of the S-mode firmware to locate and load the kernel.
> - * In either case, the next_addr for opensbi will be the flash address.
> - */
> - riscv_setup_firmware_boot(machine);
> - kernel_entry = virt_memmap[VIRT_FLASH].base +
> - virt_memmap[VIRT_FLASH].size / 2;
> - } else if (machine->kernel_filename) {
> + if (drive_get(IF_PFLASH, 0, 0)) {
> + if (machine->firmware && !strcmp(machine->firmware, "none")) {
> + /*
> + * Pflash was supplied but bios is none, let's overwrite the
> + * address we jump to after reset to the base of the flash.
> + */
> + start_addr = virt_memmap[VIRT_FLASH].base;
> + } else {
> + /*
> + * Pflash was supplied but bios is not none. In this case,
> + * base of the flash would contain S-mode payload.
> + */
> + riscv_setup_firmware_boot(machine);
> + kernel_entry = virt_memmap[VIRT_FLASH].base;
> + }
> + }
> +
> + if (machine->kernel_filename && !kernel_entry) {
> kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
> kernel_start_addr, true, NULL);
> - } else {
> - /*
> - * If dynamic firmware is used, it doesn't know where is the next mode
> - * if kernel argument is not set.
> - */
> - kernel_entry = 0;
> - }
> -
> - if (drive_get(IF_PFLASH, 0, 0)) {
> - /*
> - * Pflash was supplied, let's overwrite the address we jump to after
> - * reset to the base of the flash.
> - */
> - start_addr = virt_memmap[VIRT_FLASH].base;
> }
>
> fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
> --
> 2.34.1
>
>
next prev parent reply other threads:[~2023-05-26 3:38 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-23 10:28 [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none" Sunil V L
2023-05-24 15:50 ` Andrea Bolognani
2023-05-24 17:33 ` Sunil V L
2023-05-25 13:43 ` Andrea Bolognani
2023-05-25 15:02 ` Sunil V L
2023-05-25 16:19 ` Andrea Bolognani
2023-05-25 16:54 ` Sunil V L
2023-05-26 3:37 ` Alistair Francis [this message]
2023-05-26 6:47 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKmqyKNOU65R3Yc=K=xHZKb00_e88HkGG8jwO=qfng1fg78aAw@mail.gmail.com' \
--to=alistair23@gmail.com \
--cc=alistair.francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=dbarboza@ventanamicro.com \
--cc=liweiwei@iscas.ac.cn \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=sunilvl@ventanamicro.com \
--cc=xypron.glpk@gmx.de \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).