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

[Daniel Henrique Barboza]

On 8/8/22 00:26, David Gibson wrote:
> On Fri, Aug 05, 2022 at 06:39:38AM -0300, 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.
> >
> > 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.
>
> Given there is now an fdt field in the generic MACHINE structure, we
> should be able to remove the one in spapr->fdt_blob, yes?

I thought about it but I backed down when I realized that spapr->fdt_blob is
being migrated.

At first glance it would be a matter of migrating ms->fdt and then removing
spapr->fdt_blob, but then I got confused about how a migration to an older
version would occur (e.g. QEMU 7.2 with ms->fdt to a QEMU 7.0 with
spapr->fdt_blob).

Migration to a newer QEMU would require us to move the spapr->version_id to 4
and then handle the old version accordingly in spapr_post_load().

Probably something to think about after this work is accepted.


Thanks,


Daniel



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