Re: [RFC v3 04/21] vfio: VFIO_IOMMU_SET_PASID_TABLE

[Auger Eric]

Hi Alex,

On 1/11/19 11:50 PM, Alex Williamson wrote:
> On Tue,  8 Jan 2019 11:26:16 +0100
> Eric Auger <eric.auger@xxxxxxxxxx> wrote:
> 
>> From: "Liu, Yi L" <yi.l.liu@xxxxxxxxxxxxxxx>
>>
>> This patch adds VFIO_IOMMU_SET_PASID_TABLE ioctl which aims at
>> passing the virtual iommu guest configuration to the VFIO driver
>> downto to the iommu subsystem.
>>
>> 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>
>>
>> ---
>> v2 -> v3:
>> - s/BIND_PASID_TABLE/SET_PASID_TABLE
>>
>> v1 -> v2:
>> - s/BIND_GUEST_STAGE/BIND_PASID_TABLE
>> - remove the struct device arg
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 31 +++++++++++++++++++++++++++++++
>>  include/uapi/linux/vfio.h       |  8 ++++++++
>>  2 files changed, 39 insertions(+)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 7651cfb14836..d9dd23f64f00 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -1644,6 +1644,24 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
>>  	return ret;
>>  }
>>  
>> +static int
>> +vfio_set_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_set_pasid_table(d->domain, &ustruct->config);
>> +		if (ret)
>> +			break;
>> +	}
> 
> There's no unwind on failure here, leaves us in an inconsistent state
> should something go wrong or domains don't have homogeneous PASID
> support.  What's expected to happen if a PASID table is already set for
> a domain, does it replace the old one or return -EBUSY?
Effectively the setting can succeed on one domain and fail on another
one, typically if one underlying SMMU does not support nested while the
others support it.

At the moment setting a new PASID table whereas one is installed does
not return any error in the SMMU code, just overwrite the associated
fields in the stream table entry. This is also the way we turn nested
off (cfg->bypass == true). Needs to be clarified anyway.
> 
>> +	mutex_unlock(&iommu->lock);
>> +	return ret;
>> +}
>> +
>>  static long vfio_iommu_type1_ioctl(void *iommu_data,
>>  				   unsigned int cmd, unsigned long arg)
>>  {
>> @@ -1714,6 +1732,19 @@ 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 || ustruct.flags)
>> +			return -EINVAL;
>> +
>> +		return vfio_set_pasid_table(iommu, &ustruct);
>>  	}
>>  
>>  	return -ENOTTY;
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 02bb7ad6e986..0d9f4090c95d 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -14,6 +14,7 @@
>>  
>>  #include <linux/types.h>
>>  #include <linux/ioctl.h>
>> +#include <linux/iommu.h>
>>  
>>  #define VFIO_API_VERSION	0
>>  
>> @@ -759,6 +760,13 @@ struct vfio_iommu_type1_dma_unmap {
>>  #define VFIO_IOMMU_ENABLE	_IO(VFIO_TYPE, VFIO_BASE + 15)
>>  #define VFIO_IOMMU_DISABLE	_IO(VFIO_TYPE, VFIO_BASE + 16)
>>  
>> +struct vfio_iommu_type1_set_pasid_table {
>> +	__u32	argsz;
>> +	__u32	flags;
>> +	struct iommu_pasid_table_config config;
>> +};
>> +#define VFIO_IOMMU_SET_PASID_TABLE	_IO(VFIO_TYPE, VFIO_BASE + 22)
> 
> -ENOCOMMENTS  Thanks,
sure

Thanks

Eric
> 
> Alex
> 
>> +
>>  /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
>>  
>>  /*
>