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 <da...@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. -- 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
signature.asc
Description: PGP signature