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 <mdr...@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) {
--
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
pgpfEpavIAit1.pgp
Description: PGP signature
