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

[David Gibson]

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 <address@hidden>

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

pgp6VnSHfvg3N.pgp
Description: PGP signature