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) 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 <da...@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 <dgib...@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.
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?
> > > + }
> > > +
> > > + 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.
>
