diff mbox series

[1/1] hw/riscv/virt.c: move create_fw_cfg() back to virt_machine_init()

Message ID 20230117132751.229738-2-dbarboza@ventanamicro.com
State New
Headers show
Series move create_fw_cfg() to init() (Gitlab #1343) | expand

Commit Message

Daniel Henrique Barboza Jan. 17, 2023, 1:27 p.m. UTC
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(-)

Comments

Alistair Francis Jan. 18, 2023, 10:56 p.m. UTC | #1
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
>
>
Alistair Francis Jan. 19, 2023, 12:23 a.m. UTC | #2
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 mbox series

Patch

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);