| Message ID | 20230125130024.158721-2-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | E500 cleanups and enhancements | expand |
On 1/25/23 10:00, Bernhard Beschow wrote: > This enables support for the 'dumpdtb' QMP/HMP command for all > e500 machines. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/ppc/e500.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c > index 9fa1f8e6cf..7239993acc 100644 > --- a/hw/ppc/e500.c > +++ b/hw/ppc/e500.c > @@ -659,9 +659,14 @@ done: > if (!dry_run) { > qemu_fdt_dumpdtb(fdt, fdt_size); > cpu_physical_memory_write(addr, fdt, fdt_size); > + > + /* Set machine->fdt for 'dumpdtb' QMP/HMP command */ > + g_free(machine->fdt); > + machine->fdt = fdt; > + } else { > + g_free(fdt); > } > ret = fdt_size; > - g_free(fdt); > I tried to do this change last year when introducing 'dumpdtb' and Phil had some comments in how the FDT was being handled by the e500 board: https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg03256.html ================ + + /* + * Update the machine->fdt pointer to enable support for the + * 'dumpdtb' QMP/HMP command. + * + * The FDT is re-created during reset, Why are we doing that? Is it really necessary? This seems to be only required at cold power-on. + so free machine->fdt + * to avoid leaking the old FDT. + */ + g_free(machine->fdt); + machine->fdt = fdt; ================ I ended up not going after Phil's concern. I don't think it's required to accept this change, but it would simplify it a bit if the FDT isn't required to be re-generated on boot. I'm CCing Phil in case he wants to comment on it as well. Daniel > out: > g_free(pci_map);
Am 26. Januar 2023 15:58:18 UTC schrieb Daniel Henrique Barboza <danielhb413@gmail.com>: > > >On 1/25/23 10:00, Bernhard Beschow wrote: >> This enables support for the 'dumpdtb' QMP/HMP command for all >> e500 machines. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/ppc/e500.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >> index 9fa1f8e6cf..7239993acc 100644 >> --- a/hw/ppc/e500.c >> +++ b/hw/ppc/e500.c >> @@ -659,9 +659,14 @@ done: >> if (!dry_run) { >> qemu_fdt_dumpdtb(fdt, fdt_size); >> cpu_physical_memory_write(addr, fdt, fdt_size); >> + >> + /* Set machine->fdt for 'dumpdtb' QMP/HMP command */ >> + g_free(machine->fdt); >> + machine->fdt = fdt; >> + } else { >> + g_free(fdt); >> } >> ret = fdt_size; >> - g_free(fdt); >> > >I tried to do this change last year when introducing 'dumpdtb' and Phil had some >comments in how the FDT was being handled by the e500 board: > >https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg03256.html > > >================ > >+ >+ /* >+ * Update the machine->fdt pointer to enable support for the >+ * 'dumpdtb' QMP/HMP command. >+ * >+ * The FDT is re-created during reset, > >Why are we doing that? Is it really necessary? This seems to be only required at cold power-on. The e500 boards have user-creatable eTSEC nics which are registered with the machine via the platform bus mechanism. IIUC this mechanism causes these nics to show up only after reset. The nics need to show up in the device tree, so the reset trigger was apparently chosen to create the fdt. N.B.: The size of the fdt needs to be known during machine_init to check whether it fits into guest ram. That's what the dry_run parameter is for. Does that explanation help? Best regards, Bernhard > >+ so free machine->fdt >+ * to avoid leaking the old FDT. >+ */ >+ g_free(machine->fdt); >+ machine->fdt = fdt; >================ > >I ended up not going after Phil's concern. I don't think it's required to accept >this change, but it would simplify it a bit if the FDT isn't required to be >re-generated on boot. > > >I'm CCing Phil in case he wants to comment on it as well. > > > > >Daniel > > >> out: >> g_free(pci_map);
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 9fa1f8e6cf..7239993acc 100644 --- a/hw/ppc/e500.c +++ b/hw/ppc/e500.c @@ -659,9 +659,14 @@ done: if (!dry_run) { qemu_fdt_dumpdtb(fdt, fdt_size); cpu_physical_memory_write(addr, fdt, fdt_size); + + /* Set machine->fdt for 'dumpdtb' QMP/HMP command */ + g_free(machine->fdt); + machine->fdt = fdt; + } else { + g_free(fdt); } ret = fdt_size; - g_free(fdt); out: g_free(pci_map);
This enables support for the 'dumpdtb' QMP/HMP command for all e500 machines. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/ppc/e500.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)