Re: [Qemu-devel] [RFC PATCH 01/14] vfio: Start adding VFIO/EEH interface

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5393 bytes --]

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) 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.
> > 
> > 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 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 assumes
> > there is only one VFIO group per container, which is no longer always the
> > case.  So, add logic to detect this case and warn.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  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) += common.o
> >  obj-$(CONFIG_PCI) += pci.o
> >  obj-$(CONFIG_SOFTMMU) += platform.o
> >  obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
> > +obj-$(CONFIG_PSERIES) += 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 <dgibson@redhat.com>
> > + *
> > + * 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 <http://www.gnu.org/licenses/>.
> > + *
> > + * Based on earlier EEH implementations:
> > + * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
> > + */
> > +#include <sys/ioctl.h>
> > +#include <linux/vfio.h>
> > +
> > +#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 = group->container;
> > +    struct vfio_eeh_pe_op pe_op = {
> > +        .argsz = sizeof(pe_op),
> > +        .op = op,
> > +    };
> > +    int ret;
> > +
> > +    if (!container) {
> > +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on unattached 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) != 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 that
case.

Perhaps EBUSY?  Since there isn't an ESHOULDWORKBUTDOESNT or EDEVBROKEN.

> 
> > +    }
> > +
> > +    ret = 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;
> 
> Would -errno be more interesting in the failure case?

Oh, yes.  Too much kernel work, I'm used to things returning the error
code.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]