From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57893) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpbJW-0002K8-8K for qemu-devel@nongnu.org; Tue, 05 May 2015 07:47:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YpbJT-0003dP-Bc for qemu-devel@nongnu.org; Tue, 05 May 2015 07:47:02 -0400 Date: Tue, 5 May 2015 21:34:15 +1000 From: David Gibson Message-ID: <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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="AX2clmtSw6ieHBMM" Content-Disposition: inline In-Reply-To: <1430335224-6716-7-git-send-email-mdroth@linux.vnet.ibm.com> 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: Michael Roth Cc: aik@ozlabs.ru, nfont@linux.vnet.ibm.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org, bharata@linux.vnet.ibm.com --AX2clmtSw6ieHBMM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > BUIDs would be a good choice, however, those are 64-bit values, > whereas DRC indexes are 32-bit. >=20 > '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). >=20 > 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. >=20 > 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). > --- > hw/ppc/spapr_pci.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) >=20 > 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, Er= ror **errp) > if ((sphb->buid !=3D (uint64_t)-1) || (sphb->dma_liobn !=3D (uin= t32_t)-1) > || (sphb->mem_win_addr !=3D (hwaddr)-1) > || (sphb->io_win_addr !=3D (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 identifi= er for > + * the PHB's DR connector, enforce the old semantics of = index > + * being purely a shorthand for PHB configuration option= s. > + */ > + error_setg(errp, "Either \"index\" or other parameters m= ust" > + " be specified for PAPR PHB, not both"); > + } > return; > } > =20 > @@ -1174,6 +1180,14 @@ static void spapr_phb_realize(DeviceState *dev, Er= ror **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 mac= hine" > + " types that support PHB hotplug (and in such cas= es" > + " can be used alongside \"buid\" and other" > + " configuration properties)"); > + return; > + } > } > =20 > if (sphb->buid =3D=3D (uint64_t)-1) { --=20 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 --AX2clmtSw6ieHBMM Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVSKq3AAoJEGw4ysog2bOSAa0P/32f2lfWSjiGik+LfjjaSDJi VwrD+nkDvxnLDCGbYmILUvUGdHxoQCyx3DPDzHzaSigeQ88h+ekKmlaNNdqNmcpq GWNEABmNRXN5crmAXw9atClqPxInwk89JkFzbNS0hh14uMLJRFGOHkpVhpHrN/cq qK7kN/SmUEPiDZrFSHPcQrgPVW/I2Sp1bTdN/oSSGjtuYeBb60au4e6nIdP24ErH i1RJ4AJ4yqHYfZXRb0dGJ20g5Jul/WpuBoiDqNsAhf6sO1ZLNDzYKaGlxwX3+Opc +miUn0HB7HHXuQD9nz7bg6wH+vVgaB0SoZTCVw213BpydzSctrtopYEtWrpa6xc8 Zyawx8eylYiPM2ZR2BhDRSrFdGKzyBX7Hrj60JXQ4R91dVzB8sbuuVuG5D0nuR+U LS02UPQIh2gn422XygW6W6jkvuL4MOeLs0Ui+walLIV0USEg3GzjwadLOlsyP5Hp 5BBC5ph1vAe5g393Qt8ndOnyBJnK2tPFMOC2tv8Y54LahmTf4L2keWsicIp0otdU aKAOsAYLK9Lpu5d6XDi7FxYEfv5uCiDah9k1YekClxMDZcPbxIcqZuCKCZ73dnNS sKns7GGefGeDKZHtFfCNLNiAFJ9GdWbgKmD37MmIz6LHojOoiNOKLJz4rPMBksHt okoc5c7nHIhkym6YF2WO =YPOF -----END PGP SIGNATURE----- --AX2clmtSw6ieHBMM--