On Thu, 2015-11-19 at 15:29 +1100, 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 6797208..4733625 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1002,3 +1002,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;
> +}
Seems like there are ways to make this a non-eeh specific function,
vfio_container_group_count(), vfio_container_group_empty_or_singleton(),
etc.
> +
> +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;
> +}
Here too, vfio_container_from_as()
Overall the series looks good to me, nice cleanup both in power and vfio
code. Thanks,
Alex
> +
> +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
