Re: [PATCH for-7.2 v2 10/20] hw/ppc: set machine->fdt in spapr machine

[Daniel Henrique Barboza]

On 8/18/22 23:11, Alexey Kardashevskiy wrote:
> On 05/08/2022 19:39, Daniel Henrique Barboza wrote:
> > The pSeries machine never bothered with the common machine->fdt
> > attribute. We do all the FDT related work using spapr->fdt_blob.
> >
> > We're going to introduce HMP commands to read and save the FDT, which
> > will rely on setting machine->fdt properly to work across all machine
> > archs/types.
>
>
> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property 
> enough?

I tried to do the minimal changes needed for the commands to work. ms::fdt is
one of the few MachineState fields that hasn't been QOMified by
machine_class_init() yet. All pre-existing code that uses ms::fdt are using the
pointer directly. To make a QOMified use of it would require extra patches
in machine.c to QOMify the property first.

There's also the issue with how each machine is creating the FDT. Most are using
helpers from device_tree.c, some are creating it from scratch, others required
a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify
the use of ms::fdt we would need some machine hooks that standardize all that.
I believe it's worth the trouble, but it would be too much to do right now.


Thanks,



Daniel

> Another thing is that on every HMP dump I'd probably rebuild the entire FDT for 
> the reasons David explained. Thanks,
>
>
> > Let's set machine->fdt in the two places where we manipulate the FDT:
> > spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what
> > we want is a way to access the FDT from HMP, not replace
> > spapr->fdt_blob.
> >
> > Cc: Cédric Le Goater <clg@kaod.org>
> > Cc: qemu-ppc@nongnu.org
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >   hw/ppc/spapr.c       | 6 ++++++
> >   hw/ppc/spapr_hcall.c | 8 ++++++++
> >   2 files changed, 14 insertions(+)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index bc9ba6e6dc..94c90f0351 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1713,6 +1713,12 @@ static void spapr_machine_reset(MachineState *machine)
> >       spapr->fdt_initial_size = spapr->fdt_size;
> >       spapr->fdt_blob = fdt;
> > +    /*
> > +     * Set the common machine->fdt pointer to enable support
> > +     * for 'dumpdtb' and 'info fdt' commands.
> > +     */
> > +    machine->fdt = fdt;
> > +
> >       /* Set up the entry state */
> >       first_ppc_cpu->env.gpr[5] = 0;
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index a8d4a6bcf0..0079bc6fdc 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1256,6 +1256,14 @@ target_ulong do_client_architecture_support(PowerPCCPU 
> > *cpu,
> >       spapr->fdt_initial_size = spapr->fdt_size;
> >       spapr->fdt_blob = fdt;
> > +    /*
> > +     * Set the machine->fdt pointer again since we just freed
> > +     * it above (by freeing spapr->fdt_blob). We set this
> > +     * pointer to enable support for 'dumpdtb' and 'info fdt'
> > +     * HMP commands.
> > +     */
> > +    MACHINE(spapr)->fdt = fdt;
> > +
> >       return H_SUCCESS;
> >   }