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 <gr...@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
signature.asc
Description: PGP signature
