From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44440) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c12j3-0004BM-TK for qemu-devel@nongnu.org; Sun, 30 Oct 2016 22:53:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c12j0-0005zb-Lk for qemu-devel@nongnu.org; Sun, 30 Oct 2016 22:53:29 -0400 Date: Mon, 31 Oct 2016 13:53:14 +1100 From: David Gibson Message-ID: <20161031025314.GJ18226@umbus.fritz.box> References: <1477641400-23292-1-git-send-email-aik@ozlabs.ru> <20161028120712.4a911866@bahia> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="NqSa+Xr3J/G6Hhls" Content-Disposition: inline In-Reply-To: <20161028120712.4a911866@bahia> Subject: Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH qemu] spapr_pci: Create PCI-express root bus by default List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Alexey Kardashevskiy , qemu-devel@nongnu.org, Michael Roth , Alex Williamson , qemu-ppc@nongnu.org, Paolo Bonzini --NqSa+Xr3J/G6Hhls Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 28, 2016 at 12:07:12PM +0200, Greg Kurz wrote: > On Fri, 28 Oct 2016 18:56:40 +1100 > Alexey Kardashevskiy wrote: >=20 > > At the moment sPAPR PHB creates a root buf of TYPE_PCI_BUS type. > > This means that vfio-pci devices attached to it (and this is > > a default behaviour) hide PCIe extended capabilities as > > the bus does not pass a pci_bus_is_express(pdev->bus) check. > >=20 > > This changes adds a default PCI bus type property to sPAPR PHB > > and uses TYPE_PCIE_BUS if none passed; older machines get TYPE_PCI_BUS > > for backward compatibility as a bus type is used in the bus name > > so the root bus name becomes "pcie.0" instead of "pci.0". > >=20 > > Signed-off-by: Alexey Kardashevskiy > > --- > >=20 > > What can possibly go wrong with such change of a name? > > From devices prospective, I cannot see any. > >=20 > > libvirt might get upset as "pci.0" will not be available, > > will it make sense to create pcie.0 as a root bus and always > > add a PCIe->PCI bridge and name its bus "pci.0"? > >=20 > > Or create root bus from TYPE_PCIE_BUS and force name to "pci.0"? > > pci_register_bus() can do this. > >=20 > >=20 > > --- > > hw/ppc/spapr.c | 5 +++++ > > hw/ppc/spapr_pci.c | 5 ++++- > > include/hw/pci-host/spapr.h | 1 + > > 3 files changed, 10 insertions(+), 1 deletion(-) > >=20 > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 0b3820b..a268511 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2541,6 +2541,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true); > > .driver =3D TYPE_SPAPR_PCI_HOST_BRIDGE, \ > > .property =3D "mem64_win_size", \ > > .value =3D "0", \ > > + }, \ > > + { \ > > + .driver =3D TYPE_SPAPR_PCI_HOST_BRIDGE, \ > > + .property =3D "root_bus_type", \ > > + .value =3D TYPE_PCI_BUS, \ > > }, > > =20 > > static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index, > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 7cde30e..2fa1f22 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1434,7 +1434,9 @@ static void spapr_phb_realize(DeviceState *dev, E= rror **errp) > > bus =3D pci_register_bus(dev, NULL, > > pci_spapr_set_irq, pci_spapr_map_irq, sphb, > > &sphb->memspace, &sphb->iospace, > > - PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS= ); > > + PCI_DEVFN(0, 0), PCI_NUM_PINS, > > + sphb->root_bus_type ? sphb->root_bus_type : > > + TYPE_PCIE_BUS); >=20 > Shouldn't we ensure that sphb->root_bus_type is either TYPE_PCIE_BUS or > TYPE_PCI_BUS ? Yes, I think so. In fact, I think it would be better to make the property a boolean that just selects PCI-E, rather than this which exposes qemu (semi-)internal type names on the comamnd line. > Otherwise, if we pass something like: >=20 > -global spapr-pci-host-bridge.root_bus_type=3Dpcie >=20 > we get the following not really explicit error: >=20 > ERROR:/home/greg/Work/qemu/qemu-spapr/qom/object.c:474:object_new_with_ty= pe: assertion failed: (type !=3D NULL) >=20 > > phb->bus =3D bus; > > qbus_set_hotplug_handler(BUS(phb->bus), DEVICE(sphb), NULL); > > =20 > > @@ -1590,6 +1592,7 @@ static Property spapr_phb_properties[] =3D { > > DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask, > > (1ULL << 12) | (1ULL << 16)), > > DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1), > > + DEFINE_PROP_STRING("root_bus_type", sPAPRPHBState, root_bus_type), > > DEFINE_PROP_END_OF_LIST(), > > }; > > =20 > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > > index b92c1b5..a4ee873 100644 > > --- a/include/hw/pci-host/spapr.h > > +++ b/include/hw/pci-host/spapr.h > > @@ -79,6 +79,7 @@ struct sPAPRPHBState { > > uint64_t dma64_win_addr; > > =20 > > uint32_t numa_node; > > + char *root_bus_type; > > }; > > =20 > > #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL >=20 --=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 --NqSa+Xr3J/G6Hhls Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYFrIaAAoJEGw4ysog2bOSx+QQAN9M3W+910dK+WDanhcVyjas HsGroAX7I6ADyQgRkihqiENNtiayOzDZdWTJGF5oJjHgYt5fLeWjjbvj5hz3QL+U /y8NwZyuZe1bIYeyTc9IzabBs5jODEqWuAufkNCXOHR6IdgbAvoLzlkR/yv8oqTt zU/9/Ba9/saJaPaqTPKIx8tITWGdpYIXqi+7R0uBfethD5tkiti2fU4QsH41NQyK GNx4kKl5uCXGZI+VRXQiRFcjH4NLgmoBc/+uQJqfMOrVvmOZNWansXsFfPYc6rS8 f1XXbqmexSA0jRCLe/ttCh0PQtW5fUkG+oxXuwd6H10KIRulWT2oqLWTeHiNr1xL 3GQ3SSkl2FaabrUxNYJ+8UA+NgJuhaTaXQrU5fYg7k3+MI4CNRf57f98KzxMFkle B8RqcejBpcR6M6msT4s3Jykesj4N9aKaJC5a1wm6FN7woWbj8yYzKIbTq+Yi+7eA WGnUzmQctROkOkpbA27IcwrYO50TNJpl3GLAyq8u1YLV5VVOtb8QYBGSqC4g1uXu tTbYsl29/EVnajPeHeScW5udWmPezV39JMMImTvuyzglhtYYIuwftW0a2e+gHvHi GgzlyntTa1bPkaFCH//4p7BmI7c84C79YuSkH9xz3cNyRhLrC0IrlYCgopKmIGAl /VhAkXzyez1TRcEsXmpO =uS// -----END PGP SIGNATURE----- --NqSa+Xr3J/G6Hhls--