Message ID | 1442647117-2726-3-git-send-email-david@gibson.dropbear.id.au |
---|---|
State | New |
Headers | show |
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; > }
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; +} + 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); } 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; }
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(-)