Re: [PATCH 1/4] hw/ppc: Set machine->fdt in e500 machines

[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.

+ 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);