Re: [Qemu-devel] [RFC PATCH 02/14] spapr_pci: Switch EEH to vfio_eeh_op() interface

[Alex Williamson <alex.williamson@redhat.com>]

On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> This switches all EEH on VFIO operations in spapr_pci_vfio() from the old
> broken vfio_container_ioctl() interface to the new vfio_eeh_op() interface.
> 
> In order to obtain the VFIOGroup * handle that vfio_eeh_op() requires, we
> add a helper function spapr_phb_check_vfio_group().  It gets the group from
> the VFIO PHB's groupid by calling into vfio_get_group().  Using that
> outside the VFIO is an abstraction violation, but so was
> vfio_container_ioctl() itself, and this is a useful intermediate step to
> cleaning it up properly.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci_vfio.c | 90 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 59 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index a61b418..a2b1093 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -22,12 +22,27 @@
>  #include "hw/pci/msix.h"
>  #include "linux/vfio.h"
>  #include "hw/vfio/vfio.h"
> +#include "hw/vfio/vfio-eeh.h"
> +
> +#include "hw/vfio/vfio-common.h"
>  
>  static Property spapr_phb_vfio_properties[] = {
>      DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> +{
> +    VFIOGroup *group;
> +
> +    /* FIXME: this is an abstraction violation */
> +    group = vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugroupid,
> +                           &phb->iommu_as);
> +    if (gpp)
> +        *gpp = group;
> +    return RTAS_OUT_SUCCESS;
> +}

"check" for what?  Why does this have a return value if it can't fail?
This is a rather obscure wrapper of vfio_get_group().  It also seems
like yet another abstraction violation to make such a simply function
return RTAS error codes.

> +
>  static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>  {
>      sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> @@ -74,13 +89,14 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>  
>  static void spapr_phb_vfio_eeh_reenable(sPAPRPHBVFIOState *svphb)
>  {
> -    struct vfio_eeh_pe_op op = {
> -        .argsz = sizeof(op),
> -        .op    = VFIO_EEH_PE_ENABLE
> -    };
> +    VFIOGroup *group;
> +    int ret;
>  
> -    vfio_container_ioctl(&svphb->phb.iommu_as,
> -                         svphb->iommugroupid, VFIO_EEH_PE_OP, &op);
> +    ret = spapr_phb_check_vfio_group(SPAPR_PCI_HOST_BRIDGE(svphb), &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return;
> +    }
> +    vfio_eeh_op(group, VFIO_EEH_PE_ENABLE);

What did we just accomplish?  Is group valid?

>  }
>  
>  static void spapr_phb_vfio_reset(DeviceState *qdev)
> @@ -97,13 +113,18 @@ static void spapr_phb_vfio_reset(DeviceState *qdev)
>  static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
>                                           unsigned int addr, int option)
>  {
> -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> -    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> +    VFIOGroup *group;
> +    uint32_t op;
>      int ret;
>  
> +    ret = spapr_phb_check_vfio_group(sphb, &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return ret;
> +    }
> +
>      switch (option) {
>      case RTAS_EEH_DISABLE:
> -        op.op = VFIO_EEH_PE_DISABLE;
> +        op = VFIO_EEH_PE_DISABLE;
>          break;
>      case RTAS_EEH_ENABLE: {
>          PCIHostState *phb;
> @@ -121,21 +142,20 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
>              return RTAS_OUT_PARAM_ERROR;
>          }
>  
> -        op.op = VFIO_EEH_PE_ENABLE;
> +        op = VFIO_EEH_PE_ENABLE;
>          break;
>      }
>      case RTAS_EEH_THAW_IO:
> -        op.op = VFIO_EEH_PE_UNFREEZE_IO;
> +        op = VFIO_EEH_PE_UNFREEZE_IO;
>          break;
>      case RTAS_EEH_THAW_DMA:
> -        op.op = VFIO_EEH_PE_UNFREEZE_DMA;
> +        op = VFIO_EEH_PE_UNFREEZE_DMA;
>          break;
>      default:
>          return RTAS_OUT_PARAM_ERROR;
>      }
>  
> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> -                               VFIO_EEH_PE_OP, &op);
> +    ret = vfio_eeh_op(group, op);
>      if (ret < 0) {
>          return RTAS_OUT_HW_ERROR;
>      }
> @@ -145,13 +165,15 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
>  
>  static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
>  {
> -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> -    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> +    VFIOGroup *group;
>      int ret;
>  
> -    op.op = VFIO_EEH_PE_GET_STATE;
> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> -                               VFIO_EEH_PE_OP, &op);
> +    ret = spapr_phb_check_vfio_group(sphb, &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return ret;
> +    }
> +
> +    ret = vfio_eeh_op(group, VFIO_EEH_PE_GET_STATE);
>      if (ret < 0) {
>          return RTAS_OUT_PARAM_ERROR;
>      }
> @@ -205,28 +227,32 @@ static void spapr_phb_vfio_eeh_pre_reset(sPAPRPHBState *sphb)
>  
>  static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
>  {
> -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> -    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> +    VFIOGroup *group;
> +    uint32_t op;
>      int ret;
>  
> +    ret = spapr_phb_check_vfio_group(sphb, &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return ret;
> +    }
> +
>      switch (option) {
>      case RTAS_SLOT_RESET_DEACTIVATE:
> -        op.op = VFIO_EEH_PE_RESET_DEACTIVATE;
> +        op = VFIO_EEH_PE_RESET_DEACTIVATE;
>          break;
>      case RTAS_SLOT_RESET_HOT:
>          spapr_phb_vfio_eeh_pre_reset(sphb);
> -        op.op = VFIO_EEH_PE_RESET_HOT;
> +        op = VFIO_EEH_PE_RESET_HOT;
>          break;
>      case RTAS_SLOT_RESET_FUNDAMENTAL:
>          spapr_phb_vfio_eeh_pre_reset(sphb);
> -        op.op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
> +        op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
>          break;
>      default:
>          return RTAS_OUT_PARAM_ERROR;
>      }
>  
> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> -                               VFIO_EEH_PE_OP, &op);
> +    ret = vfio_eeh_op(group, op);
>      if (ret < 0) {
>          return RTAS_OUT_HW_ERROR;
>      }
> @@ -236,13 +262,15 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
>  
>  static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
>  {
> -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> -    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> +    VFIOGroup *group;
>      int ret;
>  
> -    op.op = VFIO_EEH_PE_CONFIGURE;
> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> -                               VFIO_EEH_PE_OP, &op);
> +    ret = spapr_phb_check_vfio_group(sphb, &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return ret;
> +    }
> +
> +    ret = vfio_eeh_op(group, VFIO_EEH_PE_CONFIGURE);
>      if (ret < 0) {
>          return RTAS_OUT_PARAM_ERROR;
>      }