| Message ID | 20250225005446.13894-5-sebastian.huber@embedded-brains.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve Microchip Polarfire SoC customization | expand |
On Tue, Feb 25, 2025 at 10:55 AM Sebastian Huber <sebastian.huber@embedded-brains.de> wrote: > > Further customize the -bios and -kernel options behaviour for the > microchip-icicle-kit machine. If "-bios none -kernel filename" is > specified, then do not load a firmware and instead only load and start > the kernel image. > > Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/riscv/microchip_pfsoc.c | 57 ++++++++++++++++++++++++++------------ > 1 file changed, 40 insertions(+), 17 deletions(-) > > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c > index 844dc0545c..df902c8667 100644 > --- a/hw/riscv/microchip_pfsoc.c > +++ b/hw/riscv/microchip_pfsoc.c > @@ -578,29 +578,45 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) > } > > /* > - * We follow the following table to select which payload we execute. > + * We follow the following table to select which firmware we use. > * > - * -bios | -kernel | payload > - * -------+------------+-------- > - * N | N | HSS > - * Y | don't care | HSS > - * N | Y | kernel > - * > - * This ensures backwards compatibility with how we used to expose -bios > - * to users but allows them to run through direct kernel booting as well. > + * -bios | -kernel | firmware > + * --------------+------------+-------- > + * none | N | error > + * none | Y | kernel > + * NULL, default | N | BIOS_FILENAME > + * NULL, default | Y | RISCV64_BIOS_BIN > + * other | don't care | other > */ > + if (machine->firmware && !strcmp(machine->firmware, "none")) { > + if (!machine->kernel_filename) { > + error_report("for -bios none, a kernel is required"); > + exit(1); > + } > > - if (machine->kernel_filename) { > - firmware_name = RISCV64_BIOS_BIN; > - firmware_load_addr = memmap[MICROCHIP_PFSOC_DRAM_LO].base; > + firmware_name = NULL; > + firmware_load_addr = RESET_VECTOR; > + } else if (!machine->firmware || !strcmp(machine->firmware, "default")) { > + if (machine->kernel_filename) { > + firmware_name = RISCV64_BIOS_BIN; > + firmware_load_addr = memmap[MICROCHIP_PFSOC_DRAM_LO].base; > + } else { > + firmware_name = BIOS_FILENAME; > + firmware_load_addr = RESET_VECTOR; > + } > } else { > - firmware_name = BIOS_FILENAME; > + firmware_name = machine->firmware; > firmware_load_addr = RESET_VECTOR; > } > > - /* Load the firmware */ > - firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name, > - &firmware_load_addr, NULL); > + /* Load the firmware if necessary */ > + if (firmware_name) { > + const char *filename = riscv_find_firmware(firmware_name, NULL); > + firmware_end_addr = riscv_load_firmware(filename, &firmware_load_addr, > + NULL); > + } else { > + firmware_end_addr = firmware_load_addr; > + } > > riscv_boot_info_init(&boot_info, &s->soc.u_cpus); > if (machine->kernel_filename) { > @@ -638,8 +654,15 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) > fdt_load_addr = 0; > } > > + hwaddr start_addr; > + if (firmware_name) { > + start_addr = firmware_load_addr; > + } else { > + start_addr = kernel_entry; > + } > + > /* Load the reset vector */ > - riscv_setup_rom_reset_vec(machine, &s->soc.u_cpus, firmware_load_addr, > + riscv_setup_rom_reset_vec(machine, &s->soc.u_cpus, start_addr, > memmap[MICROCHIP_PFSOC_ENVM_DATA].base, > memmap[MICROCHIP_PFSOC_ENVM_DATA].size, > kernel_entry, fdt_load_addr); > -- > 2.43.0 >
Hi, On 2/24/25 9:54 PM, Sebastian Huber wrote: > Further customize the -bios and -kernel options behaviour for the > microchip-icicle-kit machine. If "-bios none -kernel filename" is > specified, then do not load a firmware and instead only load and start > the kernel image. > > Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> > --- > hw/riscv/microchip_pfsoc.c | 57 ++++++++++++++++++++++++++------------ > 1 file changed, 40 insertions(+), 17 deletions(-) > > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c > index 844dc0545c..df902c8667 100644 > --- a/hw/riscv/microchip_pfsoc.c > +++ b/hw/riscv/microchip_pfsoc.c > @@ -578,29 +578,45 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) > } > > /* > - * We follow the following table to select which payload we execute. > + * We follow the following table to select which firmware we use. > * > - * -bios | -kernel | payload > - * -------+------------+-------- > - * N | N | HSS > - * Y | don't care | HSS > - * N | Y | kernel > - * > - * This ensures backwards compatibility with how we used to expose -bios > - * to users but allows them to run through direct kernel booting as well. > + * -bios | -kernel | firmware > + * --------------+------------+-------- > + * none | N | error > + * none | Y | kernel > + * NULL, default | N | BIOS_FILENAME This change is breaking the following test: --------- $ QTEST_QEMU_BINARY=./build/qemu-system-riscv64 ./build/tests/qtest/qom-test (...) # slow test /riscv64/qom/amd-microblaze-v-generic executed in 2.28 secs # starting QEMU: exec ./build/qemu-system-riscv64 -qtest unix:/tmp/qtest-1361875.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1361875.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine microchip-icicle-kit -accel qtest ** ERROR:../hw/riscv/boot.c:164:riscv_load_firmware: assertion failed: (firmware_filename != NULL) Bail out! ERROR:../hw/riscv/boot.c:164:riscv_load_firmware: assertion failed: (firmware_filename != NULL) Broken pipe --------- The reason is that, with the default machine settings (no -bios and no -kernel options), firmware_name is now defaulted to BIOS_FILENAME (hss.bin). But we're not distributing 'hss.bin' in pc-bios: $ ls pc-bios/ | grep hss $ Then, in the following code, 'filename' will be NULL and riscv_load_firmware() will g_assert(): > + if (firmware_name) { > + const char *filename = riscv_find_firmware(firmware_name, NULL); > + firmware_end_addr = riscv_load_firmware(filename, &firmware_load_addr, > + NULL); > + } Possible solutions: - package hss.bin in QEMU so it can be used as a default firmware; - redo the logic to allow the board to run (even if inactive) with absent -bios and -kernel to allow QEMU model tests to run. Thanks, Daniel > + * NULL, default | Y | RISCV64_BIOS_BIN > + * other | don't care | other > */ > + if (machine->firmware && !strcmp(machine->firmware, "none")) { > + if (!machine->kernel_filename) { > + error_report("for -bios none, a kernel is required"); > + exit(1); > + } > > - if (machine->kernel_filename) { > - firmware_name = RISCV64_BIOS_BIN; > - firmware_load_addr = memmap[MICROCHIP_PFSOC_DRAM_LO].base; > + firmware_name = NULL; > + firmware_load_addr = RESET_VECTOR; > + } else if (!machine->firmware || !strcmp(machine->firmware, "default")) { > + if (machine->kernel_filename) { > + firmware_name = RISCV64_BIOS_BIN; > + firmware_load_addr = memmap[MICROCHIP_PFSOC_DRAM_LO].base; > + } else { > + firmware_name = BIOS_FILENAME; > + firmware_load_addr = RESET_VECTOR; > + } > } else { > - firmware_name = BIOS_FILENAME; > + firmware_name = machine->firmware; > firmware_load_addr = RESET_VECTOR; > } > > - /* Load the firmware */ > - firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name, > - &firmware_load_addr, NULL); > + /* Load the firmware if necessary */ > + if (firmware_name) { > + const char *filename = riscv_find_firmware(firmware_name, NULL); > + firmware_end_addr = riscv_load_firmware(filename, &firmware_load_addr, > + NULL); > + } else { > + firmware_end_addr = firmware_load_addr; > + } > > riscv_boot_info_init(&boot_info, &s->soc.u_cpus); > if (machine->kernel_filename) { > @@ -638,8 +654,15 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) > fdt_load_addr = 0; > } > > + hwaddr start_addr; > + if (firmware_name) { > + start_addr = firmware_load_addr; > + } else { > + start_addr = kernel_entry; > + } > + > /* Load the reset vector */ > - riscv_setup_rom_reset_vec(machine, &s->soc.u_cpus, firmware_load_addr, > + riscv_setup_rom_reset_vec(machine, &s->soc.u_cpus, start_addr, > memmap[MICROCHIP_PFSOC_ENVM_DATA].base, > memmap[MICROCHIP_PFSOC_ENVM_DATA].size, > kernel_entry, fdt_load_addr);
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c index 844dc0545c..df902c8667 100644 --- a/hw/riscv/microchip_pfsoc.c +++ b/hw/riscv/microchip_pfsoc.c @@ -578,29 +578,45 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) } /* - * We follow the following table to select which payload we execute. + * We follow the following table to select which firmware we use. * - * -bios | -kernel | payload - * -------+------------+-------- - * N | N | HSS - * Y | don't care | HSS - * N | Y | kernel - * - * This ensures backwards compatibility with how we used to expose -bios - * to users but allows them to run through direct kernel booting as well. + * -bios | -kernel | firmware + * --------------+------------+-------- + * none | N | error + * none | Y | kernel + * NULL, default | N | BIOS_FILENAME + * NULL, default | Y | RISCV64_BIOS_BIN + * other | don't care | other */ + if (machine->firmware && !strcmp(machine->firmware, "none")) { + if (!machine->kernel_filename) { + error_report("for -bios none, a kernel is required"); + exit(1); + } - if (machine->kernel_filename) { - firmware_name = RISCV64_BIOS_BIN; - firmware_load_addr = memmap[MICROCHIP_PFSOC_DRAM_LO].base; + firmware_name = NULL; + firmware_load_addr = RESET_VECTOR; + } else if (!machine->firmware || !strcmp(machine->firmware, "default")) { + if (machine->kernel_filename) { + firmware_name = RISCV64_BIOS_BIN; + firmware_load_addr = memmap[MICROCHIP_PFSOC_DRAM_LO].base; + } else { + firmware_name = BIOS_FILENAME; + firmware_load_addr = RESET_VECTOR; + } } else { - firmware_name = BIOS_FILENAME; + firmware_name = machine->firmware; firmware_load_addr = RESET_VECTOR; } - /* Load the firmware */ - firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name, - &firmware_load_addr, NULL); + /* Load the firmware if necessary */ + if (firmware_name) { + const char *filename = riscv_find_firmware(firmware_name, NULL); + firmware_end_addr = riscv_load_firmware(filename, &firmware_load_addr, + NULL); + } else { + firmware_end_addr = firmware_load_addr; + } riscv_boot_info_init(&boot_info, &s->soc.u_cpus); if (machine->kernel_filename) { @@ -638,8 +654,15 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) fdt_load_addr = 0; } + hwaddr start_addr; + if (firmware_name) { + start_addr = firmware_load_addr; + } else { + start_addr = kernel_entry; + } + /* Load the reset vector */ - riscv_setup_rom_reset_vec(machine, &s->soc.u_cpus, firmware_load_addr, + riscv_setup_rom_reset_vec(machine, &s->soc.u_cpus, start_addr, memmap[MICROCHIP_PFSOC_ENVM_DATA].base, memmap[MICROCHIP_PFSOC_ENVM_DATA].size, kernel_entry, fdt_load_addr);
Further customize the -bios and -kernel options behaviour for the microchip-icicle-kit machine. If "-bios none -kernel filename" is specified, then do not load a firmware and instead only load and start the kernel image. Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> --- hw/riscv/microchip_pfsoc.c | 57 ++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 17 deletions(-)