Re: [Qemu-devel] [RFC PATCH 06/15] spapr_pci: also use 'index' property as DRC index for PHBs

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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