* [PATCH] hw/riscv: Skip re-generating DT nodes for a given DTB
@ 2023-02-21 6:12 Bin Meng
2023-02-21 11:31 ` Daniel Henrique Barboza
0 siblings, 1 reply; 3+ messages in thread
From: Bin Meng @ 2023-02-21 6:12 UTC (permalink / raw)
To: qemu-devel
Cc: Alistair Francis, Bin Meng, Daniel Henrique Barboza, Liu Zhiwei,
Palmer Dabbelt, Weiwei Li, qemu-riscv
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(+)
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) {
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/riscv: Skip re-generating DT nodes for a given DTB
2023-02-21 6:12 [PATCH] hw/riscv: Skip re-generating DT nodes for a given DTB Bin Meng
@ 2023-02-21 11:31 ` Daniel Henrique Barboza
2023-02-27 3:09 ` Bin Meng
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Henrique Barboza @ 2023-02-21 11:31 UTC (permalink / raw)
To: Bin Meng, qemu-devel
Cc: Alistair Francis, Bin Meng, Liu Zhiwei, Palmer Dabbelt, Weiwei Li,
qemu-riscv
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) {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/riscv: Skip re-generating DT nodes for a given DTB
2023-02-21 11:31 ` Daniel Henrique Barboza
@ 2023-02-27 3:09 ` Bin Meng
0 siblings, 0 replies; 3+ messages in thread
From: Bin Meng @ 2023-02-27 3:09 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Bin Meng, qemu-devel, Alistair Francis, Bin Meng, Liu Zhiwei,
Palmer Dabbelt, Weiwei Li, qemu-riscv
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-02-27 3:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-21 6:12 [PATCH] hw/riscv: Skip re-generating DT nodes for a given DTB Bin Meng
2023-02-21 11:31 ` Daniel Henrique Barboza
2023-02-27 3:09 ` Bin Meng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).