| Message ID | 20230221061204.1658306-1-bmeng@tinylab.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/riscv: Skip re-generating DT nodes for a given DTB | expand |
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) {
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
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) {
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()") Signed-off-by: Bin Meng <bmeng@tinylab.org> --- hw/riscv/sifive_u.c | 1 + hw/riscv/virt.c | 1 + 2 files changed, 2 insertions(+)