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

[Daniel Henrique Barboza]

On 8/26/22 15:56, BALATON Zoltan wrote:
> 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.

As an counter example of what you said, there's no guarantee that the FDT is
fully exposed in the guest sysfs. This is the case of the /chosen/rng-seed
property that was added recently in the pSeries machine - the property exists
in the FDT, the guest is aware of it, but the guest doesn't show it in under
/proc/device-tree. In fact, I think I mentioned in the cover letter of the first
version that this situation was the motivation of this work.

But you're right when you questioned how useful these QMP/HMP commands might be
for sam460ex. The utility of both commands relies on how the machine handles the
FDT internally and what you're trying to check.


> (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 */


Changed in my local tree.


Thanks,

Daniel

> 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;
> > }