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

[Bin Meng]

On Tue, Feb 21, 2023 at 7:32 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> 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 = d(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.

Yes, this makes sense.

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

I think we can create another patch to do the move as you suggested.
Because this patch we use a "Fixes" tag to refer to the culprit
commit, and this patch just does the minimum thing to fix that.

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

Regards,
Bin