From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58527) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ad7He-0002ti-I6 for qemu-devel@nongnu.org; Mon, 07 Mar 2016 21:22:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ad7Hc-0002Th-NF for qemu-devel@nongnu.org; Mon, 07 Mar 2016 21:22:02 -0500 Date: Tue, 8 Mar 2016 12:34:29 +1100 From: David Gibson Message-ID: <20160308013429.GR22546@voom.fritz.box> References: <1456729587-17229-1-git-send-email-david@gibson.dropbear.id.au> <1456729587-17229-2-git-send-email-david@gibson.dropbear.id.au> <20160307124904.2cc7343f@t450s.home> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Hw0FrjWlp+qkNlJP" Content-Disposition: inline In-Reply-To: <20160307124904.2cc7343f@t450s.home> Subject: Re: [Qemu-devel] [PATCHv2 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, agraf@suse.de, gwshan@au1.ibm.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org --Hw0FrjWlp+qkNlJP Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Mar 07, 2016 at 12:49:04PM -0700, Alex Williamson wrote: > On Mon, 29 Feb 2016 18:06:21 +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 and at most a single group attached), and > > vfio_eeh_as_op() which will perform an operation on an AddressSpace in > > the unambiguous case, 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 > > --- > > hw/vfio/common.c | 84 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > > include/hw/vfio/vfio.h | 2 ++ > > 2 files changed, 86 insertions(+) > >=20 > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > index 607ec70..b61118e 100644 > > --- a/hw/vfio/common.c > > +++ b/hw/vfio/common.c > > @@ -1003,3 +1003,87 @@ 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) > > +{ > > + /* A broken kernel implementation means EEH operations won't work > > + * correctly if there are multiple groups in a container. So > > + * return true only if there is exactly one group attached to the > > + * container */ >=20 > Please don't add a new comment style to the file. What's broken about > the kernel implementation? It would be great if someone reading this > comment could understand the "why" rather than just the "what". Ok, I've altered the style and expanded the details. > > + > > + 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 called on container" > > + " with multiple groups", op); >=20 > I know you rejected this before, but why is vfio_eeh_container_ok() not > called vfio_eeh_singleton_container() since that's what it's checking > for? Because the intention is that when the kernel gets fixed, this will be altered to succeed if we see whatever capability we use to advertise the fixed kernel. > This error should also say "not singleton", or something to that > effect, since it might have failed for having no groups. The line wrap > could also easily be done after the %x to make searching for the error > string easier. I've adjusted the message to address those points. >=20 > > + 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); > > + > > + /* Shouldn't be called unless vfio_eeh_as_ok() returned true */ > > + assert(container); >=20 > Why not just let vfio_eeh_container_op() test for this too and return > -ENODEV? I'm generally not a fan of asserts when we could just return > an error to the caller? Ok, I'll change this. >=20 > > + 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 --Hw0FrjWlp+qkNlJP Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW3iwlAAoJEGw4ysog2bOSdfAQAIM6n+JokAKQgbfKQhRjpHvy v8RY255sHAlSSAj5MJWtC8qWe1QRsssXfYM9mswyIY1uoQqsQUZNs6lXBhX4loTq OREuD4+frn8RvexuB0XTk7ZKDUIgyRkPVl2/0phLzVyae21uSo0rg+guZkSLU1tA JieCggvnoIGAElcu5KntnoMMi/BzIj9+yWgk4XJkPlvWL6oLEBsi46ZJS5kksqqg K7Uu6jrp0vAnxa/AaUxCZ5lcX3eWdbObA2EB6B6HC3My0btD9kc+DR8rR6bx81su qxGJMdmy/KdhUzsIt3VoZ8+OjnrDXz6jiQ4/tEtw/KOhmVF/qNSSB1LHaPIDtA2/ YbtU9mz/1cyl+Yoc6rA8VzOBb9z4iGhVpmzDzP9TYJZg5Lcn57QnWr5oS2rG3Opq Z8YRvUK/yS9Psad8g47Dj4WloJ/v4wjF4VqQ+Lu3TNZVaaCjvnxp2oierUNue35D P3ltcbmAE1RkULxKwiJfVLrAuu5Xpl2/jeYKxJfLVN2hJNgKUDoB/50KfE8h3CFf 1j+OYic4vcXdSmRwCqH/CkdIMp0b9hgeLlumprgSrCh7ntc3CK7th4GtGlo4Rn7Z yav93pgrOp6vrZp09kvoYZbYtZtfFD7BBkDNg+Cx0NAVYph29ang7ew3PQbsJh21 2v/BTBpVe2wFF3yj6ggE =fP2S -----END PGP SIGNATURE----- --Hw0FrjWlp+qkNlJP--