Re: [PATCH for-8.2] hw/riscv/virt.c: do create_fdt() earlier, add finalize_fdt()

[Alistair Francis]

On Sat, Nov 11, 2023 at 3:26 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Commit 49554856f0 fixed a problem, where TPM devices were not appearing
> in the FDT, by delaying the FDT creation up until virt_machine_done().
> This create a side effect (see gitlab #1925) - devices that need access
> to the '/chosen' FDT node during realize() stopped working because, at
> that point, we don't have a FDT.
>
> This happens because our FDT creation is monolithic, but it doesn't need
> to be. We can add the needed FDT components for realize() time and, at
> the same time, do another FDT round where we account for dynamic sysbus
> devices.  In other words, the problem fixed by 49554856f0 could also be
> fixed by postponing only create_fdt_sockets() and its dependencies,
> leaving everything else from create_fdt() to be done during init().
>
> Split the FDT creation in two parts:
>
> - create_fdt(), now moved back to virt_machine_init(), will create FDT
>   nodes that doesn't depend on additional (dynamic) devices from the
>   sysbus;
>
> - a new finalize_fdt() step is added, where create_fdt_sockets() and
>   friends is executed, accounting for the dynamic sysbus devices that
>   were added during realize().
>
> This will make both use cases happy: TPM devices are still working as
> intended, and devices such as 'guest-loader' have a FDT to work on
> during realize().
>
> Fixes: 49554856f0 ("riscv: Generate devicetree only after machine 
> initialization is complete")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1925
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  hw/riscv/virt.c | 71 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index c7fc97e273..d2eac24156 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -962,7 +962,6 @@ static void create_fdt_uart(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>          qemu_fdt_setprop_cells(ms->fdt, name, "interrupts", UART0_IRQ, 0x4);
>      }
>
> -    qemu_fdt_add_subnode(ms->fdt, "/chosen");
>      qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", name);
>      g_free(name);
>  }
> @@ -1023,11 +1022,29 @@ static void create_fdt_fw_cfg(RISCVVirtState *s, 
> const MemMapEntry *memmap)
>      g_free(nodename);
>  }
>
> -static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
> +static void finalize_fdt(RISCVVirtState *s)
>  {
> -    MachineState *ms = MACHINE(s);
>      uint32_t phandle = 1, irq_mmio_phandle = 1, msi_pcie_phandle = 1;
>      uint32_t irq_pcie_phandle = 1, irq_virtio_phandle = 1;
> +
> +    create_fdt_sockets(s, virt_memmap, &phandle, &irq_mmio_phandle,
> +                       &irq_pcie_phandle, &irq_virtio_phandle,
> +                       &msi_pcie_phandle);
> +
> +    create_fdt_virtio(s, virt_memmap, irq_virtio_phandle);
> +
> +    create_fdt_pcie(s, virt_memmap, irq_pcie_phandle, msi_pcie_phandle);
> +
> +    create_fdt_reset(s, virt_memmap, &phandle);
> +
> +    create_fdt_uart(s, virt_memmap, irq_mmio_phandle);
> +
> +    create_fdt_rtc(s, virt_memmap, irq_mmio_phandle);
> +}
> +
> +static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
> +{
> +    MachineState *ms = MACHINE(s);
>      uint8_t rng_seed[32];
>
>      ms->fdt = create_device_tree(&s->fdt_size);
> @@ -1047,28 +1064,16 @@ static void create_fdt(RISCVVirtState *s, const 
> MemMapEntry *memmap)
>      qemu_fdt_setprop_cell(ms->fdt, "/soc", "#size-cells", 0x2);
>      qemu_fdt_setprop_cell(ms->fdt, "/soc", "#address-cells", 0x2);
>
> -    create_fdt_sockets(s, memmap, &phandle, &irq_mmio_phandle,
> -                       &irq_pcie_phandle, &irq_virtio_phandle,
> -                       &msi_pcie_phandle);
> -
> -    create_fdt_virtio(s, memmap, irq_virtio_phandle);
> -
> -    create_fdt_pcie(s, memmap, irq_pcie_phandle, msi_pcie_phandle);
> -
> -    create_fdt_reset(s, memmap, &phandle);
> -
> -    create_fdt_uart(s, memmap, irq_mmio_phandle);
> -
> -    create_fdt_rtc(s, memmap, irq_mmio_phandle);
> -
> -    create_fdt_flash(s, memmap);
> -    create_fdt_fw_cfg(s, memmap);
> -    create_fdt_pmu(s);
> +    qemu_fdt_add_subnode(ms->fdt, "/chosen");
>
>      /* Pass seed to RNG */
>      qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
>      qemu_fdt_setprop(ms->fdt, "/chosen", "rng-seed",
>                       rng_seed, sizeof(rng_seed));
> +
> +    create_fdt_flash(s, memmap);
> +    create_fdt_fw_cfg(s, memmap);
> +    create_fdt_pmu(s);
>  }
>
>  static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
> @@ -1257,15 +1262,12 @@ static void virt_machine_done(Notifier *notifier, 
> void *data)
>      uint64_t kernel_entry = 0;
>      BlockBackend *pflash_blk0;
>
> -    /* load/create device tree */
> -    if (machine->dtb) {
> -        machine->fdt = load_device_tree(machine->dtb, &s->fdt_size);
> -        if (!machine->fdt) {
> -            error_report("load_device_tree() failed");
> -            exit(1);
> -        }
> -    } else {
> -        create_fdt(s, memmap);
> +    /*
> +     * An user provided dtb must include everything, including
> +     * dynamic sysbus devices. Our FDT needs to be finalized.
> +     */
> +    if (machine->dtb == NULL) {
> +        finalize_fdt(s);
>      }
>
>      /*
> @@ -1541,6 +1543,17 @@ static void virt_machine_init(MachineState *machine)
>      }
>      virt_flash_map(s, system_memory);
>
> +    /* load/create device tree */
> +    if (machine->dtb) {
> +        machine->fdt = load_device_tree(machine->dtb, &s->fdt_size);
> +        if (!machine->fdt) {
> +            error_report("load_device_tree() failed");
> +            exit(1);
> +        }
> +    } else {
> +        create_fdt(s, memmap);
> +    }
> +
>      s->machine_done.notify = virt_machine_done;
>      qemu_add_machine_init_done_notifier(&s->machine_done);
>  }
> --
> 2.41.0
>
>