Re: [PATCH] hw/riscv: Skip re-generating DT nodes for a given DTB

[Daniel Henrique Barboza]

On 2/21/23 03:12, Bin Meng wrote:
> Lanuch qemu-system-riscv64 with a given dtb for 'sifive_u' and 'virt'
> machines, QEMU complains:
>
>    qemu_fdt_add_subnode: Failed to create subnode /soc: FDT_ERR_EXISTS
>
> The whole DT generation logic should be skipped when a given DTB is
> present.
>
> Fixes: b1f19f238cae ("hw/riscv: write bootargs 'chosen' FDT after 
> riscv_load_kernel()")

Thanks for cleaning my mess :)

I was wondering whether we should move the ms->dtb verification/load bits 
outside of
create_fdt(), and put it explicitly in sifive_u_machine_init() and 
virt_machine_init().
Like this:

    /* load/create device tree*/
    if (ms->dtb) {
        ms->fdt = load_device_tree(ms->dtb, &s->fdt_size);
        if (!ms->fdt) {
            error_report("load_device_tree() failed");
            exit(1);
        }
    } else {
        create_fdt(s, memmap);
    }


This looks clearer to me because create_fdt() will actually create a fdt, not 
load or create
a fdt. create_fdt() from spike works this way.

I'll leave to your discretion. The patch is already good enough as is.


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   hw/riscv/sifive_u.c | 1 +
>   hw/riscv/virt.c     | 1 +
>   2 files changed, 2 insertions(+)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index ad3bb35b34..76db5ed3dd 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -118,6 +118,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry 
> *memmap,
>               error_report("load_device_tree() failed");
>               exit(1);
>           }
> +        return;
>       } else {
>           fdt = ms->fdt = create_device_tree(&fdt_size);
>           if (!fdt) {
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 86c4adc0c9..0c7b4a1e46 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1014,6 +1014,7 @@ static void create_fdt(RISCVVirtState *s, const 
> MemMapEntry *memmap)
>               error_report("load_device_tree() failed");
>               exit(1);
>           }
> +        return;
>       } else {
>           ms->fdt = create_device_tree(&s->fdt_size);
>           if (!ms->fdt) {