From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44126) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaCB2-0004I0-LD for qemu-devel@nongnu.org; Sun, 28 Feb 2016 19:59:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaCAy-0002DO-64 for qemu-devel@nongnu.org; Sun, 28 Feb 2016 19:59:08 -0500 Received: from mail-pf0-x242.google.com ([2607:f8b0:400e:c00::242]:36051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaCAx-0002D5-RB for qemu-devel@nongnu.org; Sun, 28 Feb 2016 19:59:04 -0500 Received: by mail-pf0-x242.google.com with SMTP id q129so1840541pfb.3 for ; Sun, 28 Feb 2016 16:59:02 -0800 (PST) References: <1456486323-8047-1-git-send-email-david@gibson.dropbear.id.au> <1456486323-8047-2-git-send-email-david@gibson.dropbear.id.au> From: Alexey Kardashevskiy Message-ID: <56D397CE.5060906@ozlabs.ru> Date: Mon, 29 Feb 2016 11:58:54 +1100 MIME-Version: 1.0 In-Reply-To: <1456486323-8047-2-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit 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: David Gibson , benh@kernel.crashing.org Cc: agraf@suse.de, qemu-devel@nongnu.org, gwshan@au1.ibm.com, mdroth@linux.vnet.ibm.com, alex.williamson@redhat.com, qemu-ppc@nongnu.org On 02/26/2016 10:31 PM, David Gibson wrote: > At present the code handling IBM's Enhanced Error Handling (EEH) interface > 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 the > 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_next)) { > + return false; > + } > + > + return true; If &container->group_list is empty, this helper returns "true". Does not look right, does it?... > +} > + > +static int vfio_eeh_container_op(VFIOContainer *container, uint32_t op) > +{ > + struct vfio_eeh_pe_op pe_op = { > + .argsz = sizeof(pe_op), > + .op = 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 = 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 = vfio_get_address_space(as); > + VFIOContainer *container = NULL; > + > + if (QLIST_EMPTY(&space->containers)) { > + /* No containers to act on */ > + goto out; > + } > + > + container = QLIST_FIRST(&space->containers); > + > + if (QLIST_NEXT(container, next)) { > + /* We don't yet have logic to synchronize EEH state across > + * multiple containers */ > + container = NULL; > + goto out; > + } > + > +out: > + vfio_put_address_space(space); > + return container; > +} > + > +bool vfio_eeh_as_ok(AddressSpace *as) > +{ > + VFIOContainer *container = vfio_eeh_as_container(as); > + > + return (container != NULL) && vfio_eeh_container_ok(container); > +} > + > +int vfio_eeh_as_op(AddressSpace *as, uint32_t op) > +{ > + VFIOContainer *container = vfio_eeh_as_container(as); > + > + return vfio_eeh_container_op(container, op); vfio_eeh_as_ok() checks for (container != NULL) but this one does not, should not it? > +} > 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 @@ > > 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); > > #endif > -- Alexey