Re: [PATCH] hw/riscv: virt: Enable booting M-mode or S-mode FW from pflash0

[Heinrich Schuchardt]

On 4/21/23 06:33, 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_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>

QEMU 7.2 (and possibly 8.0 to be released) contains the old behavior.

Changed use of command line parameters should depend on the version of
the virt machine, i.e. virt-7.2 should use the old behavior and virt as
alias for virt-8.0 should use the new behavior. Please, have a look at
the option handling in hw/arm/virt.c and macro DEFINE_VIRT_MACHINE().

Best regards

Heinrich

> ---
>   hw/riscv/virt.c | 51 ++++++++++++++++++-------------------------------
>   1 file changed, 19 insertions(+), 32 deletions(-)
>
> 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,