Re: [PATCH v10 01/11] vfio: VFIO_IOMMU_SET_PASID_TABLE

[Auger Eric]

Hi Zenghui,

On 9/23/20 1:27 PM, Zenghui Yu wrote:
> Hi Eric,
> 
> On 2020/3/21 0:19, Eric Auger wrote:
>> From: "Liu, Yi L" <yi.l.liu@xxxxxxxxxxxxxxx>
>>
>> This patch adds an VFIO_IOMMU_SET_PASID_TABLE ioctl
>> which aims to pass the virtual iommu guest configuration
>> to the host. This latter takes the form of the so-called
>> PASID table.
>>
>> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
>> Signed-off-by: Liu, Yi L <yi.l.liu@xxxxxxxxxxxxxxx>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> 
> [...]
> 
>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>> b/drivers/vfio/vfio_iommu_type1.c
>> index a177bf2c6683..bfacbd876ee1 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -2172,6 +2172,43 @@ static int vfio_iommu_iova_build_caps(struct
>> vfio_iommu *iommu,
>>       return ret;
>>   }
>>   +static void
>> +vfio_detach_pasid_table(struct vfio_iommu *iommu)
>> +{
>> +    struct vfio_domain *d;
>> +
>> +    mutex_lock(&iommu->lock);
>> +
>> +    list_for_each_entry(d, &iommu->domain_list, next) {
>> +        iommu_detach_pasid_table(d->domain);
>> +    }
>> +    mutex_unlock(&iommu->lock);
>> +}
>> +
>> +static int
>> +vfio_attach_pasid_table(struct vfio_iommu *iommu,
>> +            struct vfio_iommu_type1_set_pasid_table *ustruct)
>> +{
>> +    struct vfio_domain *d;
>> +    int ret = 0;
>> +
>> +    mutex_lock(&iommu->lock);
>> +
>> +    list_for_each_entry(d, &iommu->domain_list, next) {
>> +        ret = iommu_attach_pasid_table(d->domain, &ustruct->config);
>> +        if (ret)
>> +            goto unwind;
>> +    }
>> +    goto unlock;
>> +unwind:
>> +    list_for_each_entry_continue_reverse(d, &iommu->domain_list, next) {
>> +        iommu_detach_pasid_table(d->domain);
>> +    }
>> +unlock:
>> +    mutex_unlock(&iommu->lock);
>> +    return ret;
>> +}
>> +
>>   static long vfio_iommu_type1_ioctl(void *iommu_data,
>>                      unsigned int cmd, unsigned long arg)
>>   {
>> @@ -2276,6 +2313,25 @@ static long vfio_iommu_type1_ioctl(void
>> *iommu_data,
>>             return copy_to_user((void __user *)arg, &unmap, minsz) ?
>>               -EFAULT : 0;
>> +    } else if (cmd == VFIO_IOMMU_SET_PASID_TABLE) {
>> +        struct vfio_iommu_type1_set_pasid_table ustruct;
>> +
>> +        minsz = offsetofend(struct vfio_iommu_type1_set_pasid_table,
>> +                    config);
>> +
>> +        if (copy_from_user(&ustruct, (void __user *)arg, minsz))
>> +            return -EFAULT;
>> +
>> +        if (ustruct.argsz < minsz)
>> +            return -EINVAL;
>> +
>> +        if (ustruct.flags & VFIO_PASID_TABLE_FLAG_SET)
>> +            return vfio_attach_pasid_table(iommu, &ustruct);
>> +        else if (ustruct.flags & VFIO_PASID_TABLE_FLAG_UNSET) {
>> +            vfio_detach_pasid_table(iommu);
>> +            return 0;
>> +        } else
>> +            return -EINVAL;
> 
> Nit:
> 
> What if user-space blindly set both flags? Should we check that only one
> flag is allowed to be set at this stage, and return error otherwise?
Indeed I can check that.
> 
> Besides, before going through the whole series [1][2], I'd like to know
> if this is the latest version of your Nested-Stage-Setup work in case I
> had missed something.
> 
> [1] https://lore.kernel.org/r/20200320161911.27494-1-eric.auger@xxxxxxxxxx
> [2] https://lore.kernel.org/r/20200414150607.28488-1-eric.auger@xxxxxxxxxx

yes those 2 series are the last ones. Thank you for reviewing.

FYI, I intend to respin within a week or 2 on top of Jacob's  [PATCH v10
0/7] IOMMU user API enhancement. But functionally there won't a lot of
changes.

Thanks

Eric
> 
> 
> Thanks,
> Zenghui
>