From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41807) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZevfM-0007xg-Ro for qemu-devel@nongnu.org; Wed, 23 Sep 2015 21:49:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZevfL-0002tG-Dw for qemu-devel@nongnu.org; Wed, 23 Sep 2015 21:49:44 -0400 Date: Thu, 24 Sep 2015 11:11:27 +1000 From: David Gibson Message-ID: <20150924011127.GK15944@voom.fritz.box> References: <1442647117-2726-1-git-send-email-david@gibson.dropbear.id.au> <1442647117-2726-2-git-send-email-david@gibson.dropbear.id.au> <1443029309.23936.483.camel@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Z9t8O/5YJLB6LEUl" Content-Disposition: inline In-Reply-To: <1443029309.23936.483.camel@redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH 01/14] vfio: Start adding VFIO/EEH interface 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 --Z9t8O/5YJLB6LEUl Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 23, 2015 at 11:28:29AM -0600, Alex Williamson wrote: > On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote: > > 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 > > As a first step to cleaning that up, start creating a new VFIO interface > > for EEH operations. Because EEH operates in units of a "Partitionable > > Endpoint" (PE) - a group of devices that can't be mutually isolated), it > > needs to expose host PEs (that is, IOMMU groups) to the guest. This me= ans > > EEH needs VFIO concepts exposed that other VFIO users don't. > >=20 > > At present all EEH ioctl()s in use operate conceptually on a single PE = and > > take no parameters except the opcode itself. So, expose a vfio_eeh_op() > > function to apply a single no-argument operation to a VFIO group. > >=20 > > At present the kernel VFIO/EEH interface is broken, because it assumes > > there is only one VFIO group per container, which is no longer always t= he > > case. So, add logic to detect this case and warn. > >=20 > > Signed-off-by: David Gibson > > --- > > hw/vfio/Makefile.objs | 1 + > > hw/vfio/eeh.c | 64 ++++++++++++++++++++++++++++++++++++++= ++++++++ > > include/hw/vfio/vfio-eeh.h | 42 ++++++++++++++++++++++++++++++ > > 3 files changed, 107 insertions(+) > > create mode 100644 hw/vfio/eeh.c > > create mode 100644 include/hw/vfio/vfio-eeh.h > >=20 > > diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs > > index d540c9d..384c702 100644 > > --- a/hw/vfio/Makefile.objs > > +++ b/hw/vfio/Makefile.objs > > @@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) +=3D common.o > > obj-$(CONFIG_PCI) +=3D pci.o > > obj-$(CONFIG_SOFTMMU) +=3D platform.o > > obj-$(CONFIG_SOFTMMU) +=3D calxeda-xgmac.o > > +obj-$(CONFIG_PSERIES) +=3D eeh.o > > endif > > diff --git a/hw/vfio/eeh.c b/hw/vfio/eeh.c > > new file mode 100644 > > index 0000000..35bd06c > > --- /dev/null > > +++ b/hw/vfio/eeh.c > > @@ -0,0 +1,64 @@ > > +/* > > + * EEH (IBM Enhanced Error Handling) functions for VFIO devices > > + * > > + * Copyright Red Hat, Inc. 2015 > > + * > > + * Authors: > > + * David Gibson > > + * > > + * This program is free software: you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation, either version 2 of the > > + * License, or (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see . > > + * > > + * Based on earlier EEH implementations: > > + * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation. > > + */ > > +#include > > +#include > > + > > +#include "qemu/error-report.h" > > + > > +#include "hw/vfio/vfio-common.h" > > +#include "hw/vfio/vfio-eeh.h" > > + > > +int vfio_eeh_op(VFIOGroup *group, uint32_t op) > > +{ > > + VFIOContainer *container =3D group->container; > > + struct vfio_eeh_pe_op pe_op =3D { > > + .argsz =3D sizeof(pe_op), > > + .op =3D op, > > + }; > > + int ret; > > + > > + if (!container) { > > + error_report("vfio/eeh: EEH_PE_OP 0x%x called on unattached gr= oup %d", > > + op, group->groupid); > > + return -ENODEV; > > + } > > + > > + /* A broken kernel interface means EEH operations can't work > > + * correctly if there are multiple groups in a container */ > > + if ((QLIST_FIRST(&container->group_list) !=3D group) > > + || QLIST_NEXT(group, container_next)) { > > + error_report("vfio/eeh: EEH_PE_OP 0x%x called on container" > > + " with multiple groups", op); > > + return -ENOSPC; >=20 > -EINVAL really seems more appropriate So, I agree that ENOSPC is a dubious choice, but EINVAL is definitely wrong here. Broad as it is, EINVAL should always indicate that the caller has supplied some sort of bad parameter. In this case the parameters are just fine, it's just that the kernel is broken so we can't handle that case. Perhaps EBUSY? Since there isn't an ESHOULDWORKBUTDOESNT or EDEVBROKEN. >=20 > > + } > > + > > + ret =3D ioctl(container->fd, VFIO_EEH_PE_OP, &pe_op); > > + if (ret < 0) { > > + error_report("vfio/eeh: EEH_PE_OP 0x%x failed on group %d: %m", > > + op, group->groupid); > > + } > > + > > + return ret; >=20 > Would -errno be more interesting in the failure case? Oh, yes. Too much kernel work, I'm used to things returning the error code. --=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 --Z9t8O/5YJLB6LEUl Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWA02/AAoJEGw4ysog2bOSSOgP/jMJ5Vgh3BtcKUx3aux6jxMe 6fYkQ8/R1GI+DJFUVjrszud0NowlmTvrigphgTGatGxJam3NICEDW4+HdYVmno16 0+OICq6R6R1s8zlFMwR/RiUw4US1sYC0GaSinSZlWKmHvfwRgNqTRRm0/ydVn+Zs QqlQ7YSyyzmm1/N+SBEun/+wTSRuEbMzagoe1BllFQJi4OZbdNOyyxTR5CLnbTC/ tcEFNkMat6L/iWTwiJYOL+cQB7tklN5TwH6P15T1iM+1qdSbbyVC6vIfsLSPpPL0 V7NG+kcRGADj0aIwoXifYKCSbPTg70BGpkp80HwznwtwLJxBns7oHXlj8vr/RQ84 inVLkKXcYGSmqFo6RI231SqKDWXaclQkfj1QOZFPD2iOKhr/qv2twNEE9tyJmUjW WK94GBbVNYfuVEc+r9WncPwR+0cLcJVimgyEw3ETuUa9rFV2yZ+N84vGXyo5/1Pp TkVNaWQm3ukWj0Wt3ojxRsooQcyHSWrwZJD/zlozMm/2ElJ1i/sw3W0ypy8MnOQ9 smOY1e3R73dS/UB8ofSGuI/d7Ml0UPvhyirFThLXHD9kpYu7/OgdH5MHtMy+roXJ RtDrQlH254ZXZ58UVqDYfRJRXDLLQS6r1JMjtBfXxfjSIyPcIdOO6H+jgBVSSCyC ZaaYXsuamLScWJngjP52 =qtIZ -----END PGP SIGNATURE----- --Z9t8O/5YJLB6LEUl--