From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35051) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaFyD-0004SX-1a for qemu-devel@nongnu.org; Mon, 29 Feb 2016 00:02:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaFyB-0000v0-Ne for qemu-devel@nongnu.org; Mon, 29 Feb 2016 00:02:08 -0500 Date: Mon, 29 Feb 2016 14:13:10 +1100 From: David Gibson Message-ID: <20160229031310.GD5427@voom.redhat.com> References: <1456486323-8047-1-git-send-email-david@gibson.dropbear.id.au> <1456486323-8047-2-git-send-email-david@gibson.dropbear.id.au> <56D397CE.5060906@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="veXX9dWIonWZEC6h" Content-Disposition: inline In-Reply-To: <56D397CE.5060906@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH 01/12] vfio: Start improving VFIO/EEH interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, gwshan@au1.ibm.com, agraf@suse.de, alex.williamson@redhat.com, qemu-ppc@nongnu.org --veXX9dWIonWZEC6h Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Feb 29, 2016 at 11:58:54AM +1100, Alexey Kardashevskiy wrote: > On 02/26/2016 10:31 PM, David Gibson wrote: > >At present the code handling IBM's Enhanced Error Handling (EEH) interfa= ce > >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. > > > >In particular it operates on a single vfio container internally (hence t= he > >name), but takes an address space and group id, from which it deduces the > >container in a rather roundabout way. groupids are something that code > >outside vfio shouldn't even be aware of. > > > >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. > > > >This interface still isn't great, but it's enough of an improvement to > >allow a number of cleanups in other places. > > > >Signed-off-by: David Gibson > >--- > > hw/vfio/common.c | 77 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > > include/hw/vfio/vfio.h | 2 ++ > > 2 files changed, 79 insertions(+) > > > >diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >index 607ec70..e419241 100644 > >--- a/hw/vfio/common.c > >+++ b/hw/vfio/common.c > >@@ -1003,3 +1003,80 @@ int vfio_container_ioctl(AddressSpace *as, int32_= t groupid, > > > > 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 */ > >+ > >+ if (!QLIST_EMPTY(&container->group_list) > >+ && QLIST_NEXT(QLIST_FIRST(&container->group_list), container_ne= xt)) { > >+ return false; > >+ } > >+ > >+ return true; >=20 > If &container->group_list is empty, this helper returns "true". Does not > look right, does it?... Hmm.. my thinking was that EEH was safe (if a no-op) on a container with no groups. But, thinking about it again I'm not sure that the state of previous EEH ops will get transferred to a group added to the container later. So I think returning false on an empty container probably is safer. I'll change it. > >+} > >+ > >+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); > >+ 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); > >+ > >+ return vfio_eeh_container_op(container, op); >=20 >=20 > vfio_eeh_as_ok() checks for (container !=3D NULL) but this one does not, > should not it? Well.. you're not supposed to call as_op() if as_ok() returned false, so it should be safe. I'll add an assert to make this clearer. --=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 --veXX9dWIonWZEC6h Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW07dGAAoJEGw4ysog2bOSs4UQALvs6XVI8dzE+QVRc7XvJjpF R1iai18mq8X8Q8TYOZLJiYdQY84pgVaoCmlwmoEqXFRXGZ2ZFfNjXYTlGBnQP7II 1D7JPUNwNdKdEOweUAk4+NxXQsKQ6ChrQBcSLm5oEoiKlJlg79u1YNG8arVaTnen NlYYbsBCMF5CFArAZm3mTcTZasKkCH6TtNGxwr/I+yueSBDKNOBWhwMcDnc1C+wX afckIjk1lsRpbbbV/eVWHZVhN1HRfxjDAy67rAMzXjnB3u700e+d6iC6kjrhDSDg 3wmzCPuo/rpM+WUKrvSPa4VIxlxBibdjj4rbwjfMe/5nOByiq8ujB21fkeCsW/Oa Jaj/HqBBch1SXdAPcAgkoGNy2uQOHpwe2//9+F9KIB6iRzLTRQsks6UM+0FW59Q6 5ZK/ZLVmWZ/OSiWpCttqlbJkD8ixgYNfq4EJYHlrMIEvOKDHyrsqax6DP1eaU1s+ t9RsW49Ed8bsGv5BIoP0yi+C9aLD/8QICyDmdpaeugL3SjnF6XipTw2adRrYqHJq E8sULQFffyHA9xys5C0D5bZiUhWGXStANXzyqBTHNhhF1SW8Qpiq1haHYFAkWgln ZldCTKPfjHE9+HskCHVHWhGjoSWz4cXzd9n5vyQBRq20FYx8+z6nYrjYbtJkA6lF LtwGIiKfUJTQSmMtxy9+ =62A3 -----END PGP SIGNATURE----- --veXX9dWIonWZEC6h--