From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37492) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZezLi-0001jx-CA for qemu-devel@nongnu.org; Thu, 24 Sep 2015 01:45:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZezLe-00056U-Ox for qemu-devel@nongnu.org; Thu, 24 Sep 2015 01:45:42 -0400 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> <20150924040950.GN15944@voom.fritz.box> From: Thomas Huth Message-ID: <56038DFC.2050500@redhat.com> Date: Thu, 24 Sep 2015 07:45:32 +0200 MIME-Version: 1.0 In-Reply-To: <20150924040950.GN15944@voom.fritz.box> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="DEAULVEaHKgwrUugv6aArbUnUJ6IqUNCe" 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: David Gibson , Alex Williamson Cc: lvivier@redhat.com, mdroth@linux.vnet.ibm.com, aik@ozlabs.ru, qemu-devel@nongnu.org, gwshan@linux.vnet.ibm.com, qemu-ppc@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --DEAULVEaHKgwrUugv6aArbUnUJ6IqUNCe Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 24/09/15 06:09, David Gibson wrote: > 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. >>>>> >>>>> 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. >>>>> >>>>> 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. >>>>> >>>>> 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. >>>>> >>>>> 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 >>>>> >>>>> 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; >>>> >>>> -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 tha= t >>> case. >>> >>> Perhaps EBUSY? Since there isn't an ESHOULDWORKBUTDOESNT or EDEVBROK= EN. >> >> The caller has supplied a bad parameter, a group that doesn't match th= e >> existing group in the container. If you really object, EPERM? EACCES= ? >=20 > 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. What about ENOTSUP ? According to http://www.gnu.org/software/libc/manual/html_node/Error-Code= s.html : "Not supported. A function returns this error when certain parameter valu= es are valid, but the functionality they request is not available." Thomas --DEAULVEaHKgwrUugv6aArbUnUJ6IqUNCe Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJWA438AAoJEC7Z13T+cC211loP/RDu4diXMp9HFe6Gz+ARSuQw vX7yd9JqQDXsih+mNA+pmOPFb/56MjMfa1qiaSZh/GrJb/hpnVejR6mhZ9JThA3I GlExncthcIJa4IjNzcyzbgn/ydQUqBI0icve942aC8gGdTUQZDxSZx+/aL9WTSW3 JlSLUDQ6ABsFlVBqXSG0TtijurhVJXPGwNPB0WBLrFLLamuFVlEySQG7Dtd1FP+7 ZWMP2c7rkxMiRchnBAqHWuCayz7ZWR9519EJEqbI9OPEJgTJMOMevrrdwIq+f8ls tC+42iyB4NbHKQPoU0zVtG603sflkUCtuhxqkuobNPkKH2OA9hxGt45xZbCv6m3a uZFzpmLUPi1DoSZGCIe7DKa29ld5kFDQRucTmEKEvf60IJnjgEDdPA/ni3bDjtPi b13BUS2+Jdxy5Y5KbtVATPP1Hht3unCykfxAtfCYgoA+RH6313IRb2MUtlQ/+uuT TfjoAdJJKURP5m31uFaQkK6e6RPxpmreGYKY6cUj8gXZkrsAeip3f0OseJNPQa4T 0A9hy8gVOY9vtUqRKsu4zob7z73m0ey5ni6DrngfK+0p4Gw9impcGXXW5wz9S9wo ham2JNAPHKxEUq7kICKO1bXnFSCK8eWobDN4coYfJdNhoSUDa9IjcuyrfLQNqsNb mhfW4Nw7gE7tZYvTyQBa =pByV -----END PGP SIGNATURE----- --DEAULVEaHKgwrUugv6aArbUnUJ6IqUNCe--