Re: [PATCH v2 07/10] hw/arm/aspeed: Add support for loading vbootrom image via "-bios"

[Nabih Estefan <nabihestefan@google.com>]

Hi Jamin,


On Tue, Apr 15, 2025 at 2:35 AM Jamin Lin <jamin_lin@aspeedtech.com> wrote:
>
> Hi Nabih,
>
> > <qemu-arm@nongnu.org>; Troy Lee <troy_lee@aspeedtech.com>
> > Subject: Re: [PATCH v2 07/10] hw/arm/aspeed: Add support for loading
> > vbootrom image via "-bios"
> >
> > Hi Jamin and Cedric,
> >
> > On Sun, Apr 13, 2025 at 8:17 PM Jamin Lin <jamin_lin@aspeedtech.com>
> > wrote:
> > >
> > > Hi Cedric,
> > >
> > > > Subject: Re: [PATCH v2 07/10] hw/arm/aspeed: Add support for loading
> > > > vbootrom image via "-bios"
> > > >
> > > > On 4/10/25 04:38, 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 <jamin_lin@aspeedtech.com>
> > > > > ---
> > > > >   hw/arm/aspeed.c | 32 ++++++++++++++++++++++++++++++++
> > > > >   1 file changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index
> > > > > b70a120e62..2811868c1a 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,32 @@ 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)
> > > >
> > > > please add an 'Error **' parameter and let the caller decide to exit.
> > > >
> > >
> > > Will add.
> > >
> > > > > +{
> > > > > +    AspeedMachineState *bmc = ASPEED_MACHINE(machine);
> > > > > +    const char *bios_name = machine->firmware;
> > > > > +    g_autofree char *filename = NULL;
> > > > > +    AspeedSoCState *soc = bmc->soc;
> > > > > +    int ret;
> > > > > +
> > > > > +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> > > >
> > > > What if the user didn't provide any -bios command line option ?
> > > >
> > >
> > >
> > > Will update to support both vbootrom and loader.
> >
> > For this case, could we have something like the npcm8xx_board.c where we
> > have a default bootrom and override it with -bios? That would also fix the qtest
> > issues with the ast2700 qtests which fail with this patchset.
> >
> Do you mean that if the user does not specify "-bios", we should still load a default vbootrom image into the vbootrom memory region?
> We can verify whether -bios was provided by checking if machine->firmware is NULL.
> It seems that if the user doesn't provide -bios, NPCM QEMU will look for a default vbootrom image in the current working directory — is that correct?
> https://github.com/qemu/qemu/blob/master/hw/arm/npcm8xx_boards.c#L37

Yeah. IIUC the default functionality should be to use a vbootrom to
boot with the AST27X0 so
if there is no bootrom declared we should default to the one you
created. for the NPCM one, if
we don't provide bios it will default to the one in pc-bios since it's
supposed to be the base true version.
I think it makes sense to do the same thing in ast2700. If we really
find use in supporting attaching the
loader binaries separately we could add a flag that skip the vbootrom,
but I don't see much use in that
since the information that would change is in the "image-bmc".

>
> ```
> const char *bios_name = machine->firmware ?: npcm8xx_default_bootrom;
> ```
> Could you let me know which qtest(s) failed, so I can run them and verify everything before sending out the v3 patch?

qemu:qtest+qtest-aarch64 / qtest-aarch64/ast2700-gpio-test
qemu:qtest+qtest-aarch64 / qtest-aarch64/ast2700-smc-test

They both fail with " qemu-system-aarch64: Could not find vbootrom
image '(null)' " so setting the default
bootrom should fix this.

>
> Thanks-Jamin
>
> > >
> > > > > +    if (!filename) {
> > > > > +        error_report("Could not find vbootrom image '%s'",
> > bios_name);
> > > > > +        exit(1);
> > > > > +    }
> > > > > +
> > > > > +    ret = load_image_mr(filename, &soc->vbootrom);
> > > > > +    if (ret < 0) {
> > > > > +        error_report("Failed to load vbootrom image '%s'", filename);
> > > > > +        exit(1);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > >   void aspeed_board_init_flashes(AspeedSMCState *s, const char
> > > > *flashtype,
> > > > >                                         unsigned int count, int
> > > > unit0)
> > > > >   {
> > > > > @@ -483,6 +510,11 @@ static void aspeed_machine_init(MachineState
> > > > *machine)
> > > > >           }
> > > > >       }
> > > > >
> > > > > +    if (amc->vbootrom) {
> > > > > +        rom_size = memory_region_size(&bmc->soc->vbootrom);> +
> > > > aspeed_load_vbootrom(machine, rom_size);
> > > > > +    }
> > > > > +
> > > >
> > > > Even without a vbootrom file, the machine could boot with '-device loader'
> > > > options. We should preserve this way of booting an ast2700-evb machine.
> > > >
> > >
> > > Will support both loader and vbootrom.
> > > Thanks for review and suggestion.
> > >
> > > Jamin
> > > >
> > > > Thanks,
> > > >
> > > > C.
> > > >
> > > >
> > > >
> > > >
> > > > >       arm_load_kernel(ARM_CPU(first_cpu), machine,
> > > > &aspeed_board_binfo);
> > > > >   }
> > > > >
> > >
> >
> > Also, tested against our custom machine + custom bmc image and the bootrom
> > itself works.
> > I think it might just need that default set.
> >
> > Tested-By: Nabih Estefan <nabihestefan@google.com>

Thanks,
Nabih