Re: [Qemu-devel] [PATCH qemu v8 03/14] spapr_pci_vfio: Enable multiple groups per container

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 06/26/2015 05:59 AM, Alex Williamson wrote:
> On Thu, 2015-06-18 at 21:37 +1000, Alexey Kardashevskiy wrote:
>> This enables multiple IOMMU groups in one VFIO container which means
>> that multiple devices from different groups can share the same IOMMU
>> table (or tables if DDW).
>>
>> This removes a group id from vfio_container_ioctl(). The kernel support
>> is required for this; if the host kernel does not have the support,
>> it will allow only one group per container. The PHB's "iommuid" property
>> is ignored. The ioctl is called for every container attached to
>> the address space. At the moment there is just one container anyway.
>>
>> If there is no container attached to the address space,
>> vfio_container_do_ioctl() returns -1.
>>
>> This removes casts to sPAPRPHBVFIOState as none of sPAPRPHBVFIOState
>> members is accessed here.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>   hw/ppc/spapr_pci_vfio.c | 21 ++++++---------------
>>   hw/vfio/common.c        | 20 ++++++--------------
>>   include/hw/vfio/vfio.h  |  2 +-
>>   3 files changed, 13 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
>> index 99a1be5..e89cbff 100644
>> --- a/hw/ppc/spapr_pci_vfio.c
>> +++ b/hw/ppc/spapr_pci_vfio.c
>> @@ -35,12 +35,7 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>>       sPAPRTCETable *tcet;
>>       uint32_t liobn = svphb->phb.dma_liobn;
>>
>> -    if (svphb->iommugroupid == -1) {
>> -        error_setg(errp, "Wrong IOMMU group ID %d", svphb->iommugroupid);
>> -        return;
>> -    }
>> -
>> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
>> +    ret = vfio_container_ioctl(&svphb->phb.iommu_as,
>>                                  VFIO_CHECK_EXTENSION,
>>                                  (void *) VFIO_SPAPR_TCE_IOMMU);
>>       if (ret != 1) {
>> @@ -49,7 +44,7 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>>           return;
>>       }
>>
>> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
>> +    ret = vfio_container_ioctl(&sphb->iommu_as,
>>                                  VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
>>       if (ret) {
>>           error_setg_errno(errp, -ret,
>> @@ -79,7 +74,6 @@ 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) };
>>       int ret;
>>
>> @@ -116,7 +110,7 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
>>           return RTAS_OUT_PARAM_ERROR;
>>       }
>>
>> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
>> +    ret = vfio_container_ioctl(&sphb->iommu_as,
>>                                  VFIO_EEH_PE_OP, &op);
>>       if (ret < 0) {
>>           return RTAS_OUT_HW_ERROR;
>> @@ -127,12 +121,11 @@ 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) };
>>       int ret;
>>
>>       op.op = VFIO_EEH_PE_GET_STATE;
>> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
>> +    ret = vfio_container_ioctl(&sphb->iommu_as,
>>                                  VFIO_EEH_PE_OP, &op);
>>       if (ret < 0) {
>>           return RTAS_OUT_PARAM_ERROR;
>> @@ -144,7 +137,6 @@ static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
>>
>>   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) };
>>       int ret;
>>
>> @@ -162,7 +154,7 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
>>           return RTAS_OUT_PARAM_ERROR;
>>       }
>>
>> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
>> +    ret = vfio_container_ioctl(&sphb->iommu_as,
>>                                  VFIO_EEH_PE_OP, &op);
>>       if (ret < 0) {
>>           return RTAS_OUT_HW_ERROR;
>> @@ -173,12 +165,11 @@ 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) };
>>       int ret;
>>
>>       op.op = VFIO_EEH_PE_CONFIGURE;
>> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
>> +    ret = vfio_container_ioctl(&sphb->iommu_as,
>>                                  VFIO_EEH_PE_OP, &op);
>>       if (ret < 0) {
>>           return RTAS_OUT_PARAM_ERROR;
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 3e4c685..369e564 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -793,34 +793,26 @@ void vfio_put_base_device(VFIODevice *vbasedev)
>>       close(vbasedev->fd);
>>   }
>>
>> -static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid,
>> +static int vfio_container_do_ioctl(AddressSpace *as,
>>                                      int req, void *param)
>>   {
>> -    VFIOGroup *group;
>>       VFIOContainer *container;
>>       int ret = -1;
>> +    VFIOAddressSpace *space = vfio_get_address_space(as);
>>
>> -    group = vfio_get_group(groupid, as);
>> -    if (!group) {
>> -        error_report("vfio: group %d not registered", groupid);
>> -        return ret;
>> -    }
>> -
>> -    container = group->container;
>> -    if (group->container) {
>> +    QLIST_FOREACH(container, &space->containers, next) {
>>           ret = ioctl(container->fd, req, param);
>>           if (ret < 0) {
>>               error_report("vfio: failed to ioctl %d to container: ret=%d, %s",
>>                            _IOC_NR(req) - VFIO_BASE, ret, strerror(errno));
>> +            return -errno;
>>           }
>>       }
>
>
> How does this work with VFIO_EEH_PE_OP?  Suddenly we're potentially
> calling it on multiple containers?

Gavin says yes.


> Should all of this container ioctl
> code also be pulled out to spapr?

Well, I could but I won't as I am dropping 2/14 at all.


> There's clearly an approach here where the patches could be split into
> separate ppc and vfio series rather than this horrible practice of
> intermixing them.  Thanks,

Oh. Ok. I'll move 3/14 further. I just wanted to put 2/14 and 3/14 earlier 
as it allowed me to avoid changing same lines twice in the same patchset.

>
> Alex
>>
>> -    vfio_put_group(group);
>> -
>>       return ret;
>>   }
>>
>> -int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>> +int vfio_container_ioctl(AddressSpace *as,
>>                            int req, void *param)
>>   {
>>       /* We allow only certain ioctls to the container */
>> @@ -835,5 +827,5 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>>           return -1;
>>       }
>>
>> -    return vfio_container_do_ioctl(as, groupid, req, param);
>> +    return vfio_container_do_ioctl(as, req, param);
>>   }
>> diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h
>> index 0b26cd8..76b5744 100644
>> --- a/include/hw/vfio/vfio.h
>> +++ b/include/hw/vfio/vfio.h
>> @@ -3,7 +3,7 @@
>>
>>   #include "qemu/typedefs.h"
>>
>> -extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>> +extern int vfio_container_ioctl(AddressSpace *as,
>>                                   int req, void *param);
>>
>>   #endif
>
>
>


-- 
Alexey