| Message ID | 160794479566.35245.17809158217760761558.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: Fix DR properties of the root node | expand |
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 > >
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
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> --- hw/ppc/spapr.c | 21 ++++++++++++--------- hw/ppc/spapr_drc.c | 6 ++++++ 2 files changed, 18 insertions(+), 9 deletions(-)