From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35171) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpfB4-0004Dd-AG for qemu-devel@nongnu.org; Tue, 05 May 2015 11:54:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YpfAy-0001MZ-6d for qemu-devel@nongnu.org; Tue, 05 May 2015 11:54:34 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:53082) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpfAx-0001MJ-Vx for qemu-devel@nongnu.org; Tue, 05 May 2015 11:54:28 -0400 Received: from /spool/local by e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 5 May 2015 09:54:27 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20150505113415.GA14090@voom.redhat.com> References: <1430335224-6716-1-git-send-email-mdroth@linux.vnet.ibm.com> <1430335224-6716-7-git-send-email-mdroth@linux.vnet.ibm.com> <20150505113415.GA14090@voom.redhat.com> Message-ID: <20150505155422.25451.99774@loki> Date: Tue, 05 May 2015 10:54:22 -0500 Subject: Re: [Qemu-devel] [RFC PATCH 06/15] spapr_pci: also use 'index' property as DRC index for PHBs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: aik@ozlabs.ru, nfont@linux.vnet.ibm.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org, bharata@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 > = > 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 !=3D (uint64_t)-1) || (sphb->dma_liobn !=3D (u= int32_t)-1) > > || (sphb->mem_win_addr !=3D (hwaddr)-1) > > || (sphb->io_win_addr !=3D (hwaddr)-1)) { > > - error_setg(errp, "Either \"index\" or other parameters mus= t" > > - " be specified for PAPR PHB, not both"); > > + if (!spapr->dr_phb_enabled) { > > + /* if they aren't potentially using index as an identi= fier for > > + * the PHB's DR connector, enforce the old semantics o= f index > > + * being purely a shorthand for PHB configuration opti= ons. > > + */ > > + 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 =3D windows_base + SPAPR_PCI_MMIO_WIN_OFF; > > sphb->io_win_addr =3D windows_base + SPAPR_PCI_IO_WIN_OFF; > > + } else { > > + if (spapr->dr_phb_enabled) { > > + error_setg(errp, "The \"index\" property is required for m= achine" > > + " types that support PHB hotplug (and in such c= ases" > > + " can be used alongside \"buid\" and other" > > + " configuration properties)"); > > + return; > > + } > > } > > = > > if (sphb->buid =3D=3D (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_ _othe= r_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson