Re: [PATCH for-7.2 v4 06/21] hw/ppc: set machine->fdt in sam460ex_load_device_tree()

[BALATON Zoltan]

On Fri, 26 Aug 2022, Daniel Henrique Barboza wrote:
> This will enable support for 'dumpdtb' and 'info fdt' HMP commands for
> the sam460ex machine.

This only works when booting with -kernel not when the firmware is used 
which creates its own DT. (The same is true for pegasos2 but there VOF is 
the default as the firmware is not free like for sam460ex.) After reading 
the other comments I wonder if this info fdt command is really useful or 
would it be easier to boot some simple Linux guest and inspect the device 
tree from there. The dumpdtb command might be simple enough and a bit more 
useful for debugging before the guest boots but that alone could be enough 
as external tools can be used to decode the binary dump. The info fdt 
might be too complex and an overkill if it might not even work or give 
correct results. But I don't mind either way and not against adding it 
just noted the possible shortcoming here.

(In case you do another iteration I wouldn't mind if the comment could be 
shortened to one line instead of 4 but it's not critical. Something like:

/* Set machine->fdt for dumpdtb and info fdt QMP/HMP commands */

would be enough and use less space. The current one is unnecessarily 
verbose for a simple line.)

Regards,
BALATON Zoltan

> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/sam460ex.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 0357ee077f..413a425d37 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -138,6 +138,7 @@ static int sam460ex_load_device_tree(hwaddr addr,
>                                      hwaddr initrd_size,
>                                      const char *kernel_cmdline)
> {
> +    MachineState *machine = MACHINE(qdev_get_machine());
>     uint32_t mem_reg_property[] = { 0, 0, cpu_to_be32(ramsize) };
>     char *filename;
>     int fdt_size;
> @@ -209,7 +210,12 @@ static int sam460ex_load_device_tree(hwaddr addr,
>                               EBC_FREQ);
>
>     rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
> -    g_free(fdt);
> +
> +    /*
> +     * Update the machine->fdt pointer to enable support for
> +     * 'dumpdtb' and 'info fdt' QMP/HMP commands.
> +     */
> +    machine->fdt = fdt;
>
>     return fdt_size;
> }