From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41814) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZevfO-0007xj-87 for qemu-devel@nongnu.org; Wed, 23 Sep 2015 21:49:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZevfL-0002t7-BO for qemu-devel@nongnu.org; Wed, 23 Sep 2015 21:49:46 -0400 Date: Thu, 24 Sep 2015 11:49:59 +1000 From: David Gibson Message-ID: <20150924014959.GM15944@voom.fritz.box> References: <1442647117-2726-1-git-send-email-david@gibson.dropbear.id.au> <1442647117-2726-12-git-send-email-david@gibson.dropbear.id.au> <1443029281.23936.481.camel@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="1rguoi8KZGYj2k4L" Content-Disposition: inline In-Reply-To: <1443029281.23936.481.camel@redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH 11/14] spapr_pci: Allow EEH on spapr-pci-host-bridge List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: lvivier@redhat.com, thuth@redhat.com, mdroth@linux.vnet.ibm.com, aik@ozlabs.ru, qemu-devel@nongnu.org, gwshan@linux.vnet.ibm.com, qemu-ppc@nongnu.org --1rguoi8KZGYj2k4L Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 23, 2015 at 11:28:01AM -0600, Alex Williamson wrote: > On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote: > > The pseries machine type has two variants of the PCI Host Bridge device: > > spapr-pci-host-bridge and spapr-pci-vfio-host-bridge. Originally, only= the > > latter could support VFIO devices, but we've now extended the VFIO code= so > > that they both can. > >=20 > > However, it's still only spapr-pci-vfio-host-bridge which supports the = PAPR > > Enhanced Error Handling (EEH) interfaces. The reason is that we don't = yet > > have a way to determine the correct VFIOGroup for EEH operations on the > > "plain" host bridge. > >=20 > > Handling this in general is problematic due to some limitations in the > > current code, and some bugs in the kernel EEH interfaces. However, it's > > easy enough to do for the unambiguous case: where there is only one VFIO > > group used on the whole host bridge i.e. one Partitionable Endpoint (PE= ). > >=20 > > This re-implements spapr_phb_check_vfio_group() in terms of the host > > bridge's Partitionable Endpoints, allowing EEH on any host bridge in the > > unambiguous case. This is enough to make spapr-pci-host-bridge support > > EEH as well as spapr-pci-vfio-host-bridge, since the latter only suppor= ted > > devices from one IOMMU group anyway (although it didn't properly > > enforce that). > >=20 > > Signed-off-by: David Gibson > > --- > > hw/ppc/spapr_pci.c | 36 ++++++++++++++++++++++++++++++++++++ > > hw/ppc/spapr_pci_vfio.c | 18 ------------------ > > include/hw/pci-host/spapr.h | 1 - > > 3 files changed, 36 insertions(+), 19 deletions(-) > >=20 > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 81ad3ae..5614b45 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -41,6 +41,8 @@ > > #include "hw/ppc/spapr_drc.h" > > #include "sysemu/device_tree.h" > > =20 > > +#include "hw/vfio/vfio-common.h" > > + > > /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */ > > #define RTAS_QUERY_FN 0 > > #define RTAS_CHANGE_FN 1 > > @@ -446,6 +448,40 @@ static sPAPRPHBGuestPE *spapr_phb_pe_by_device(sPA= PRPHBState *phb, > > return pe; > > } > > =20 > > +static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **= gpp) > > +{ > > + sPAPRPHBGuestPE *pe; > > + > > + if (QLIST_EMPTY(&phb->pe_list)) { > > + /* No EEH capable devices on this PHB */ > > + return RTAS_OUT_PARAM_ERROR; > > + } > > + > > + /* Limitations in both qemu and the kernel mean that, for now, EEH > > + * won't work if there are devices from more than one PE > > + * (i.e. IOMMU group) on the same PHB */ > > + pe =3D QLIST_FIRST(&phb->pe_list); > > + if (QLIST_NEXT(pe, list)) { > > + error_report("spapr-pci-host-bridge: EEH attempted on PHB with= multiple" > > + " IOMMU groups"); >=20 > Don't wrap lines with search-able strings Noted. > > + return RTAS_OUT_HW_ERROR; > > + } > > + > > + if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge"= )) { > > + sPAPRPHBVFIOState *svphb =3D SPAPR_PCI_VFIO_HOST_BRIDGE(phb); > > + /* FIXME: this is an abstraction violation */ > > + if (pe->group->groupid !=3D svphb->iommugroupid) { > > + error_report("spapr-pci-vfio-host-bridge: Bad IOMMU group"= ); > > + return RTAS_OUT_HW_ERROR; > > + } > > + } > > + > > + if (gpp) { > > + *gpp =3D pe->group; > > + } > > + return RTAS_OUT_SUCCESS; >=20 >=20 > The structure of this function finally makes some sense once we get > here. Yeah, I started off with something more like what you suggested in earlier comments, but I changed to this rather awkward structure to avoid churning the callers as this moves towards its final form > I'm still not sure whether RTAS error codes make sense though, > but it's just a nit. Well, (nearly) all the callers are in RTAS, and translating the error codes there seems pretty ugly too. > I'd still prefer the function was renamed, "check" > indicates a verification, not a return. The primary purpose of calling > this function is to get a group, not simply to check the validity. Yeah, it's a crap name. How about spapr_phb_find_vfio_group(). Fwiw, I plan to make this function go away to as I rework things to remove the one-group-per-PHB restriction. > > +} > > + > > static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu, > > sPAPRMachineState *spapr, > > uint32_t token, uint32_t nargs, > > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > > index b61923c..a08292a 100644 > > --- a/hw/ppc/spapr_pci_vfio.c > > +++ b/hw/ppc/spapr_pci_vfio.c > > @@ -24,29 +24,11 @@ > > #include "hw/vfio/vfio.h" > > #include "hw/vfio/vfio-eeh.h" > > =20 > > -#include "hw/vfio/vfio-common.h" > > - > > static Property spapr_phb_vfio_properties[] =3D { > > DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1), > > DEFINE_PROP_END_OF_LIST(), > > }; > > =20 > > -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp) > > -{ > > - VFIOGroup *group; > > - > > - if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge"= )) { > > - return RTAS_OUT_PARAM_ERROR; > > - } > > - > > - /* FIXME: this is an abstraction violation */ > > - group =3D vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugro= upid, > > - &phb->iommu_as); > > - if (gpp) > > - *gpp =3D group; > > - return RTAS_OUT_SUCCESS; > > -} > > - > > static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error *= *errp) > > { > > sPAPRPHBVFIOState *svphb =3D SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > > index 535e5ef..8c8d187 100644 > > --- a/include/hw/pci-host/spapr.h > > +++ b/include/hw/pci-host/spapr.h > > @@ -138,7 +138,6 @@ void spapr_pci_msi_init(sPAPRMachineState *spapr, h= waddr addr); > > =20 > > void spapr_pci_rtas_init(void); > > =20 > > -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp); > > sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t b= uid); > > PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid, > > uint32_t config_addr); >=20 >=20 >=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 --1rguoi8KZGYj2k4L Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWA1bGAAoJEGw4ysog2bOSddsP/RNA7u0u4ghhMNE2/spxkTYL B0uK08YEIxS7dtP5dGuxxlME/KZ09ydPWw8nk22YNILemTks+FOotET8wA5hQvcb kSJZEXjcoMTvcdKKmiEqXnXKJ5kriUOdpnx/ldmn3xITyUwJDvIMr8QaiAulscsC vHQ7/5W+1xhDCc9TobS/SdOqdoSypTTtD4gKhVH7H+gBFCxZtzYFmvbhMGy3j2US O1u1/1IAdEYW7+MJW0zNJprizUyjD8zfF0GFVMbZwiEXoZRgCmwURpzurLFWcFx3 i09zL4ieVRb7YzXqISz7101ovVoxQgfZj5oDhdFIoeB/DjrrafkdYetQTl8UKq2H AEP3lz+Hp7lS9PzqnnPh+q/N5g5XzQKkM0EOeWj9bdVkzsyiOf8BjSs+WhKjll7G rJzhgxhrI83DVyGG3aIrD6BBZUJ5f2qwp6vl3+0XI8w1prGbPPhYBYEap8Cz89lG UMjgaM80Pf4ZEvgOzvP2k7hfSuhJfqkWCm/KWXVybV3IYvGXW3mvEu8iSZB9o9pt 1WlLDcsAhbfjn0Okq7ii2YwfMJtQ2zRU/Bv8m9fUMGWMFeyQp0KU1MdpyuYH6PNS 1VSUPtIYsUKADXApWJr9bynd6+BKmWOoUe2TZZ0WjlE3Wf4k1ewnbBRj24+bI+qo 0tRP8xxmuIT7u6BNaQ9u =mBhP -----END PGP SIGNATURE----- --1rguoi8KZGYj2k4L--