diff mbox

[01/12] vfio: Start improving VFIO/EEH interface

Message ID 1456486323-8047-2-git-send-email-david@gibson.dropbear.id.au
State New, archived
Headers show

Commit Message

David Gibson Feb. 26, 2016, 11:31 a.m. UTC
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 <david@gibson.dropbear.id.au>
---
 hw/vfio/common.c       | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio.h |  2 ++
 2 files changed, 79 insertions(+)

Comments

Alexey Kardashevskiy Feb. 29, 2016, 12:58 a.m. UTC | #1
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 <david@gibson.dropbear.id.au>
> ---
>   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
>
David Gibson Feb. 29, 2016, 3:13 a.m. UTC | #2
On Mon, Feb 29, 2016 at 11:58:54AM +1100, Alexey Kardashevskiy wrote:
> 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 <david@gibson.dropbear.id.au>
> >---
> >  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?...

Hmm.. my thinking was that EEH was safe (if a no-op) on a container
with no groups.  But, thinking about it again I'm not sure that the
state of previous EEH ops will get transferred to a group added to the
container later.  So I think returning false on an empty container
probably is safer.

I'll change 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?

Well.. you're not supposed to call as_op() if as_ok() returned false,
so it should be safe.  I'll add an assert to make this clearer.
diff mbox

Patch

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;
+}
+
+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);
+}
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