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 <da...@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;
> }
