[v10,01/11] vfio: VFIO_IOMMU_SET_PASID_TABLE

Commit Message

Eric Auger March 20, 2020, 4:19 p.m. UTC

From: "Liu, Yi L" <[e-mail]>

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 <[e-mail]>
Signed-off-by: Liu, Yi L <[e-mail]>
Signed-off-by: Eric Auger <[e-mail]>

---
v8 -> v9:
- Merge VFIO_IOMMU_ATTACH/DETACH_PASID_TABLE into a single
  VFIO_IOMMU_SET_PASID_TABLE ioctl.

v6 -> v7:
- add a comment related to VFIO_IOMMU_DETACH_PASID_TABLE

v3 -> v4:
- restore ATTACH/DETACH
- add unwind on failure

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 | 56 +++++++++++++++++++++++++++++++++
 include/uapi/linux/vfio.h       | 19 +++++++++++
 2 files changed, 75 insertions(+)

Comments

kernel test robot March 21, 2020, 5:18 a.m. UTC | #1

Hi Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on vfio/next]
[also build test ERROR on v5.6-rc6 next-20200320]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Eric-Auger/SMMUv3-Nested-Stage-Setup-VFIO-part/20200321-040935
base:   https://github.com/awilliam/linux-vfio.git next
config: arm64-randconfig-a001-20200321 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.2.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[e-mail]>

All errors (new ones prefixed by >>):

   In file included from include/linux/vfio.h:16,
                    from drivers/vfio/vfio.c:32:
>> include/uapi/linux/vfio.h:811:34: error: field 'config' has incomplete type
     811 |  struct iommu_pasid_table_config config; /* used on SET */
         |                                  ^~~~~~
--
   In file included from include/linux/vfio.h:16,
                    from drivers/vfio/vfio_iommu_type1.c:35:
>> include/uapi/linux/vfio.h:811:34: error: field 'config' has incomplete type
     811 |  struct iommu_pasid_table_config config; /* used on SET */
         |                                  ^~~~~~
   drivers/vfio/vfio_iommu_type1.c: In function 'vfio_detach_pasid_table':
>> drivers/vfio/vfio_iommu_type1.c:2204:3: error: implicit declaration of function 'iommu_detach_pasid_table'; did you mean 'vfio_detach_pasid_table'? [-Werror=implicit-function-declaration]
    2204 |   iommu_detach_pasid_table(d->domain);
         |   ^~~~~~~~~~~~~~~~~~~~~~~~
         |   vfio_detach_pasid_table
   drivers/vfio/vfio_iommu_type1.c: In function 'vfio_attach_pasid_table':
>> drivers/vfio/vfio_iommu_type1.c:2219:9: error: implicit declaration of function 'iommu_attach_pasid_table'; did you mean 'vfio_attach_pasid_table'? [-Werror=implicit-function-declaration]
    2219 |   ret = iommu_attach_pasid_table(d->domain, &ustruct->config);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~
         |         vfio_attach_pasid_table
   cc1: some warnings being treated as errors

vim +/config +811 include/uapi/linux/vfio.h

   797	
   798	/**
   799	 * VFIO_IOMMU_SET_PASID_TABLE - _IOWR(VFIO_TYPE, VFIO_BASE + 22,
   800	 *			struct vfio_iommu_type1_set_pasid_table)
   801	 *
   802	 * The SET operation passes a PASID table to the host while the
   803	 * UNSET operation detaches the one currently programmed. Setting
   804	 * a table while another is already programmed replaces the old table.
   805	 */
   806	struct vfio_iommu_type1_set_pasid_table {
   807		__u32	argsz;
   808		__u32	flags;
   809	#define VFIO_PASID_TABLE_FLAG_SET	(1 << 0)
   810	#define VFIO_PASID_TABLE_FLAG_UNSET	(1 << 1)
 > 811		struct iommu_pasid_table_config config; /* used on SET */
   812	};
   813	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Zenghui Yu Sept. 23, 2020, 11:27 a.m. UTC | #2

Hi Eric,

On 2020/3/21 0:19, Eric Auger wrote:
> From: "Liu, Yi L" <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Liu, Yi L <[e-mail]>
> Signed-off-by: Eric Auger <[e-mail]>

[...]

> 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?

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/[e-mail]
[2] https://lore.kernel.org/r/[e-mail]


Thanks,
Zenghui

Eric Auger Sept. 23, 2020, 11:47 a.m. UTC | #3

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" <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Liu, Yi L <[e-mail]>
>> Signed-off-by: Eric Auger <[e-mail]>
> 
> [...]
> 
>> 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/[e-mail]
> [2] https://lore.kernel.org/r/[e-mail]

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
>

Shameerali Kolothum Thodi Oct. 27, 2020, 12:20 p.m. UTC | #4

Hi Eric,

> -----Original Message-----
> From: iommu [mailto:[e-mail]] On Behalf Of
> Auger Eric
> Sent: 23 September 2020 12:47
> To: yuzenghui <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v10 01/11] vfio: VFIO_IOMMU_SET_PASID_TABLE

...

> > 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/[e-mail]
> > [2]
> > https://lore.kernel.org/r/[e-mail]
> 
> 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. 

Thanks for that. Also is there any plan to respin the related Qemu series as well?
I know dual stage interface proposals are still under discussion, but it would be
nice to have a testable solution based on new interfaces for ARM64 as well.
Happy to help with any tests or verifications.

Please let me know.

Thanks,
Shameer

Eric Auger Oct. 27, 2020, 1:04 p.m. UTC | #5

Hi Shameer,

On 10/27/20 1:20 PM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
> 
>> -----Original Message-----
>> From: iommu [mailto:[e-mail]] On Behalf Of
>> Auger Eric
>> Sent: 23 September 2020 12:47
>> To: yuzenghui <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v10 01/11] vfio: VFIO_IOMMU_SET_PASID_TABLE
> 
> ...
> 
>>> 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/[e-mail]
>>> [2]
>>> https://lore.kernel.org/r/[e-mail]
>>
>> 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. 
> 
> Thanks for that. Also is there any plan to respin the related Qemu series as well?
> I know dual stage interface proposals are still under discussion, but it would be
> nice to have a testable solution based on new interfaces for ARM64 as well.
> Happy to help with any tests or verifications.

Yes the QEMU series will be respinned as well. That's on the top of my
todo list right now.

Thanks

Eric
> 
> Please let me know.
> 
> Thanks,
> Shameer
>   
>

Patch

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;
 	}
 
 	return -ENOTTY;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 9e843a147ead..e032a1fe6ed9 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
 
@@ -794,6 +795,24 @@  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)
 
+/**
+ * VFIO_IOMMU_SET_PASID_TABLE - _IOWR(VFIO_TYPE, VFIO_BASE + 22,
+ *			struct vfio_iommu_type1_set_pasid_table)
+ *
+ * The SET operation passes a PASID table to the host while the
+ * UNSET operation detaches the one currently programmed. Setting
+ * a table while another is already programmed replaces the old table.
+ */
+struct vfio_iommu_type1_set_pasid_table {
+	__u32	argsz;
+	__u32	flags;
+#define VFIO_PASID_TABLE_FLAG_SET	(1 << 0)
+#define VFIO_PASID_TABLE_FLAG_UNSET	(1 << 1)
+	struct iommu_pasid_table_config config; /* used on SET */
+};
+
+#define VFIO_IOMMU_SET_PASID_TABLE	_IO(VFIO_TYPE, VFIO_BASE + 22)
+
 /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
 
 /*