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

[Thomas Huth]

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) 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 <address@hidden>
>>>>> ---
>>>>>  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 <address@hidden>
>>>>> + *
>>>>> + * 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?
> 
> 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-Codes.html 
:
"Not supported. A function returns this error when certain parameter values
 are valid, but the functionality they request is not available."

Thomas

signature.asc
Description: OpenPGP digital signature