From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55503) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c1QDy-0007Wm-Rd for qemu-devel@nongnu.org; Mon, 31 Oct 2016 23:59:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c1QDu-0005Kg-KE for qemu-devel@nongnu.org; Mon, 31 Oct 2016 23:58:58 -0400 Date: Tue, 1 Nov 2016 13:46:34 +1100 From: David Gibson Message-ID: <20161101024634.GA14860@umbus.fritz.box> References: <1477641400-23292-1-git-send-email-aik@ozlabs.ru> <20161028120712.4a911866@bahia> <20161031025314.GJ18226@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="x+6KMIRAuhnl3hBn" Content-Disposition: inline In-Reply-To: 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: Alexey Kardashevskiy Cc: Greg Kurz , Paolo Bonzini , Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Michael Roth , abologna@redhat.com --x+6KMIRAuhnl3hBn Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 31, 2016 at 03:10:23PM +1100, Alexey Kardashevskiy wrote: > On 31/10/16 13:53, David Gibson wrote: > > 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: > >> > >>> 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. > >>> > >>> 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". > >>> > >>> Signed-off-by: Alexey Kardashevskiy > >>> --- > >>> > >>> What can possibly go wrong with such change of a name? > >>> From devices prospective, I cannot see any. > >>> > >>> 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"? > >>> > >>> Or create root bus from TYPE_PCIE_BUS and force name to "pci.0"? > >>> pci_register_bus() can do this. > >>> > >>> > >>> --- > >>> 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(-) > >>> > >>> 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 ind= ex, > >>> 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,= Error **errp) > >>> bus =3D pci_register_bus(dev, NULL, > >>> pci_spapr_set_irq, pci_spapr_map_irq, sph= b, > >>> &sphb->memspace, &sphb->iospace, > >>> - PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_B= US); > >>> + PCI_DEVFN(0, 0), PCI_NUM_PINS, > >>> + sphb->root_bus_type ? sphb->root_bus_type= : > >>> + TYPE_PCIE_BUS); > >> > >> Shouldn't we ensure that sphb->root_bus_type is either TYPE_PCIE_BUS or > >> TYPE_PCI_BUS ? > >=20 > > 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. >=20 >=20 > Sure, a "pcie-root" boolean property should do. >=20 > However this is not my main concern, I rather wonder if we have to have > pci.0 when we pick PCIe for the root. Right. I've added Andrea Bologna to the CC list to get a libvirt perspective. Andrea, To summarise the issue here: * As I've said before the PAPR spec kinda-sorta abstracts the difference between vanilla PCI and PCI-E * However, because within qemu we're declaring the bus as PCI that means some PCI-E devices aren't working right * In particular it means that PCI-E extended config space isn't available The proposal is to change (on newer machine types) the spapr PHB code to declare a PCI-E bus instead. AIUI this still won't make the root complex guest visible (which it's not supposed to be under PAPR), and the guest shouldn't see a difference in most cases - it will still see the PAPR abstracted PCIish bus, but will now be able to get extended config space. The possible problem from a libvirt perspective is that doing this in the simplest way in qemu would change the name of the default bus from pci.0 to pcie.0. We have two suggested ways to mitigate this: 1) Automatically create a PCI-E to PCI bridge, so that new machine types will have both a pcie.0 and pci.0 bus 2) Force the name of the bus to be pci.0, even though it's treated as PCI-E in other ways. We're trying to work out exactly what will and won't cause trouble for libvirt. --=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 --x+6KMIRAuhnl3hBn Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYGAIHAAoJEGw4ysog2bOSGoQP/0ZOn69lZaHVe2xnCymO4ezz LhzbakSQ2LTsDQ3rBmcRewhMdcTmfLHSp40uEYVCyj6PXU3uzbZ4LS0PtMNHHuBu qm3+IsB/uV0tzAq5qOV2yUPszyaWTwt4L9F4HGUY6BRB8eFIL+pDhX7Jfrk4GcTu qB04z09Lyi4LPcO/K/vxD58fQ084P1+9y19vMVfIl5rQWBYIB27VCfKbWg1AiKox lzAZxx1puH0UbH3XlDjUPS1YNc4et2OMF11ppLF5WMVc7gE9/4GOk0czOH3JCOvL wZ+3SzTa20md3Y9MdaJjcaQnv1xxPnT08g3tsxQ/kdKGUw9Bm9uVgPEqUMDmQBjY XF8zHcOb59SDys/+DOgumta8Ju3qrfyEecdSS1OKz/9hnGkYE6Lvfx1LtJgRpFLS NaIK8Thclrl2JRurjBYqSUuLNZubNUnh4JlGiGb9FGDD6ZLUr6AgOPwRRIIpb0Tl vxmOEhcD7oJ1Bi3uG22pW0E1hb5mnLnCfEEtGVTuzu0DW2qIZr/yQlmWIp0nfVqb 7Ybpp/lSIfVnRqx7Q0GMW9VLo9zeWIYmlFFMABdYPngtQ0qJysGgVjnceS1jfU3N esjKMSFKK/EosTEdh8Y9UfqTSzDYJNLYH1wbI8we1DHBvTUWu0KvQItzIMsiMr6r GgU04YGoOSmrtIdDWQVf =NGsR -----END PGP SIGNATURE----- --x+6KMIRAuhnl3hBn--