Re: [PATCH] spapr: Fix DR properties of the root node

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5460 bytes --]

On Mon, Dec 14, 2020 at 12:19:55PM +0100, Greg Kurz wrote:
> Section 13.5.2 of LoPAPR mandates various DR related indentifiers
> for all hot-pluggable entities to be exposed in the "ibm,drc-indexes",
> "ibm,drc-power-domains", "ibm,drc-names" and "ibm,drc-types" properties
> of their parent node. These properties are created with spapr_dt_drc().
> 
> PHBs and LMBs are both children of the machine. Their DR identifiers
> are thus supposed to be exposed in the afore mentioned properties of
> the root node.
> 
> When PHB hot-plug support was added, an extra call to spapr_dt_drc()
> was introduced: this overwrites the existing properties, previously
> populated with the LMB identifiers, and they end up containing only
> PHB identifiers. This went unseen so far because linux doesn't care,
> but this is still not conformant with LoPAPR.
> 
> Fortunately spapr_dt_drc() is able to handle multiple DR entity types
> at the same time. Use that to handle DR indentifiers for PHBs and LMBs
> with a single call to spapr_dt_drc(). While here also account for PMEM
> DR identifiers, which were forgotten when NVDIMM hot-plug support was
> added. Also add an assert to prevent further misuse of spapr_dt_drc().
> 
> With -m 1G,maxmem=2G,slots=8 passed on the QEMU command line we get:
> 
> Without this patch:
> 
> /proc/device-tree/ibm,drc-indexes
> 		 0000001f 20000001 20000002 20000003
> 		 20000000 20000005 20000006 20000007
> 		 20000004 20000009 20000008 20000010
> 		 20000011 20000012 20000013 20000014
> 		 20000015 20000016 20000017 20000018
> 		 20000019 2000000a 2000000b 2000000c
> 		 2000000d 2000000e 2000000f 2000001a
> 		 2000001b 2000001c 2000001d 2000001e
> 
> These are the DRC indexes for the 31 possible PHBs.
> 
> With this patch:
> 
> /proc/device-tree/ibm,drc-indexes
> 		 0000002b 90000000 90000001 90000002
> 		 90000003 90000004 90000005 90000006
> 		 90000007 20000001 20000002 20000003
> 		 20000000 20000005 20000006 20000007
> 		 20000004 20000009 20000008 20000010
> 		 20000011 20000012 20000013 20000014
> 		 20000015 20000016 20000017 20000018
> 		 20000019 2000000a 2000000b 2000000c
> 		 2000000d 2000000e 2000000f 2000001a
> 		 2000001b 2000001c 2000001d 2000001e
> 		 80000004 80000005 80000006 80000007
> 
> And now we also have the 4 ((2G - 1G) / 256M) LMBs and the
> 8 (slots) PMEMs.
> 
> Fixes: 3998ccd09298 ("spapr: populate PHB DRC entries for root DT node")
> Signed-off-by: Greg Kurz <groug@kaod.org>

Oops, good catch.  Applied to ppc-for-6.0.

> ---
>  hw/ppc/spapr.c     |   21 ++++++++++++---------
>  hw/ppc/spapr_drc.c |    6 ++++++
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 16d42ba7a937..b2f9896c8bed 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1119,6 +1119,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>      MachineState *machine = MACHINE(spapr);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> +    uint32_t root_drc_type_mask = 0;
>      int ret;
>      void *fdt;
>      SpaprPhbState *phb;
> @@ -1193,8 +1194,18 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>  
>      spapr_dt_cpus(fdt, spapr);
>  
> +    /* ibm,drc-indexes and friends */
>      if (smc->dr_lmb_enabled) {
> -        _FDT(spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
> +        root_drc_type_mask |= SPAPR_DR_CONNECTOR_TYPE_LMB;
> +    }
> +    if (smc->dr_phb_enabled) {
> +        root_drc_type_mask |= SPAPR_DR_CONNECTOR_TYPE_PHB;
> +    }
> +    if (mc->nvdimm_supported) {
> +        root_drc_type_mask |= SPAPR_DR_CONNECTOR_TYPE_PMEM;
> +    }
> +    if (root_drc_type_mask) {
> +        _FDT(spapr_dt_drc(fdt, 0, NULL, root_drc_type_mask));
>      }
>  
>      if (mc->has_hotpluggable_cpus) {
> @@ -1232,14 +1243,6 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>          }
>      }
>  
> -    if (smc->dr_phb_enabled) {
> -        ret = spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_PHB);
> -        if (ret < 0) {
> -            error_report("Couldn't set up PHB DR device tree properties");
> -            exit(1);
> -        }
> -    }
> -
>      /* NVDIMM devices */
>      if (mc->nvdimm_supported) {
>          spapr_dt_persistent_memory(spapr, fdt);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index f991cf89a08a..fc7e321fcdf6 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -832,6 +832,12 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
>      GString *drc_names, *drc_types;
>      int ret;
>  
> +    /*
> +     * This should really be only called once per node since it overwrites
> +     * the OF properties if they already exist.
> +     */
> +    g_assert(!fdt_get_property(fdt, offset, "ibm,drc-indexes", NULL));
> +
>      /* the first entry of each properties is a 32-bit integer encoding
>       * the number of elements in the array. we won't know this until
>       * we complete the iteration through all the matching DRCs, but
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]