| Message ID | 20230117132751.229738-2-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | move create_fw_cfg() to init() (Gitlab #1343) | expand |
On Tue, Jan 17, 2023 at 11:28 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > Commit 1c20d3ff6004 ("hw/riscv: virt: Add a machine done notifier") > moved the initialization of fw_cfg to the virt_machine_done() callback. > > Problem is that the validation of fw_cfg by devices such as ramfb is > done before the machine done notifier is called. Moving create_fw_cfg() > to machine_done() results in QEMU failing to boot when using a ramfb > device: > > ./qemu-system-riscv64 -machine virt -device ramfb -serial stdio > qemu-system-riscv64: -device ramfb: ramfb device requires fw_cfg with DMA > > The fix is simple: move create_fw_cfg() config back to > virt_machine_init(). This happens to be the same way the ARM 'virt' > machine deals with fw_cfg (see machvirt_init() and virt_machine_done() > in hw/arm/virt.c), so we're keeping consistency with how other machines > handle this device. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1343 > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Thanks for the fix! Alistair > --- > hw/riscv/virt.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index e6d4f06e8d..4a11b4b010 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1254,13 +1254,6 @@ static void virt_machine_done(Notifier *notifier, void *data) > firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name, > start_addr, NULL); > > - /* > - * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the device > - * tree cannot be altered and we get FDT_ERR_NOSPACE. > - */ > - s->fw_cfg = create_fw_cfg(machine); > - rom_set_fw(s->fw_cfg); > - > if (drive_get(IF_PFLASH, 0, 1)) { > /* > * S-mode FW like EDK2 will be kept in second plash (unit 1). > @@ -1468,6 +1461,13 @@ static void virt_machine_init(MachineState *machine) > memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base, > mask_rom); > > + /* > + * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the > + * device tree cannot be altered and we get FDT_ERR_NOSPACE. > + */ > + s->fw_cfg = create_fw_cfg(machine); > + rom_set_fw(s->fw_cfg); > + > /* SiFive Test MMIO device */ > sifive_test_create(memmap[VIRT_TEST].base); > > -- > 2.39.0 > >
On Tue, Jan 17, 2023 at 11:28 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > Commit 1c20d3ff6004 ("hw/riscv: virt: Add a machine done notifier") > moved the initialization of fw_cfg to the virt_machine_done() callback. > > Problem is that the validation of fw_cfg by devices such as ramfb is > done before the machine done notifier is called. Moving create_fw_cfg() > to machine_done() results in QEMU failing to boot when using a ramfb > device: > > ./qemu-system-riscv64 -machine virt -device ramfb -serial stdio > qemu-system-riscv64: -device ramfb: ramfb device requires fw_cfg with DMA > > The fix is simple: move create_fw_cfg() config back to > virt_machine_init(). This happens to be the same way the ARM 'virt' > machine deals with fw_cfg (see machvirt_init() and virt_machine_done() > in hw/arm/virt.c), so we're keeping consistency with how other machines > handle this device. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1343 > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Thanks! Applied to riscv-to-apply.next Alistair > --- > hw/riscv/virt.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index e6d4f06e8d..4a11b4b010 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1254,13 +1254,6 @@ static void virt_machine_done(Notifier *notifier, void *data) > firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name, > start_addr, NULL); > > - /* > - * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the device > - * tree cannot be altered and we get FDT_ERR_NOSPACE. > - */ > - s->fw_cfg = create_fw_cfg(machine); > - rom_set_fw(s->fw_cfg); > - > if (drive_get(IF_PFLASH, 0, 1)) { > /* > * S-mode FW like EDK2 will be kept in second plash (unit 1). > @@ -1468,6 +1461,13 @@ static void virt_machine_init(MachineState *machine) > memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base, > mask_rom); > > + /* > + * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the > + * device tree cannot be altered and we get FDT_ERR_NOSPACE. > + */ > + s->fw_cfg = create_fw_cfg(machine); > + rom_set_fw(s->fw_cfg); > + > /* SiFive Test MMIO device */ > sifive_test_create(memmap[VIRT_TEST].base); > > -- > 2.39.0 > >
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index e6d4f06e8d..4a11b4b010 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1254,13 +1254,6 @@ static void virt_machine_done(Notifier *notifier, void *data) firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name, start_addr, NULL); - /* - * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the device - * tree cannot be altered and we get FDT_ERR_NOSPACE. - */ - s->fw_cfg = create_fw_cfg(machine); - rom_set_fw(s->fw_cfg); - if (drive_get(IF_PFLASH, 0, 1)) { /* * S-mode FW like EDK2 will be kept in second plash (unit 1). @@ -1468,6 +1461,13 @@ static void virt_machine_init(MachineState *machine) memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base, mask_rom); + /* + * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the + * device tree cannot be altered and we get FDT_ERR_NOSPACE. + */ + s->fw_cfg = create_fw_cfg(machine); + rom_set_fw(s->fw_cfg); + /* SiFive Test MMIO device */ sifive_test_create(memmap[VIRT_TEST].base);
Commit 1c20d3ff6004 ("hw/riscv: virt: Add a machine done notifier") moved the initialization of fw_cfg to the virt_machine_done() callback. Problem is that the validation of fw_cfg by devices such as ramfb is done before the machine done notifier is called. Moving create_fw_cfg() to machine_done() results in QEMU failing to boot when using a ramfb device: ./qemu-system-riscv64 -machine virt -device ramfb -serial stdio qemu-system-riscv64: -device ramfb: ramfb device requires fw_cfg with DMA The fix is simple: move create_fw_cfg() config back to virt_machine_init(). This happens to be the same way the ARM 'virt' machine deals with fw_cfg (see machvirt_init() and virt_machine_done() in hw/arm/virt.c), so we're keeping consistency with how other machines handle this device. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1343 Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- hw/riscv/virt.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)