From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32797) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adSRk-0000Zk-TP for qemu-devel@nongnu.org; Tue, 08 Mar 2016 19:57:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adSRj-0003f7-66 for qemu-devel@nongnu.org; Tue, 08 Mar 2016 19:57:52 -0500 Date: Wed, 9 Mar 2016 11:56:57 +1100 From: David Gibson Message-ID: <20160309005657.GF22546@voom.fritz.box> References: <1457403029-21322-1-git-send-email-david@gibson.dropbear.id.au> <1457403029-21322-2-git-send-email-david@gibson.dropbear.id.au> <20160308113345.28412740@t450s.home> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="iBipwI8N6cjWJAiJ" Content-Disposition: inline In-Reply-To: <20160308113345.28412740@t450s.home> Subject: Re: [Qemu-devel] [PATCHv3 1/7] vfio: Start improving VFIO/EEH interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: aik@ozlabs.ru, qemu-devel@nongnu.org, gwshan@au1.ibm.com, agraf@suse.de, qemu-ppc@nongnu.org --iBipwI8N6cjWJAiJ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 08, 2016 at 11:33:45AM -0700, Alex Williamson wrote: > On Tue, 8 Mar 2016 13:10:23 +1100 > David Gibson wrote: >=20 > > At present the code handling IBM's Enhanced Error Handling (EEH) interf= ace > > on VFIO devices operates by bypassing the usual VFIO logic with > > vfio_container_ioctl(). That's a poorly designed interface with unclear > > semantics about exactly what can be operated on. > >=20 > > In particular it operates on a single vfio container internally (hence = the > > name), but takes an address space and group id, from which it deduces t= he > > container in a rather roundabout way. groupids are something that code > > outside vfio shouldn't even be aware of. > >=20 > > This patch creates new interfaces for EEH operations. Internally we > > have vfio_eeh_container_op() which takes a VFIOContainer object > > directly. For external use we have vfio_eeh_as_ok() which determines > > if an AddressSpace is usable for EEH (at present this means it has a > > single container with exactly one group attached), and vfio_eeh_as_op() > > which will perform an operation on an AddressSpace in the unambiguous c= ase, > > and otherwise returns an error. > >=20 > > This interface still isn't great, but it's enough of an improvement to > > allow a number of cleanups in other places. > >=20 > > Signed-off-by: David Gibson > > Reviewed-by: Alexey Kardashevskiy > > --- >=20 > I'll let you push this through your tree: >=20 > Acked-by: Alex Williamson Thanks. Any guess at when your vGPU series will be pushed? Mine will conflict until that is merged upstream. >=20 > > hw/vfio/common.c | 95 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > > include/hw/vfio/vfio.h | 2 ++ > > 2 files changed, 97 insertions(+) > >=20 > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > index 96ccb79..0636bb1 100644 > > --- a/hw/vfio/common.c > > +++ b/hw/vfio/common.c > > @@ -1137,3 +1137,98 @@ int vfio_container_ioctl(AddressSpace *as, int32= _t groupid, > > =20 > > return vfio_container_do_ioctl(as, groupid, req, param); > > } > > + > > +/* > > + * Interfaces for IBM EEH (Enhanced Error Handling) > > + */ > > +static bool vfio_eeh_container_ok(VFIOContainer *container) > > +{ > > + /* > > + * As of 2016-03-04 (linux-4.5) the host kernel EEH/VFIO > > + * implementation is broken if there are multiple groups in a > > + * container. The hardware works in units of Partitionable > > + * Endpoints (=3D=3D IOMMU groups) and the EEH operations naively > > + * iterate across all groups in the container, without any logic > > + * to make sure the groups have their state synchronized. For > > + * certain operations (ENABLE) that might be ok, until an error > > + * occurs, but for others (GET_STATE) it's clearly broken. > > + */ > > + > > + /* > > + * XXX Once fixed kernels exist, test for them here > > + */ > > + > > + if (QLIST_EMPTY(&container->group_list)) { > > + return false; > > + } > > + > > + if (QLIST_NEXT(QLIST_FIRST(&container->group_list), container_next= )) { > > + return false; > > + } > > + > > + return true; > > +} > > + > > +static int vfio_eeh_container_op(VFIOContainer *container, uint32_t op) > > +{ > > + struct vfio_eeh_pe_op pe_op =3D { > > + .argsz =3D sizeof(pe_op), > > + .op =3D op, > > + }; > > + int ret; > > + > > + if (!vfio_eeh_container_ok(container)) { > > + error_report("vfio/eeh: EEH_PE_OP 0x%x: " > > + "kernel requires a container with exactly one gro= up", op); > > + return -EPERM; > > + } > > + > > + ret =3D ioctl(container->fd, VFIO_EEH_PE_OP, &pe_op); > > + if (ret < 0) { > > + error_report("vfio/eeh: EEH_PE_OP 0x%x failed: %m", op); > > + return -errno; > > + } > > + > > + return 0; > > +} > > + > > +static VFIOContainer *vfio_eeh_as_container(AddressSpace *as) > > +{ > > + VFIOAddressSpace *space =3D vfio_get_address_space(as); > > + VFIOContainer *container =3D NULL; > > + > > + if (QLIST_EMPTY(&space->containers)) { > > + /* No containers to act on */ > > + goto out; > > + } > > + > > + container =3D QLIST_FIRST(&space->containers); > > + > > + if (QLIST_NEXT(container, next)) { > > + /* We don't yet have logic to synchronize EEH state across > > + * multiple containers */ > > + container =3D NULL; > > + goto out; > > + } > > + > > +out: > > + vfio_put_address_space(space); > > + return container; > > +} > > + > > +bool vfio_eeh_as_ok(AddressSpace *as) > > +{ > > + VFIOContainer *container =3D vfio_eeh_as_container(as); > > + > > + return (container !=3D NULL) && vfio_eeh_container_ok(container); > > +} > > + > > +int vfio_eeh_as_op(AddressSpace *as, uint32_t op) > > +{ > > + VFIOContainer *container =3D vfio_eeh_as_container(as); > > + > > + if (!container) { > > + return -ENODEV; > > + } > > + return vfio_eeh_container_op(container, op); > > +} > > diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h > > index 0b26cd8..fd3933b 100644 > > --- a/include/hw/vfio/vfio.h > > +++ b/include/hw/vfio/vfio.h > > @@ -5,5 +5,7 @@ > > =20 > > extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid, > > int req, void *param); > > +bool vfio_eeh_as_ok(AddressSpace *as); > > +int vfio_eeh_as_op(AddressSpace *as, uint32_t op); > > =20 > > #endif >=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 --iBipwI8N6cjWJAiJ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW33TZAAoJEGw4ysog2bOSO4IQAMHwMuYTfCJDZunqQRmgfIpJ FtOT22Oi2NqNBUkyuSG9lu9LDgX0NQtezYBEAJ4O83fSOL6F+vH7quJrVxyqfUZ3 gma1fN4zhsJdaC7MBfoA5KpiZxwgbiiAHBtv7S/XJY+NaOgNlEq41az8RRVXRjWF W5PRn5qDYEv9HtnfmBe34eCFmIXt6gCKVqVHUaZCuSzMVWxREeh0phPhlVToAviH Gh3KapgakwEyf3rzGk9aIqyBXu+9VoIr2AJHwMaqzlPfDf8A61KOaoI5zZpDAme/ PboSYNPSWHRg/INucR5ECHXPdCn++rqz1jLeg7dymq3UbWnwdx1iuNKTwu8jzO8d nS7Rq+AD8FD5B48DeojCVU0J5tK1xIt+D/4O1osD5g9vFX/lz5Ew1nZh+ssLq/Rf uGAdvW4wtZZxtFdXszAsr3L0dRZefZLj3aeCv4GF7ntvYCe7Rf3C5oK4TjjFu3P2 szLWx3NaxiKA0OJF3/J0ngoEJVlaaPoZWsZe2iCpxoDe1m4/xX5j3v+5/UBRziPy STAJtmoS3CQdPJ2vHAajYVmtxeohZRUpHyXxN49tbfZnsWG2RSNgvVYeQ/4RBB1s WZOHmhCJiZXqox3yafufDrgKoOOm9pQs0+IZqtX1I9aQ4NgNP/R789MjCkYcFSrH gp03mdIJPwFtWhL7e3kd =GbHM -----END PGP SIGNATURE----- --iBipwI8N6cjWJAiJ--