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

[David Gibson]

On Tue, Aug 16, 2022 at 02:34:18PM -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.

So, there's a third place where fdt_blob is updated, in h_update_dt();
that happens because SLOF can make some updates to the DT that qemu
needs to be aware of.  It's kinda ugly, and is a consequence of the
fact that qemu and SLOF kind of share the role of "platform firmware"
for spapr.

But.. it's worse than that.  Those are the only 3 places we actually
alter fdt_blob, but not the only places we logically update the device
tree.  Up until now there wasn't a way to introspect the DT, and so we
didn't bother keeping spapr->fdt_blob update.  Essentially, we
considered maintaining the DT image the job of the guest after CAS.

Specifically, every dynamic reconfiguration event (hotplug or unplug)
alters the device tree.  We generate an fdt fragment for the new
device then stream that as an update to the guest using the PAPR
specified interface (rtas_ibm_configure_connector).  As noted we
currently don't update qemu's global fdt image based on that.  On hot
unplug logically we need to revert those changes, which is actually
pretty tricky, but currently the guest's job.


Really, the trouble is that just dumping or viewing the dt is only
simple in an "embedded" style environment where the fdt is generate
then spit into the guest.  In an actual open firmware environment like
spapr, the DT is logically a dynamic thing maintained by firmware -
but because "firmware"'s responsibility is split between SLOF and
RTAS/qemu, keeping track of that is pretty nasty.  For an environment
like this, the flat tree format isn't really suited either - we'd want
a dynamic representation of the tree.  We get away with flat trees for
now (barely) only because we mostly delegate the responsibility for
managing the tree to SLOF and/or the OS kernel, both of which do use
non-flat representations of the tree.

> spapr->fdt_blob is left untouched for now. To replace it with
> machine->fdt, since we're migrating spapr->fdt_blob, we would need to
> migrate machine->fdt as well. This is something that we would like to to
> do keep our code simpler but it's a work we'll do another day.
> 
> 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..7031cf964a 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' QMP/HMP 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..a53bfd76f4 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'
> +     * QMP/HMP commands.
> +     */
> +    MACHINE(spapr)->fdt = fdt;
> +
>      return H_SUCCESS;
>  }
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature