| Message ID | 1430335224-6716-7-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 29, 2015 at 02:20:15PM -0500, Michael Roth wrote: > Prior to this patch 'index' is purely a shorthand for specifying > MMIO windows, BUIDs, and other configuration values for a PHB. > > With the addition of PHB hotplug, we have a static number of DRCs > that can be used to handle hotplug/unplug operations on our PHBs, > and need a consistent way to map PHBs to these connectors, and > assign a unique identifiers for the connectors. > > BUIDs would be a good choice, however, those are 64-bit values, > whereas DRC indexes are 32-bit. > > 'index' serves this purpose nicely, and also allows us to align > the maximum number PHBs that can be plugged with the maximum > 'index' value we allow (255). > > This means that when PHB hotplug is enabled (2.4+), 'index' is > now always a required value, regardless of whether or not other > configuration properties are specified explicitly. We could > potentially arrange for 'index'-less PHBs to be added in an > 'unpluggable' fashion via command-line, and have checks to > generate an error when hotplugged via device_add, but the simpler > path seems to be to just make it required now. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> So, if I was doing this again from scratch, I'd probably just make index mandatory. But being where we already are.. I'd prefer to add an explicit "drc_id" (or whatever) property and have that set to index by default (just as index provides defaults for the other window properties). > --- > hw/ppc/spapr_pci.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 25a738c..e37de28 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1156,8 +1156,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1) > || (sphb->mem_win_addr != (hwaddr)-1) > || (sphb->io_win_addr != (hwaddr)-1)) { > - error_setg(errp, "Either \"index\" or other parameters must" > - " be specified for PAPR PHB, not both"); > + if (!spapr->dr_phb_enabled) { > + /* if they aren't potentially using index as an identifier for > + * the PHB's DR connector, enforce the old semantics of index > + * being purely a shorthand for PHB configuration options. > + */ > + error_setg(errp, "Either \"index\" or other parameters must" > + " be specified for PAPR PHB, not both"); > + } > return; > } > > @@ -1174,6 +1180,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > + sphb->index * SPAPR_PCI_WINDOW_SPACING; > sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF; > sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF; > + } else { > + if (spapr->dr_phb_enabled) { > + error_setg(errp, "The \"index\" property is required for machine" > + " types that support PHB hotplug (and in such cases" > + " can be used alongside \"buid\" and other" > + " configuration properties)"); > + return; > + } > } > > if (sphb->buid == (uint64_t)-1) {
Quoting David Gibson (2015-05-05 06:34:15) > On Wed, Apr 29, 2015 at 02:20:15PM -0500, Michael Roth wrote: > > Prior to this patch 'index' is purely a shorthand for specifying > > MMIO windows, BUIDs, and other configuration values for a PHB. > > > > With the addition of PHB hotplug, we have a static number of DRCs > > that can be used to handle hotplug/unplug operations on our PHBs, > > and need a consistent way to map PHBs to these connectors, and > > assign a unique identifiers for the connectors. > > > > BUIDs would be a good choice, however, those are 64-bit values, > > whereas DRC indexes are 32-bit. > > > > 'index' serves this purpose nicely, and also allows us to align > > the maximum number PHBs that can be plugged with the maximum > > 'index' value we allow (255). > > > > This means that when PHB hotplug is enabled (2.4+), 'index' is > > now always a required value, regardless of whether or not other > > configuration properties are specified explicitly. We could > > potentially arrange for 'index'-less PHBs to be added in an > > 'unpluggable' fashion via command-line, and have checks to > > generate an error when hotplugged via device_add, but the simpler > > path seems to be to just make it required now. > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > So, if I was doing this again from scratch, I'd probably just make > index mandatory. But being where we already are.. > > I'd prefer to add an explicit "drc_id" (or whatever) property and have > that set to index by default (just as index provides defaults for the > other window properties). Hmm, yah that's much less of a headache. Thanks! > > > --- > > hw/ppc/spapr_pci.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 25a738c..e37de28 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1156,8 +1156,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > > if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1) > > || (sphb->mem_win_addr != (hwaddr)-1) > > || (sphb->io_win_addr != (hwaddr)-1)) { > > - error_setg(errp, "Either \"index\" or other parameters must" > > - " be specified for PAPR PHB, not both"); > > + if (!spapr->dr_phb_enabled) { > > + /* if they aren't potentially using index as an identifier for > > + * the PHB's DR connector, enforce the old semantics of index > > + * being purely a shorthand for PHB configuration options. > > + */ > > + error_setg(errp, "Either \"index\" or other parameters must" > > + " be specified for PAPR PHB, not both"); > > + } > > return; > > } > > > > @@ -1174,6 +1180,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > > + sphb->index * SPAPR_PCI_WINDOW_SPACING; > > sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF; > > sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF; > > + } else { > > + if (spapr->dr_phb_enabled) { > > + error_setg(errp, "The \"index\" property is required for machine" > > + " types that support PHB hotplug (and in such cases" > > + " can be used alongside \"buid\" and other" > > + " configuration properties)"); > > + return; > > + } > > } > > > > if (sphb->buid == (uint64_t)-1) { > > -- > 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
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 25a738c..e37de28 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1156,8 +1156,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1) || (sphb->mem_win_addr != (hwaddr)-1) || (sphb->io_win_addr != (hwaddr)-1)) { - error_setg(errp, "Either \"index\" or other parameters must" - " be specified for PAPR PHB, not both"); + if (!spapr->dr_phb_enabled) { + /* if they aren't potentially using index as an identifier for + * the PHB's DR connector, enforce the old semantics of index + * being purely a shorthand for PHB configuration options. + */ + error_setg(errp, "Either \"index\" or other parameters must" + " be specified for PAPR PHB, not both"); + } return; } @@ -1174,6 +1180,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) + sphb->index * SPAPR_PCI_WINDOW_SPACING; sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF; sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF; + } else { + if (spapr->dr_phb_enabled) { + error_setg(errp, "The \"index\" property is required for machine" + " types that support PHB hotplug (and in such cases" + " can be used alongside \"buid\" and other" + " configuration properties)"); + return; + } } if (sphb->buid == (uint64_t)-1) {
Prior to this patch 'index' is purely a shorthand for specifying MMIO windows, BUIDs, and other configuration values for a PHB. With the addition of PHB hotplug, we have a static number of DRCs that can be used to handle hotplug/unplug operations on our PHBs, and need a consistent way to map PHBs to these connectors, and assign a unique identifiers for the connectors. BUIDs would be a good choice, however, those are 64-bit values, whereas DRC indexes are 32-bit. 'index' serves this purpose nicely, and also allows us to align the maximum number PHBs that can be plugged with the maximum 'index' value we allow (255). This means that when PHB hotplug is enabled (2.4+), 'index' is now always a required value, regardless of whether or not other configuration properties are specified explicitly. We could potentially arrange for 'index'-less PHBs to be added in an 'unpluggable' fashion via command-line, and have checks to generate an error when hotplugged via device_add, but the simpler path seems to be to just make it required now. Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- hw/ppc/spapr_pci.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)