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