Hi Cedric,
> Cc: Troy Lee <[email protected]>; [email protected]
> Subject: Re: [PATCH v4 06/10] hw/arm/aspeed: Add support for loading
> vbootrom image via "-bios"
>
> On 4/17/25 05:12, Jamin Lin wrote:
> > Introduce "aspeed_load_vbootrom()" to support loading a virtual boot
> > ROM image into the vbootrom memory region, using the "-bios"
> command-line option.
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > Reviewed-by: Nabih Estefan <[email protected]>
> > Tested-by: Nabih Estefan <[email protected]>
> > ---
> > include/hw/arm/aspeed.h | 1 +
> > hw/arm/aspeed.c | 36
> ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 37 insertions(+)
> >
> > diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h index
> > 973277bea6..2b8332a7d7 100644
> > --- a/include/hw/arm/aspeed.h
> > +++ b/include/hw/arm/aspeed.h
> > @@ -41,6 +41,7 @@ struct AspeedMachineClass {
> > uint32_t uart_default;
> > bool sdhci_wp_inverted;
> > bool vbootrom;
> > + const char *vbootrom_name;
>
> I don't think adding a class attribute for the default firmware image file is
> that
> useful. A define should be enough :
>
> #define VBOOTROM_FILE_NAME "ast27x0_bootrom.bin"
>
> and use VBOOTROM_FILE_NAME when defining the bios_name variable.
>
> Unless you already plan to have different default firmware files for the
> ast27*
> machines ?
>
If we plan to support the vbootrom feature for multiple ASPEED SoCs such as the
AST28x0 series, it seems that the current default define
for the image name cannot be reused for both AST27x0 and future ASPEED SoCs.
This is because the vbootrom file names differ between the two series.
That’s the reason I introduced a machine class attribute — to allow flexibility
in specifying the correct vbootrom name for each SoC family.
I will use the DEFINE for now in the v5 patch. When we plan to support vbootrom
for new SoCs in the future, we can revisit and discuss the appropriate default
naming convention at that time.
>
> > };
> >
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index
> > b70a120e62..7f11371f02 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -27,6 +27,7 @@
> > #include "system/reset.h"
> > #include "hw/loader.h"
> > #include "qemu/error-report.h"
> > +#include "qemu/datadir.h"
> > #include "qemu/units.h"
> > #include "hw/qdev-clock.h"
> > #include "system/system.h"
> > @@ -305,6 +306,34 @@ static void
> aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
> > rom_size, &error_abort);
> > }
> >
> > +/*
> > + * This function locates the vbootrom image file specified via the
> > +command line
> > + * using the -bios option. It loads the specified image into the
> > +vbootrom
> > + * memory region and handles errors if the file cannot be found or loaded.
> > + */
> > +static void aspeed_load_vbootrom(MachineState *machine, uint64_t
> rom_size,
> > + Error **errp) {
> > + AspeedMachineState *bmc = ASPEED_MACHINE(machine);
> > + AspeedMachineClass *amc =
> ASPEED_MACHINE_GET_CLASS(machine);
> > + const char *bios_name = machine->firmware ?:
> amc->vbootrom_name;
> > + g_autofree char *filename = NULL;
> > + AspeedSoCState *soc = bmc->soc;
> > + int ret;
> > +
> > + filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> > + if (!filename) {
> > + error_setg(errp, "Could not find vbootrom image '%s'",
> bios_name);
> > + return;
> > + }
> > +
> > + ret = load_image_mr(filename, &soc->vbootrom);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load vbootrom image '%s'",
> bios_name);
> > + return;
> > + }
> > +}
> > +
> > void aspeed_board_init_flashes(AspeedSMCState *s, const char
> *flashtype,
> > unsigned int count, int
> unit0)
> > {
> > @@ -483,6 +512,11 @@ static void aspeed_machine_init(MachineState
> *machine)
> > }
> > }
> >
> > + if (amc->vbootrom) {
>
> what about the "aspeed.boot_rom" region ? Is it ok to have both ?
>
Based on the design of aspeed_install_boot_rom, users can place their ROM code
at the top of the image-bmc, and this function will install image-bmc which
included the user's ROM IMAGE at the ASPEED_DEV_SPI_BOOT address.
For AST2600, users typically set the boot address to 0x0 and boot directly from
there.
For AST2700, we introduced a vbootrom to simulate the ROM code and the BOOTMCU
SPL (RISC-V).
We use aspeed_install_boot_rom to load the image-bmc at the FMC CS0
memory-mapped I/O address, so we set ASPEED_DEV_SPI_BOOT to 0x100000000.
We load the vbootrom image into the vbootrom memory region at address 0x0 and
start execution from there.
The guest OS first enters the vbootrom. From there, it can easily access the
flash data (image-bmc) at 0x100000000, since vbootrom itself doesn’t require
SPI/flash/emmc host drivers.
To support future eMMC booting, we also plan to install the eMMC image at the
ASPEED_DEV_SPI_BOOT address.
https://github.com/qemu/qemu/blob/master/hw/arm/aspeed.c#L477
It is fully supported to have both options. If users want to include their own
ROM code within image-bmc, they can set the program counter (PC) to
0x100000000, just like how a manual loader set it to 0x43000000 (e.g., to jump
directly to BL31).
This allows users to bypass the vbootrom if desired.
However, I believe this use case will be rare, as the vbootrom design should be
able to satisfy most, if not all, user requirements.
Thanks-Jamin
>
> Thanks,
>
> C.
>
>
>
> > + rom_size = memory_region_size(&bmc->soc->vbootrom);
> > + aspeed_load_vbootrom(machine, rom_size, &error_abort);
> > + }
> > +
> > arm_load_kernel(ARM_CPU(first_cpu), machine,
> &aspeed_board_binfo);
> > }
> >
> > @@ -1691,6 +1725,7 @@ static void
> aspeed_machine_ast2700a0_evb_class_init(ObjectClass *oc, void *data)
> > amc->uart_default = ASPEED_DEV_UART12;
> > amc->i2c_init = ast2700_evb_i2c_init;
> > amc->vbootrom = true;
> > + amc->vbootrom_name = "ast27x0_bootrom.bin";
> > mc->auto_create_sdcard = true;
> > mc->default_ram_size = 1 * GiB;
> > aspeed_machine_class_init_cpus_defaults(mc);
> > @@ -1712,6 +1747,7 @@ static void
> aspeed_machine_ast2700a1_evb_class_init(ObjectClass *oc, void *data)
> > amc->uart_default = ASPEED_DEV_UART12;
> > amc->i2c_init = ast2700_evb_i2c_init;
> > amc->vbootrom = true;
> > + amc->vbootrom_name = "ast27x0_bootrom.bin";
> > mc->auto_create_sdcard = true;
> > mc->default_ram_size = 1 * GiB;
> > aspeed_machine_class_init_cpus_defaults(mc);
