| Message ID | 1456486323-8047-2-git-send-email-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 --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
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(+)