On Sat, Nov 11, 2023 at 3:26 AM Daniel Henrique Barboza
<dbarb...@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 <dbarb...@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
>
>
