| Message ID | 20250410023856.500258-8-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support vbootrom for AST2700 | expand |
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. > +{ > + 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 ? > + 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. Thanks, C. > arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo); > } >
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. > > + 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); > > } > >
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. > > > > + 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>
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 ``` 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? 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>
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
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) +{ + 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); + 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); + } + arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo); }
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(+)