From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43926) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zext9-0000VW-HO for qemu-devel@nongnu.org; Thu, 24 Sep 2015 00:12:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zext5-0003FQ-TR for qemu-devel@nongnu.org; Thu, 24 Sep 2015 00:12:07 -0400 Date: Thu, 24 Sep 2015 14:09:50 +1000 From: David Gibson Message-ID: <20150924040950.GN15944@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> <20150924011127.GK15944@voom.fritz.box> <1443060750.23936.517.camel@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="tpyx7gKuSYt+mjHM" Content-Disposition: inline In-Reply-To: <1443060750.23936.517.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 --tpyx7gKuSYt+mjHM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 23, 2015 at 08:12:30PM -0600, Alex Williamson wrote: > On Thu, 2015-09-24 at 11:11 +1000, David Gibson wrote: > > 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) in= terface > > > > on VFIO devices operates by bypassing the usual VFIO logic with > > > > vfio_container_ioctl(). That's a poorly designed interface with un= clear > > > > semantics about exactly what can be operated on. > > > >=20 > > > > As a first step to cleaning that up, start creating a new VFIO inte= rface > > > > for EEH operations. Because EEH operates in units of a "Partitiona= ble > > > > 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. Thi= s means > > > > 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 assu= mes > > > > there is only one VFIO group per container, which is no longer alwa= ys the > > > > 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 Licen= se > > > > + * 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 unattache= d group %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 > >=20 > > So, I agree that ENOSPC is a dubious choice, but EINVAL is definitely > > wrong here. > >=20 > > 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. > >=20 > > Perhaps EBUSY? Since there isn't an ESHOULDWORKBUTDOESNT or EDEVBROKEN. >=20 > The caller has supplied a bad parameter, a group that doesn't match the > existing group in the container. If you really object, EPERM? EACCES? Well, I suppose, but the parameter is only bad because the implementation is flawed, not because the user has requested something actually impossible. EACCES is definitely wrong, since that should refer to a problem with (settable) permissions. EPERM.. I guess I can live with, I'll go with that. --=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 --tpyx7gKuSYt+mjHM Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIbBAEBAgAGBQJWA3eOAAoJEGw4ysog2bOSqakP9iSpCAHRiPdsD2Y/W/aEXDb5 5zYwYjzC6SpN1AdurzyryZZGHBBa5sYz5QJw+EmgCTxNRH7svmDUSL5LBq9ey5yx ujZ0X3w9zNHHCVoSre/wXwMadCBVWmg6dl9eY6+9pU8HJjInlIFsS56QGq9pcmlq /lXkfDyw2HyHjtefeLx15JpnGx5CWyGrJnADjxbn02VDCzO1PRj6ibdv+NBO9nAI x4PnOGsAoQemCKat5eyg7cXfBNFD41nFSHHLCcnGKmxfOTftjRNdWlSK4dFjzOng 53oElSlfl+SZ20vTND4QsOPuT44+BMlhwsecAT/8c2m/x8dcFCKlkpMnF5w+TZ3m vP9YVRUPn2auqIvVlRubKyC4DOSN+SGslUqT4CFx7AVBPJal9J6GgNvh8Z/NNFPJ /vBPa3NEqTuCF0iFC1npXux3w0n+Eywz++Y3sF4w8efLpLOL5qRnSX0x0YlH0rFl v4m9ABJ9KySFW7oIBdjvynAQuVABDXxjXtjuZbXdM5fpU7gXw/ozbD2/3KooLYna LwDQF5joa0Us6zRYaYFKk0Xvaq6LMJ3lL4RP6RWLwCZUlahoWjk/Xm7uapmeZT9x MgfK6lkEUIRdrfrffHveAkuPSsYSnk9PGKokxGAidSlLy+djBdozhGRIprNxlzyB o4N1glAsrZhC5HDwG0s= =5wBM -----END PGP SIGNATURE----- --tpyx7gKuSYt+mjHM--