[PATCH v2 0/3] IOMMU user API enhancement

[PATCH v2 0/3] IOMMU user API enhancement

[Jacob Pan]

IOMMU user API header was introduced to support nested DMA translation and
related fault handling. The current UAPI data structures consist of three
areas that cover the interactions between host kernel and guest:
 - fault handling
 - cache invalidation
 - bind guest page tables, i.e. guest PASID

With future extensions in mind, the UAPI structures passed from user to kernel
always starts with a mandatory version field (u32). While this is flexible
for extensions of individual structures, it is difficult to maintain support
of combinations of version numbers.

This patchset introduces a unified UAPI version number that governs all the
UAPI data structure versions. When userspace query UAPI version for check on
compatibility, a single match would be sufficient.

After UAPI version check, users such as VFIO can also retrieve the matching
data structure size based on version and type. Kernel IOMMU UAPI support is
always backward compatible. Data structures are also only open to extension
and closed to modifications.

The introduction of UAPI version does not change the existing UAPI but rather
simplify the data structure version and size matching.

Changelog:
- v2 Rewrite the extension rules that disallows adding new members at the end
     of each UAPI data structures. Only padding bytes and union can be extended.
     Clarified size look up array extension rules with examples.

Thanks,

Jacob


Jacob Pan (3):
  iommu/uapi: Define uapi version and capabilities
  iommu/uapi: Use unified UAPI version
  iommu/uapi: Add helper function for size lookup

 drivers/iommu/intel-iommu.c |  3 +-
 drivers/iommu/intel-svm.c   |  2 +-
 drivers/iommu/iommu.c       | 75 ++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/iommu.h       |  6 ++++
 include/uapi/linux/iommu.h  | 62 +++++++++++++++++++++++++++++++++----
 5 files changed, 139 insertions(+), 9 deletions(-)

-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

Having a single UAPI version to govern the user-kernel data structures
makes compatibility check straightforward. On the contrary, supporting
combinations of multiple versions of the data can be a nightmare to
maintain.

This patch defines a unified UAPI version to be used for compatibility
checks between user and kernel.

---
v2: Rewrite extension rules to disallow adding new members. Use padding
    and union extensions only.
---

Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Link: https://lkml.org/lkml/2020/2/3/1126
---
 include/uapi/linux/iommu.h | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index d7bcbc5f79b0..25dba9198d7f 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -8,6 +8,59 @@
 
 #include <linux/types.h>
 
+/**
+ * Current version of the IOMMU user API. This is intended for query
+ * between user and kernel to determine compatible data structures.
+ *
+ * UAPI version can be bumped up with the following rules:
+ * 1. All data structures passed between user and kernel space share
+ *    the same version number. i.e. any extension to any structure
+ *    results in version number increment.
+ *
+ * 2. Data structures are open to extension but closed to modification.
+ *    Extensions are allowed in two places:
+ *    - the padding bytes with a flag bit for each new member
+ *    - new union members at the end of each structure
+ *
+ *    No new members can be added after padding bytes are exhausted.
+ *    The reason is that the union size can change when new members are
+ *    added, having new member at the end would break backward
+ *    compatibility. Expansion of the union would move the new member
+ *    to different offset between versions.
+ *
+ *    Flag bits can be added without size change but existing ones
+ *    cannot be altered.
+ *
+ * 3. Versions are backward compatible.
+ *
+ * 4. Version to size lookup is supported by kernel internal API for each
+ *    API function type. @version is mandatory for new data structures
+ *    and must be at the beginning with type of __u32.
+ */
+#define IOMMU_UAPI_VERSION	1
+static inline int iommu_get_uapi_version(void)
+{
+	return IOMMU_UAPI_VERSION;
+}
+
+/*
+ * Supported UAPI features that can be reported to user space.
+ * These types represent the capability available in the kernel.
+ *
+ * REVISIT: UAPI version also implies the capabilities. Should we
+ * report them explicitly?
+ */
+enum IOMMU_UAPI_DATA_TYPES {
+	IOMMU_UAPI_BIND_GPASID,
+	IOMMU_UAPI_CACHE_INVAL,
+	IOMMU_UAPI_PAGE_RESP,
+	NR_IOMMU_UAPI_TYPE,
+};
+
+#define IOMMU_UAPI_CAP_MASK ((1 << IOMMU_UAPI_BIND_GPASID) |	\
+				(1 << IOMMU_UAPI_CACHE_INVAL) |	\
+				(1 << IOMMU_UAPI_PAGE_RESP))
+
 #define IOMMU_FAULT_PERM_READ	(1 << 0) /* read */
 #define IOMMU_FAULT_PERM_WRITE	(1 << 1) /* write */
 #define IOMMU_FAULT_PERM_EXEC	(1 << 2) /* exec */
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 2/3] iommu/uapi: Use unified UAPI version

[Jacob Pan]

Reuse UAPI version for each UAPI data structure.
This is to avoid supporting multiple version combinations, simplify
support model as we bump up the versions.

Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 3 ++-
 drivers/iommu/intel-svm.c   | 2 +-
 drivers/iommu/iommu.c       | 3 ++-
 include/uapi/linux/iommu.h  | 9 +++------
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index c1c0b0fb93c3..0304f7626a38 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5779,8 +5779,9 @@ static int intel_iommu_sva_invalidate(struct iommu_domain *domain,
 	int ret = 0;
 	u64 size = 0;
 
+	/* Support current or older UAPI versions */
 	if (!inv_info || !dmar_domain ||
-		inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
+		inv_info->version > IOMMU_UAPI_VERSION)
 		return -EINVAL;
 
 	if (!dev || !dev_is_pci(dev))
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 47c0deb5ae56..67ac95eb5e33 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -239,7 +239,7 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain,
 	if (WARN_ON(!iommu) || !data)
 		return -EINVAL;
 
-	if (data->version != IOMMU_GPASID_BIND_VERSION_1 ||
+	if (data->version > IOMMU_UAPI_VERSION ||
 	    data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
 		return -EINVAL;
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3e3528436e0b..c476b58e0ffb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1113,7 +1113,8 @@ int iommu_page_response(struct device *dev,
 	if (!param || !param->fault_param)
 		return -EINVAL;
 
-	if (msg->version != IOMMU_PAGE_RESP_VERSION_1 ||
+	/* Support current or older UAPI versions */
+	if (msg->version > IOMMU_UAPI_VERSION ||
 	    msg->flags & ~IOMMU_PAGE_RESP_PASID_VALID)
 		return -EINVAL;
 
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index 25dba9198d7f..e9532c747dbd 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -188,7 +188,7 @@ enum iommu_page_response_code {
 
 /**
  * struct iommu_page_response - Generic page response information
- * @version: API version of this structure
+ * @version: IOMMU_UAPI_VERSION
  * @flags: encodes whether the corresponding fields are valid
  *         (IOMMU_FAULT_PAGE_RESPONSE_* values)
  * @pasid: Process Address Space ID
@@ -196,7 +196,6 @@ enum iommu_page_response_code {
  * @code: response code from &enum iommu_page_response_code
  */
 struct iommu_page_response {
-#define IOMMU_PAGE_RESP_VERSION_1	1
 	__u32	version;
 #define IOMMU_PAGE_RESP_PASID_VALID	(1 << 0)
 	__u32	flags;
@@ -271,7 +270,7 @@ struct iommu_inv_pasid_info {
 /**
  * struct iommu_cache_invalidate_info - First level/stage invalidation
  *     information
- * @version: API version of this structure
+ * @version: IOMMU_UAPI_VERSION
  * @cache: bitfield that allows to select which caches to invalidate
  * @granularity: defines the lowest granularity used for the invalidation:
  *     domain > PASID > addr
@@ -299,7 +298,6 @@ struct iommu_inv_pasid_info {
  * must support the used granularity.
  */
 struct iommu_cache_invalidate_info {
-#define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1
 	__u32	version;
 /* IOMMU paging structure cache */
 #define IOMMU_CACHE_INV_TYPE_IOTLB	(1 << 0) /* IOMMU IOTLB */
@@ -343,7 +341,7 @@ struct iommu_gpasid_bind_data_vtd {
 					 IOMMU_SVA_VTD_GPASID_PWT)
 /**
  * struct iommu_gpasid_bind_data - Information about device and guest PASID binding
- * @version:	Version of this data structure
+ * @version:	IOMMU_UAPI_VERSION
  * @format:	PASID table entry format
  * @flags:	Additional information on guest bind request
  * @gpgd:	Guest page directory base of the guest mm to bind
@@ -360,7 +358,6 @@ struct iommu_gpasid_bind_data_vtd {
  * PASID to host PASID based on this bind data.
  */
 struct iommu_gpasid_bind_data {
-#define IOMMU_GPASID_BIND_VERSION_1	1
 	__u32 version;
 #define IOMMU_PASID_FORMAT_INTEL_VTD	1
 	__u32 format;
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 3/3] iommu/uapi: Add helper function for size lookup

[Jacob Pan]

IOMMU UAPI can be extended in the future by adding new
fields at the end of each user data structure. Since we use
a unified UAPI version for compatibility checking, a lookup
function is needed to find the correct user data size to copy
from user.

This patch adds a helper function based on a 2D lookup with
version and type as input arguments.

---
v2: Clarify size lookup array extension rules, backfill -EINVAL
    if new version introduce new union members.
---

Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/iommu.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h |  6 +++++
 2 files changed, 78 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c476b58e0ffb..e91ced212653 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1724,6 +1724,78 @@ int iommu_sva_unbind_gpasid(struct iommu_domain *domain, struct device *dev,
 }
 EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
 
+
+/**
+ * Maintain a UAPI version to user data structure size lookup for each
+ * API function types we support. e.g. bind guest pasid, cache invalidation.
+ * As data structures being extended with new members, offsetofend() is
+ * used to identify the size. In case of adding a new member to a union,
+ * offsetofend() applies to the largest member which may not be the newest.
+ *
+ * When new types are introduced with new versions, the new types for older
+ * version must be filled with -EINVAL.
+ *
+ * The table below documents UAPI revision history with the name of the
+ * newest member of each data structure. The largest member of a union was
+ * used for the initial version of each type.
+ *
+ * +--------------+---------------+
+ * | Type /       |       V1      |
+ * | UAPI Version |               |
+ * +==============+===============+
+ * | BIND_GPASID  |       vtd     |
+ * +--------------+---------------+
+ * | CACHE_INVAL  |  addr_info    |
+ * +--------------+---------------+
+ * | PAGE_RESP    |  code         |
+ * +--------------+---------------+
+ *
+ * Examples of future extensions:
+ * V2 addes new member to the union
+ * +--------------+---------------+---------------+
+ * | Type /       |       V1      |      V2       |
+ * | UAPI Version |               |               |
+ * +==============+===============+===============+
+ * | BIND_GPASID  |       vtd     |      smmu_v3  |
+ * +--------------+---------------+---------------+
+ * | CACHE_INVAL  |  addr_info    |     new_info  |
+ * +--------------+---------------+---------------+
+ * | PAGE_RESP    |  code         |     N/A       |
+ * +--------------+---------------+---------------+
+ *
+ * V3 introduces a new UAPI data type: NEW_TYPE but with no new members
+ * added to the existing types.
+ * +--------------+---------------+---------------+---------------+
+ * | Type /       |       V1      |      V2       |      V3       |
+ * | UAPI Version |               |               |               |
+ * +==============+===============+===============+===============+
+ * | BIND_GPASID  |       vtd     |      smmu_v3  |       N/A     |
+ * +--------------+---------------+---------------+---------------+
+ * | CACHE_INVAL  |  addr_info    |    new_info   |       N/A     |
+ * +--------------+---------------+---------------+---------------+
+ * | PAGE_RESP    |  code         |    N/A        |       N/A     |
+ * +--------------+---------------+---------------+---------------+
+ * | NEW_TYPE     |  -EINVAL      |     -EINVAL   | largest_member|
+ * +--------------+---------------+---------------+---------------+
+ */
+const static int iommu_uapi_data_size[NR_IOMMU_UAPI_TYPE][IOMMU_UAPI_VERSION] = {
+	/* IOMMU_UAPI_BIND_GPASID */
+	{offsetofend(struct iommu_gpasid_bind_data, vtd)},
+	/* IOMMU_UAPI_CACHE_INVAL */
+	{offsetofend(struct iommu_cache_invalidate_info, addr_info)},
+	/* IOMMU_UAPI_PAGE_RESP */
+	{offsetofend(struct iommu_page_response, code)},
+};
+
+int iommu_uapi_get_data_size(int type, int version)
+{
+	if (type >= NR_IOMMU_UAPI_TYPE || version > IOMMU_UAPI_VERSION)
+		return -EINVAL;
+
+	return iommu_uapi_data_size[type][version - 1];
+}
+EXPORT_SYMBOL_GPL(iommu_uapi_get_data_size);
+
 static void __iommu_detach_device(struct iommu_domain *domain,
 				  struct device *dev)
 {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d1b5f4d98569..4908919a98f1 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -510,6 +510,7 @@ extern int iommu_report_device_fault(struct device *dev,
 				     struct iommu_fault_event *evt);
 extern int iommu_page_response(struct device *dev,
 			       struct iommu_page_response *msg);
+extern int iommu_uapi_get_data_size(int type, int version);
 
 extern int iommu_group_id(struct iommu_group *group);
 extern struct iommu_group *iommu_group_get_for_dev(struct device *dev);
@@ -897,6 +898,11 @@ static inline int iommu_page_response(struct device *dev,
 	return -ENODEV;
 }
 
+static inline int iommu_uapi_get_data_size(int type, int version)
+{
+	return -ENODEV;
+}
+
 static inline int iommu_group_id(struct iommu_group *group)
 {
 	return -ENODEV;
-- 
2.7.4

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Christoph Hellwig]

On Wed, Mar 25, 2020 at 04:17:05PM -0700, Jacob Pan wrote:
> Having a single UAPI version to govern the user-kernel data structures
> makes compatibility check straightforward. On the contrary, supporting
> combinations of multiple versions of the data can be a nightmare to
> maintain.
> 
> This patch defines a unified UAPI version to be used for compatibility
> checks between user and kernel.

This is bullshit.  Version numbers don't scale and we've avoided them
everywhere.  You need need flags specifying specific behavior.

> +#define IOMMU_UAPI_VERSION	1
> +static inline int iommu_get_uapi_version(void)
> +{
> +	return IOMMU_UAPI_VERSION;
> +}

Also inline functions like this in UAPI headers that actually get
included by userspace programs simply don't work.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

Hi Christoph,

Thanks for the review. Please see my comments inline.

On Thu, 26 Mar 2020 02:23:16 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, Mar 25, 2020 at 04:17:05PM -0700, Jacob Pan wrote:
> > Having a single UAPI version to govern the user-kernel data
> > structures makes compatibility check straightforward. On the
> > contrary, supporting combinations of multiple versions of the data
> > can be a nightmare to maintain.
> > 
> > This patch defines a unified UAPI version to be used for
> > compatibility checks between user and kernel.  
> 
> This is bullshit.  Version numbers don't scale and we've avoided them
> everywhere.  You need need flags specifying specific behavior.
> 
We have flags or the equivalent in each UAPI structures. The flags
are used for checking validity of extensions as well. And you are right
we can use them for checking specific behavior.

So we have two options here:
1. Have a overall version for a quick compatibility check while
starting a VM. If not compatible, we will stop guest SVA entirely.

2. Let each API calls check its own capabilities/flags at runtime. It
may fail later on and lead to more complex error handling.
For example, guest starts with SVA support, allocate a PASID, bind
guest PASID works great. Then when IO page fault happens, it try to do
page request service and found out certain flags are not compatible.
This case is more complex to handle.

I am guessing your proposal is #2. right?

Overall, we don;t expect much change to the UAPI other than adding some
vendor specific part of the union. Is the scalability concern based on
frequent changes?

> > +#define IOMMU_UAPI_VERSION	1
> > +static inline int iommu_get_uapi_version(void)
> > +{
> > +	return IOMMU_UAPI_VERSION;
> > +}  
> 
> Also inline functions like this in UAPI headers that actually get
> included by userspace programs simply don't work.

I will remove that, user can just use IOMMU_UAPI_VERSION directly.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Friday, March 27, 2020 12:45 AM
> 
> Hi Christoph,
> 
> Thanks for the review. Please see my comments inline.
> 
> On Thu, 26 Mar 2020 02:23:16 -0700
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Wed, Mar 25, 2020 at 04:17:05PM -0700, Jacob Pan wrote:
> > > Having a single UAPI version to govern the user-kernel data
> > > structures makes compatibility check straightforward. On the
> > > contrary, supporting combinations of multiple versions of the data
> > > can be a nightmare to maintain.
> > >
> > > This patch defines a unified UAPI version to be used for
> > > compatibility checks between user and kernel.
> >
> > This is bullshit.  Version numbers don't scale and we've avoided them
> > everywhere.  You need need flags specifying specific behavior.
> >
> We have flags or the equivalent in each UAPI structures. The flags
> are used for checking validity of extensions as well. And you are right
> we can use them for checking specific behavior.
> 
> So we have two options here:
> 1. Have a overall version for a quick compatibility check while
> starting a VM. If not compatible, we will stop guest SVA entirely.
> 
> 2. Let each API calls check its own capabilities/flags at runtime. It
> may fail later on and lead to more complex error handling.
> For example, guest starts with SVA support, allocate a PASID, bind
> guest PASID works great. Then when IO page fault happens, it try to do
> page request service and found out certain flags are not compatible.
> This case is more complex to handle.

If those API calls are inter-dependent for composing a feature (e.g. SVA),
shouldn't we need a way to check them together before exposing the 
feature to the guest, e.g. through a iommu_get_uapi_capabilities interface?

> 
> I am guessing your proposal is #2. right?
> 
> Overall, we don;t expect much change to the UAPI other than adding some
> vendor specific part of the union. Is the scalability concern based on
> frequent changes?
> 
> > > +#define IOMMU_UAPI_VERSION	1
> > > +static inline int iommu_get_uapi_version(void)
> > > +{
> > > +	return IOMMU_UAPI_VERSION;
> > > +}
> >
> > Also inline functions like this in UAPI headers that actually get
> > included by userspace programs simply don't work.
> 
> I will remove that, user can just use IOMMU_UAPI_VERSION directly.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Christoph Hellwig]

On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin wrote:
> If those API calls are inter-dependent for composing a feature (e.g. SVA),
> shouldn't we need a way to check them together before exposing the 
> feature to the guest, e.g. through a iommu_get_uapi_capabilities interface?

Yes, that makes sense.  The important bit is to have a capability flags
and not version numbers.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

On Fri, 27 Mar 2020 00:47:02 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin wrote:
> > If those API calls are inter-dependent for composing a feature
> > (e.g. SVA), shouldn't we need a way to check them together before
> > exposing the feature to the guest, e.g. through a
> > iommu_get_uapi_capabilities interface?  
> 
> Yes, that makes sense.  The important bit is to have a capability
> flags and not version numbers.

The challenge is that there are two consumers in the kernel for this.
1. VFIO only look for compatibility, and size of each data struct such
that it can copy_from_user.

2. IOMMU driver, the "real consumer" of the content.

For 2, I agree and we do plan to use the capability flags to check
content and maintain backward compatibility etc.

For VFIO, it is difficult to do size look up based on capability flags.


Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Saturday, March 28, 2020 7:54 AM
> 
> On Fri, 27 Mar 2020 00:47:02 -0700
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin wrote:
> > > If those API calls are inter-dependent for composing a feature
> > > (e.g. SVA), shouldn't we need a way to check them together before
> > > exposing the feature to the guest, e.g. through a
> > > iommu_get_uapi_capabilities interface?
> >
> > Yes, that makes sense.  The important bit is to have a capability
> > flags and not version numbers.
> 
> The challenge is that there are two consumers in the kernel for this.
> 1. VFIO only look for compatibility, and size of each data struct such
> that it can copy_from_user.
> 
> 2. IOMMU driver, the "real consumer" of the content.
> 
> For 2, I agree and we do plan to use the capability flags to check
> content and maintain backward compatibility etc.
> 
> For VFIO, it is difficult to do size look up based on capability flags.
> 

Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig
pointed out, version number is already avoided everywhere, it is 
interesting to know whether this work becomes a real exception
or just requires a different mindset.

btw the most relevant discussion which I can find out now is here:
	https://lkml.org/lkml/2020/2/3/1126

It mentioned 3 options for handling extension:
--
1. Disallow adding new members to each structure other than reuse
padding bits or adding union members at the end.
2. Allow extension of the structures beyond union, but union size has
to be fixed with reserved spaces
3. Adopt VFIO argsz scheme, I don't think we need version for each
struct anymore. argsz implies the version that user is using assuming
UAPI data is extension only.
--

the first two are both version-based. Looks most guys agreed with 
option-1 (in this v2), but Alex didn't give his opinion at the moment. 
The last response from him was the raise of option-3 using argsz to 
avoid version. So, we also need hear from him. Alex?

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

On Mon, 30 Mar 2020 05:40:40 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Saturday, March 28, 2020 7:54 AM
> > 
> > On Fri, 27 Mar 2020 00:47:02 -0700
> > Christoph Hellwig <hch@infradead.org> wrote:
> >   
> > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin wrote:  
> > > > If those API calls are inter-dependent for composing a feature
> > > > (e.g. SVA), shouldn't we need a way to check them together
> > > > before exposing the feature to the guest, e.g. through a
> > > > iommu_get_uapi_capabilities interface?  
> > >
> > > Yes, that makes sense.  The important bit is to have a capability
> > > flags and not version numbers.  
> > 
> > The challenge is that there are two consumers in the kernel for
> > this. 1. VFIO only look for compatibility, and size of each data
> > struct such that it can copy_from_user.
> > 
> > 2. IOMMU driver, the "real consumer" of the content.
> > 
> > For 2, I agree and we do plan to use the capability flags to check
> > content and maintain backward compatibility etc.
> > 
> > For VFIO, it is difficult to do size look up based on capability
> > flags. 
> 
> Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig
> pointed out, version number is already avoided everywhere, it is 
> interesting to know whether this work becomes a real exception
> or just requires a different mindset.
> 
From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs to do two
things:
1. is the UAPI compatible?
2. what is the size to copy?

If you look at the version number, this is really a "version as size"
lookup, as provided by the helper function in this patch. An example
can be the newly introduced clone3 syscall.
https://lwn.net/Articles/792628/
In clone3, new version must have new size. The slight difference here
is that, unlike clone3, we have multiple data structures instead of a
single struct clone_args {}. And each struct has flags to enumerate its
contents besides size.

Besides breaching data abstraction, if VFIO has to check IOMMU flags to
determine the sizes, it has many combinations.

We also separate the responsibilities into two parts
1. compatibility - version, size by VFIO
2. sanity check - capability flags - by IOMMU

I think the latter matches what Christoph's comments. So we are in
agreement at the IOMMU level :)

For example:
During guest PASID bind, IOMMU driver operates on the data passed from
VFIO and check format & flags to take different code path.

#define IOMMU_PASID_FORMAT_INTEL_VTD	1
	__u32 format;
#define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
	__u64 flags;

Jacob

> btw the most relevant discussion which I can find out now is here:
> 	https://lkml.org/lkml/2020/2/3/1126
> 
> It mentioned 3 options for handling extension:
> --
> 1. Disallow adding new members to each structure other than reuse
> padding bits or adding union members at the end.
> 2. Allow extension of the structures beyond union, but union size has
> to be fixed with reserved spaces
> 3. Adopt VFIO argsz scheme, I don't think we need version for each
> struct anymore. argsz implies the version that user is using assuming
> UAPI data is extension only.
> --
> 
> the first two are both version-based. Looks most guys agreed with 
> option-1 (in this v2), but Alex didn't give his opinion at the
> moment. The last response from him was the raise of option-3 using
> argsz to avoid version. So, we also need hear from him. Alex?
> 
> Thanks
> Kevin

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Tuesday, March 31, 2020 12:08 AM
> 
> On Mon, 30 Mar 2020 05:40:40 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
> > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > Sent: Saturday, March 28, 2020 7:54 AM
> > >
> > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin wrote:
> > > > > If those API calls are inter-dependent for composing a feature
> > > > > (e.g. SVA), shouldn't we need a way to check them together
> > > > > before exposing the feature to the guest, e.g. through a
> > > > > iommu_get_uapi_capabilities interface?
> > > >
> > > > Yes, that makes sense.  The important bit is to have a capability
> > > > flags and not version numbers.
> > >
> > > The challenge is that there are two consumers in the kernel for
> > > this. 1. VFIO only look for compatibility, and size of each data
> > > struct such that it can copy_from_user.
> > >
> > > 2. IOMMU driver, the "real consumer" of the content.
> > >
> > > For 2, I agree and we do plan to use the capability flags to check
> > > content and maintain backward compatibility etc.
> > >
> > > For VFIO, it is difficult to do size look up based on capability
> > > flags.
> >
> > Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig
> > pointed out, version number is already avoided everywhere, it is
> > interesting to know whether this work becomes a real exception
> > or just requires a different mindset.
> >
> From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs to do two
> things:
> 1. is the UAPI compatible?
> 2. what is the size to copy?
> 
> If you look at the version number, this is really a "version as size"
> lookup, as provided by the helper function in this patch. An example
> can be the newly introduced clone3 syscall.
> https://lwn.net/Articles/792628/
> In clone3, new version must have new size. The slight difference here
> is that, unlike clone3, we have multiple data structures instead of a
> single struct clone_args {}. And each struct has flags to enumerate its
> contents besides size.

Thanks for providing that link. However clone3 doesn't include a version
field to do "version as size" lookup. Instead, as you said, it includes a
size parameter which sounds like the option 3 (argsz) listed below.

> 
> Besides breaching data abstraction, if VFIO has to check IOMMU flags to
> determine the sizes, it has many combinations.
> 
> We also separate the responsibilities into two parts
> 1. compatibility - version, size by VFIO
> 2. sanity check - capability flags - by IOMMU

I feel argsz+flags approach can perfectly meet above requirement. The
userspace set the size and flags for whatever capabilities it uses, and
VFIO simply copies the parameters by size and pass to IOMMU for
further sanity check. Of course the assumption is that we do provide
an interface for userspace to enumerate all supported capabilities.

Is there anything that I overlooked here? I suppose there might be
some difficulties that block you from going the argsz way...

Thanks
Kevin

> 
> I think the latter matches what Christoph's comments. So we are in
> agreement at the IOMMU level :)
> 
> For example:
> During guest PASID bind, IOMMU driver operates on the data passed from
> VFIO and check format & flags to take different code path.
> 
> #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> 	__u32 format;
> #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> 	__u64 flags;
> 
> Jacob
> 
> > btw the most relevant discussion which I can find out now is here:
> > 	https://lkml.org/lkml/2020/2/3/1126
> >
> > It mentioned 3 options for handling extension:
> > --
> > 1. Disallow adding new members to each structure other than reuse
> > padding bits or adding union members at the end.
> > 2. Allow extension of the structures beyond union, but union size has
> > to be fixed with reserved spaces
> > 3. Adopt VFIO argsz scheme, I don't think we need version for each
> > struct anymore. argsz implies the version that user is using assuming
> > UAPI data is extension only.
> > --
> >
> > the first two are both version-based. Looks most guys agreed with
> > option-1 (in this v2), but Alex didn't give his opinion at the
> > moment. The last response from him was the raise of option-3 using
> > argsz to avoid version. So, we also need hear from him. Alex?
> >
> > Thanks
> > Kevin
> 
> [Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

On Tue, 31 Mar 2020 06:06:38 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Tuesday, March 31, 2020 12:08 AM
> > 
> > On Mon, 30 Mar 2020 05:40:40 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >   
> > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > >
> > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > Christoph Hellwig <hch@infradead.org> wrote:
> > > >  
> > > > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin wrote:  
> > > > > > If those API calls are inter-dependent for composing a
> > > > > > feature (e.g. SVA), shouldn't we need a way to check them
> > > > > > together before exposing the feature to the guest, e.g.
> > > > > > through a iommu_get_uapi_capabilities interface?  
> > > > >
> > > > > Yes, that makes sense.  The important bit is to have a
> > > > > capability flags and not version numbers.  
> > > >
> > > > The challenge is that there are two consumers in the kernel for
> > > > this. 1. VFIO only look for compatibility, and size of each data
> > > > struct such that it can copy_from_user.
> > > >
> > > > 2. IOMMU driver, the "real consumer" of the content.
> > > >
> > > > For 2, I agree and we do plan to use the capability flags to
> > > > check content and maintain backward compatibility etc.
> > > >
> > > > For VFIO, it is difficult to do size look up based on capability
> > > > flags.  
> > >
> > > Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig
> > > pointed out, version number is already avoided everywhere, it is
> > > interesting to know whether this work becomes a real exception
> > > or just requires a different mindset.
> > >  
> > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs to do
> > two things:
> > 1. is the UAPI compatible?
> > 2. what is the size to copy?
> > 
> > If you look at the version number, this is really a "version as
> > size" lookup, as provided by the helper function in this patch. An
> > example can be the newly introduced clone3 syscall.
> > https://lwn.net/Articles/792628/
> > In clone3, new version must have new size. The slight difference
> > here is that, unlike clone3, we have multiple data structures
> > instead of a single struct clone_args {}. And each struct has flags
> > to enumerate its contents besides size.  
> 
> Thanks for providing that link. However clone3 doesn't include a
> version field to do "version as size" lookup. Instead, as you said,
> it includes a size parameter which sounds like the option 3 (argsz)
> listed below.
> 
Right, there is no version in clone3. size = version. I view this as
a 1:1 lookup.

> > 
> > Besides breaching data abstraction, if VFIO has to check IOMMU
> > flags to determine the sizes, it has many combinations.
> > 
> > We also separate the responsibilities into two parts
> > 1. compatibility - version, size by VFIO
> > 2. sanity check - capability flags - by IOMMU  
> 
> I feel argsz+flags approach can perfectly meet above requirement. The
> userspace set the size and flags for whatever capabilities it uses,
> and VFIO simply copies the parameters by size and pass to IOMMU for
> further sanity check. Of course the assumption is that we do provide
> an interface for userspace to enumerate all supported capabilities.
> 
You cannot trust user for argsz. the size to be copied from user must
be based on knowledge in kernel. That is why we have this version to
size lookup.

In VFIO, the size to copy is based on knowledge of each VFIO UAPI
structures and VFIO flags. But here the flags are IOMMU UAPI flags. As
you pointed out in another thread, VFIO is one user.

> Is there anything that I overlooked here? I suppose there might be
> some difficulties that block you from going the argsz way...
> 
> Thanks
> Kevin
> 
> > 
> > I think the latter matches what Christoph's comments. So we are in
> > agreement at the IOMMU level :)
> > 
> > For example:
> > During guest PASID bind, IOMMU driver operates on the data passed
> > from VFIO and check format & flags to take different code path.
> > 
> > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > 	__u32 format;
> > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> > 	__u64 flags;
> > 
> > Jacob
> >   
> > > btw the most relevant discussion which I can find out now is here:
> > > 	https://lkml.org/lkml/2020/2/3/1126
> > >
> > > It mentioned 3 options for handling extension:
> > > --
> > > 1. Disallow adding new members to each structure other than reuse
> > > padding bits or adding union members at the end.
> > > 2. Allow extension of the structures beyond union, but union size
> > > has to be fixed with reserved spaces
> > > 3. Adopt VFIO argsz scheme, I don't think we need version for each
> > > struct anymore. argsz implies the version that user is using
> > > assuming UAPI data is extension only.
> > > --
> > >
> > > the first two are both version-based. Looks most guys agreed with
> > > option-1 (in this v2), but Alex didn't give his opinion at the
> > > moment. The last response from him was the raise of option-3 using
> > > argsz to avoid version. So, we also need hear from him. Alex?
> > >
> > > Thanks
> > > Kevin  
> > 
> > [Jacob Pan]  

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Tuesday, March 31, 2020 11:55 PM
> 
> On Tue, 31 Mar 2020 06:06:38 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
> > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > Sent: Tuesday, March 31, 2020 12:08 AM
> > >
> > > On Mon, 30 Mar 2020 05:40:40 +0000
> > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > >
> > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > >
> > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > Christoph Hellwig <hch@infradead.org> wrote:
> > > > >
> > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin wrote:
> > > > > > > If those API calls are inter-dependent for composing a
> > > > > > > feature (e.g. SVA), shouldn't we need a way to check them
> > > > > > > together before exposing the feature to the guest, e.g.
> > > > > > > through a iommu_get_uapi_capabilities interface?
> > > > > >
> > > > > > Yes, that makes sense.  The important bit is to have a
> > > > > > capability flags and not version numbers.
> > > > >
> > > > > The challenge is that there are two consumers in the kernel for
> > > > > this. 1. VFIO only look for compatibility, and size of each data
> > > > > struct such that it can copy_from_user.
> > > > >
> > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > >
> > > > > For 2, I agree and we do plan to use the capability flags to
> > > > > check content and maintain backward compatibility etc.
> > > > >
> > > > > For VFIO, it is difficult to do size look up based on capability
> > > > > flags.
> > > >
> > > > Can you elaborate the difficulty in VFIO? if, as Christoph Hellwig
> > > > pointed out, version number is already avoided everywhere, it is
> > > > interesting to know whether this work becomes a real exception
> > > > or just requires a different mindset.
> > > >
> > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs to do
> > > two things:
> > > 1. is the UAPI compatible?
> > > 2. what is the size to copy?
> > >
> > > If you look at the version number, this is really a "version as
> > > size" lookup, as provided by the helper function in this patch. An
> > > example can be the newly introduced clone3 syscall.
> > > https://lwn.net/Articles/792628/
> > > In clone3, new version must have new size. The slight difference
> > > here is that, unlike clone3, we have multiple data structures
> > > instead of a single struct clone_args {}. And each struct has flags
> > > to enumerate its contents besides size.
> >
> > Thanks for providing that link. However clone3 doesn't include a
> > version field to do "version as size" lookup. Instead, as you said,
> > it includes a size parameter which sounds like the option 3 (argsz)
> > listed below.
> >
> Right, there is no version in clone3. size = version. I view this as
> a 1:1 lookup.
> 
> > >
> > > Besides breaching data abstraction, if VFIO has to check IOMMU
> > > flags to determine the sizes, it has many combinations.
> > >
> > > We also separate the responsibilities into two parts
> > > 1. compatibility - version, size by VFIO
> > > 2. sanity check - capability flags - by IOMMU
> >
> > I feel argsz+flags approach can perfectly meet above requirement. The
> > userspace set the size and flags for whatever capabilities it uses,
> > and VFIO simply copies the parameters by size and pass to IOMMU for
> > further sanity check. Of course the assumption is that we do provide
> > an interface for userspace to enumerate all supported capabilities.
> >
> You cannot trust user for argsz. the size to be copied from user must
> be based on knowledge in kernel. That is why we have this version to
> size lookup.
> 
> In VFIO, the size to copy is based on knowledge of each VFIO UAPI
> structures and VFIO flags. But here the flags are IOMMU UAPI flags. As
> you pointed out in another thread, VFIO is one user.

If that is the case, can we let VFIO only copy its own UAPI fields while 
simply passing the user pointer of IOMMU UAPI structure to IOMMU
driver for further size check and copy? Otherwise we are entering a
dead end that VFIO doesn't want to parse a structure which is not
defined by him while using version to represent the black box size
is considered as a discarded scheme and doesn't scale well...

> 
> > Is there anything that I overlooked here? I suppose there might be
> > some difficulties that block you from going the argsz way...
> >
> > Thanks
> > Kevin
> >
> > >
> > > I think the latter matches what Christoph's comments. So we are in
> > > agreement at the IOMMU level :)
> > >
> > > For example:
> > > During guest PASID bind, IOMMU driver operates on the data passed
> > > from VFIO and check format & flags to take different code path.
> > >
> > > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > > 	__u32 format;
> > > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> > > 	__u64 flags;
> > >
> > > Jacob
> > >
> > > > btw the most relevant discussion which I can find out now is here:
> > > > 	https://lkml.org/lkml/2020/2/3/1126
> > > >
> > > > It mentioned 3 options for handling extension:
> > > > --
> > > > 1. Disallow adding new members to each structure other than reuse
> > > > padding bits or adding union members at the end.
> > > > 2. Allow extension of the structures beyond union, but union size
> > > > has to be fixed with reserved spaces
> > > > 3. Adopt VFIO argsz scheme, I don't think we need version for each
> > > > struct anymore. argsz implies the version that user is using
> > > > assuming UAPI data is extension only.
> > > > --
> > > >
> > > > the first two are both version-based. Looks most guys agreed with
> > > > option-1 (in this v2), but Alex didn't give his opinion at the
> > > > moment. The last response from him was the raise of option-3 using
> > > > argsz to avoid version. So, we also need hear from him. Alex?
> > > >
> > > > Thanks
> > > > Kevin
> > >
> > > [Jacob Pan]
> 
> [Jacob Pan]

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

On Wed, 1 Apr 2020 05:32:21 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Tuesday, March 31, 2020 11:55 PM
> > 
> > On Tue, 31 Mar 2020 06:06:38 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >   
> > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > >
> > > > On Mon, 30 Mar 2020 05:40:40 +0000
> > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > >  
> > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > >
> > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > Christoph Hellwig <hch@infradead.org> wrote:
> > > > > >  
> > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin
> > > > > > > wrote:  
> > > > > > > > If those API calls are inter-dependent for composing a
> > > > > > > > feature (e.g. SVA), shouldn't we need a way to check
> > > > > > > > them together before exposing the feature to the guest,
> > > > > > > > e.g. through a iommu_get_uapi_capabilities interface?  
> > > > > > >
> > > > > > > Yes, that makes sense.  The important bit is to have a
> > > > > > > capability flags and not version numbers.  
> > > > > >
> > > > > > The challenge is that there are two consumers in the kernel
> > > > > > for this. 1. VFIO only look for compatibility, and size of
> > > > > > each data struct such that it can copy_from_user.
> > > > > >
> > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > >
> > > > > > For 2, I agree and we do plan to use the capability flags to
> > > > > > check content and maintain backward compatibility etc.
> > > > > >
> > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > capability flags.  
> > > > >
> > > > > Can you elaborate the difficulty in VFIO? if, as Christoph
> > > > > Hellwig pointed out, version number is already avoided
> > > > > everywhere, it is interesting to know whether this work
> > > > > becomes a real exception or just requires a different mindset.
> > > > >  
> > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs
> > > > to do two things:
> > > > 1. is the UAPI compatible?
> > > > 2. what is the size to copy?
> > > >
> > > > If you look at the version number, this is really a "version as
> > > > size" lookup, as provided by the helper function in this patch.
> > > > An example can be the newly introduced clone3 syscall.
> > > > https://lwn.net/Articles/792628/
> > > > In clone3, new version must have new size. The slight difference
> > > > here is that, unlike clone3, we have multiple data structures
> > > > instead of a single struct clone_args {}. And each struct has
> > > > flags to enumerate its contents besides size.  
> > >
> > > Thanks for providing that link. However clone3 doesn't include a
> > > version field to do "version as size" lookup. Instead, as you
> > > said, it includes a size parameter which sounds like the option 3
> > > (argsz) listed below.
> > >  
> > Right, there is no version in clone3. size = version. I view this as
> > a 1:1 lookup.
> >   
> > > >
> > > > Besides breaching data abstraction, if VFIO has to check IOMMU
> > > > flags to determine the sizes, it has many combinations.
> > > >
> > > > We also separate the responsibilities into two parts
> > > > 1. compatibility - version, size by VFIO
> > > > 2. sanity check - capability flags - by IOMMU  
> > >
> > > I feel argsz+flags approach can perfectly meet above requirement.
> > > The userspace set the size and flags for whatever capabilities it
> > > uses, and VFIO simply copies the parameters by size and pass to
> > > IOMMU for further sanity check. Of course the assumption is that
> > > we do provide an interface for userspace to enumerate all
> > > supported capabilities. 
> > You cannot trust user for argsz. the size to be copied from user
> > must be based on knowledge in kernel. That is why we have this
> > version to size lookup.
> > 
> > In VFIO, the size to copy is based on knowledge of each VFIO UAPI
> > structures and VFIO flags. But here the flags are IOMMU UAPI flags.
> > As you pointed out in another thread, VFIO is one user.  
> 
> If that is the case, can we let VFIO only copy its own UAPI fields
> while simply passing the user pointer of IOMMU UAPI structure to IOMMU
> driver for further size check and copy? Otherwise we are entering a
> dead end that VFIO doesn't want to parse a structure which is not
> defined by him while using version to represent the black box size
> is considered as a discarded scheme and doesn't scale well...
> 
I think this could be an other viable option. Let me try to summarize
since this has been a long discussion since the original version.

Problem statements:
1. When launching vIOMMU in the guest, how can we ensure the host has
compatible support upfront? as compared to fail later.

2. As UAPI data gets extended (both in size and flags), how can we know
the size to copy

3. Maintain backward compatibility while allowing extensions?

I think we all agreed that using flags (capability or types) is the way
to address #3. As Christoph pointed out, version number should not be
used for this purpose.

So for problem 1 & 2, we have the following options:
1. Have a version-size mapping as proposed in this set. VFIO copies from
user the correct size based on version-type lookup. Processing of the
data is based on flags in IOMMU driver.

2. VFIO copy its own minsz then pass the user pointer to IOMMU driver
for further copy_from_user based on flags. (by Kevin)

3. Adopt VFIO argsz scheme, caller fills in argsz for the offset the
variable size union. VFIO do not check argsz in that it requires IOMMU
specific knowledge. IOMMU driver Use flags to handle the variable
size.(by Alex). I think this what we have in Yi's VFIO & QEMU patch.
argsz filled by QEMU includes bind_data.

4. Do not use a unified version, have a fixed size of all UAPI
structures, padding in struct and union. (Wasteful, not preferred per
V1 discussion)

For both 2 & 3, a unified version is not used, each API
treated separately. vIOMMU will be launched w/o assurance of
compatibility of all APIs. Fault handling may be more complex in normal
operations.

Appreciate everyone's input. Joerg and Alex, could you help to make a
decision here?


Thanks,

Jacob

> >   
>  [...]  
>  [...]  
>  [...]  
> > > >
> > > > [Jacob Pan]  
> > 
> > [Jacob Pan]  
> 
> Thanks
> Kevin

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

Hi All,

Just a gentle reminder, any feedback on the options I listed below? New
ideas will be even better.

Christoph, does the explanation make sense to you? We do have the
capability/flag based scheme for IOMMU API extension, the version is
mainly used for size lookup. Compatibility checking is another use of
the version, it makes checking easy when a vIOMMU is launched.

Thanks,

Jacob

On Thu, 2 Apr 2020 11:36:04 -0700
Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:

> On Wed, 1 Apr 2020 05:32:21 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
> > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > Sent: Tuesday, March 31, 2020 11:55 PM
> > > 
> > > On Tue, 31 Mar 2020 06:06:38 +0000
> > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > >     
> > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > >
> > > > > On Mon, 30 Mar 2020 05:40:40 +0000
> > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > > >    
> > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > >
> > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > Christoph Hellwig <hch@infradead.org> wrote:
> > > > > > >    
> > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin
> > > > > > > > wrote:    
> > > > > > > > > If those API calls are inter-dependent for composing a
> > > > > > > > > feature (e.g. SVA), shouldn't we need a way to check
> > > > > > > > > them together before exposing the feature to the
> > > > > > > > > guest, e.g. through a iommu_get_uapi_capabilities
> > > > > > > > > interface?    
> > > > > > > >
> > > > > > > > Yes, that makes sense.  The important bit is to have a
> > > > > > > > capability flags and not version numbers.    
> > > > > > >
> > > > > > > The challenge is that there are two consumers in the
> > > > > > > kernel for this. 1. VFIO only look for compatibility, and
> > > > > > > size of each data struct such that it can copy_from_user.
> > > > > > >
> > > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > > >
> > > > > > > For 2, I agree and we do plan to use the capability flags
> > > > > > > to check content and maintain backward compatibility etc.
> > > > > > >
> > > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > > capability flags.    
> > > > > >
> > > > > > Can you elaborate the difficulty in VFIO? if, as Christoph
> > > > > > Hellwig pointed out, version number is already avoided
> > > > > > everywhere, it is interesting to know whether this work
> > > > > > becomes a real exception or just requires a different
> > > > > > mindset. 
> > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs
> > > > > to do two things:
> > > > > 1. is the UAPI compatible?
> > > > > 2. what is the size to copy?
> > > > >
> > > > > If you look at the version number, this is really a "version
> > > > > as size" lookup, as provided by the helper function in this
> > > > > patch. An example can be the newly introduced clone3 syscall.
> > > > > https://lwn.net/Articles/792628/
> > > > > In clone3, new version must have new size. The slight
> > > > > difference here is that, unlike clone3, we have multiple data
> > > > > structures instead of a single struct clone_args {}. And each
> > > > > struct has flags to enumerate its contents besides size.    
> > > >
> > > > Thanks for providing that link. However clone3 doesn't include a
> > > > version field to do "version as size" lookup. Instead, as you
> > > > said, it includes a size parameter which sounds like the option
> > > > 3 (argsz) listed below.
> > > >    
> > > Right, there is no version in clone3. size = version. I view this
> > > as a 1:1 lookup.
> > >     
> > > > >
> > > > > Besides breaching data abstraction, if VFIO has to check IOMMU
> > > > > flags to determine the sizes, it has many combinations.
> > > > >
> > > > > We also separate the responsibilities into two parts
> > > > > 1. compatibility - version, size by VFIO
> > > > > 2. sanity check - capability flags - by IOMMU    
> > > >
> > > > I feel argsz+flags approach can perfectly meet above
> > > > requirement. The userspace set the size and flags for whatever
> > > > capabilities it uses, and VFIO simply copies the parameters by
> > > > size and pass to IOMMU for further sanity check. Of course the
> > > > assumption is that we do provide an interface for userspace to
> > > > enumerate all supported capabilities.   
> > > You cannot trust user for argsz. the size to be copied from user
> > > must be based on knowledge in kernel. That is why we have this
> > > version to size lookup.
> > > 
> > > In VFIO, the size to copy is based on knowledge of each VFIO UAPI
> > > structures and VFIO flags. But here the flags are IOMMU UAPI
> > > flags. As you pointed out in another thread, VFIO is one user.    
> > 
> > If that is the case, can we let VFIO only copy its own UAPI fields
> > while simply passing the user pointer of IOMMU UAPI structure to
> > IOMMU driver for further size check and copy? Otherwise we are
> > entering a dead end that VFIO doesn't want to parse a structure
> > which is not defined by him while using version to represent the
> > black box size is considered as a discarded scheme and doesn't
> > scale well... 
> I think this could be an other viable option. Let me try to summarize
> since this has been a long discussion since the original version.
> 
> Problem statements:
> 1. When launching vIOMMU in the guest, how can we ensure the host has
> compatible support upfront? as compared to fail later.
> 
> 2. As UAPI data gets extended (both in size and flags), how can we
> know the size to copy
> 
> 3. Maintain backward compatibility while allowing extensions?
> 
> I think we all agreed that using flags (capability or types) is the
> way to address #3. As Christoph pointed out, version number should
> not be used for this purpose.
> 
> So for problem 1 & 2, we have the following options:
> 1. Have a version-size mapping as proposed in this set. VFIO copies
> from user the correct size based on version-type lookup. Processing
> of the data is based on flags in IOMMU driver.
> 
> 2. VFIO copy its own minsz then pass the user pointer to IOMMU driver
> for further copy_from_user based on flags. (by Kevin)
> 
> 3. Adopt VFIO argsz scheme, caller fills in argsz for the offset the
> variable size union. VFIO do not check argsz in that it requires IOMMU
> specific knowledge. IOMMU driver Use flags to handle the variable
> size.(by Alex). I think this what we have in Yi's VFIO & QEMU patch.
> argsz filled by QEMU includes bind_data.
> 
> 4. Do not use a unified version, have a fixed size of all UAPI
> structures, padding in struct and union. (Wasteful, not preferred per
> V1 discussion)
> 
> For both 2 & 3, a unified version is not used, each API
> treated separately. vIOMMU will be launched w/o assurance of
> compatibility of all APIs. Fault handling may be more complex in
> normal operations.
> 
> Appreciate everyone's input. Joerg and Alex, could you help to make a
> decision here?
> 
> 
> Thanks,
> 
> Jacob
> 
> > >     
> >  [...]  
> >  [...]  
> >  [...]    
> > > > >
> > > > > [Jacob Pan]    
> > > 
> > > [Jacob Pan]    
> > 
> > Thanks
> > Kevin  
> 
> [Jacob Pan]

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Alex Williamson]

On Mon, 13 Apr 2020 13:41:57 -0700
Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:

> Hi All,
> 
> Just a gentle reminder, any feedback on the options I listed below? New
> ideas will be even better.
> 
> Christoph, does the explanation make sense to you? We do have the
> capability/flag based scheme for IOMMU API extension, the version is
> mainly used for size lookup. Compatibility checking is another use of
> the version, it makes checking easy when a vIOMMU is launched.
> 
> Thanks,
> 
> Jacob
> 
> On Thu, 2 Apr 2020 11:36:04 -0700
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > On Wed, 1 Apr 2020 05:32:21 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >   
> > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > Sent: Tuesday, March 31, 2020 11:55 PM
> > > > 
> > > > On Tue, 31 Mar 2020 06:06:38 +0000
> > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > >       
> > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > > >
> > > > > > On Mon, 30 Mar 2020 05:40:40 +0000
> > > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > > > >      
> > > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > > >
> > > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > > Christoph Hellwig <hch@infradead.org> wrote:
> > > > > > > >      
> > > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian, Kevin
> > > > > > > > > wrote:      
> > > > > > > > > > If those API calls are inter-dependent for composing a
> > > > > > > > > > feature (e.g. SVA), shouldn't we need a way to check
> > > > > > > > > > them together before exposing the feature to the
> > > > > > > > > > guest, e.g. through a iommu_get_uapi_capabilities
> > > > > > > > > > interface?      
> > > > > > > > >
> > > > > > > > > Yes, that makes sense.  The important bit is to have a
> > > > > > > > > capability flags and not version numbers.      
> > > > > > > >
> > > > > > > > The challenge is that there are two consumers in the
> > > > > > > > kernel for this. 1. VFIO only look for compatibility, and
> > > > > > > > size of each data struct such that it can copy_from_user.
> > > > > > > >
> > > > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > > > >
> > > > > > > > For 2, I agree and we do plan to use the capability flags
> > > > > > > > to check content and maintain backward compatibility etc.
> > > > > > > >
> > > > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > > > capability flags.      
> > > > > > >
> > > > > > > Can you elaborate the difficulty in VFIO? if, as Christoph
> > > > > > > Hellwig pointed out, version number is already avoided
> > > > > > > everywhere, it is interesting to know whether this work
> > > > > > > becomes a real exception or just requires a different
> > > > > > > mindset.   
> > > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only needs
> > > > > > to do two things:
> > > > > > 1. is the UAPI compatible?
> > > > > > 2. what is the size to copy?
> > > > > >
> > > > > > If you look at the version number, this is really a "version
> > > > > > as size" lookup, as provided by the helper function in this
> > > > > > patch. An example can be the newly introduced clone3 syscall.
> > > > > > https://lwn.net/Articles/792628/
> > > > > > In clone3, new version must have new size. The slight
> > > > > > difference here is that, unlike clone3, we have multiple data
> > > > > > structures instead of a single struct clone_args {}. And each
> > > > > > struct has flags to enumerate its contents besides size.      
> > > > >
> > > > > Thanks for providing that link. However clone3 doesn't include a
> > > > > version field to do "version as size" lookup. Instead, as you
> > > > > said, it includes a size parameter which sounds like the option
> > > > > 3 (argsz) listed below.
> > > > >      
> > > > Right, there is no version in clone3. size = version. I view this
> > > > as a 1:1 lookup.
> > > >       
> > > > > >
> > > > > > Besides breaching data abstraction, if VFIO has to check IOMMU
> > > > > > flags to determine the sizes, it has many combinations.
> > > > > >
> > > > > > We also separate the responsibilities into two parts
> > > > > > 1. compatibility - version, size by VFIO
> > > > > > 2. sanity check - capability flags - by IOMMU      
> > > > >
> > > > > I feel argsz+flags approach can perfectly meet above
> > > > > requirement. The userspace set the size and flags for whatever
> > > > > capabilities it uses, and VFIO simply copies the parameters by
> > > > > size and pass to IOMMU for further sanity check. Of course the
> > > > > assumption is that we do provide an interface for userspace to
> > > > > enumerate all supported capabilities.     
> > > > You cannot trust user for argsz. the size to be copied from user
> > > > must be based on knowledge in kernel. That is why we have this
> > > > version to size lookup.
> > > > 
> > > > In VFIO, the size to copy is based on knowledge of each VFIO UAPI
> > > > structures and VFIO flags. But here the flags are IOMMU UAPI
> > > > flags. As you pointed out in another thread, VFIO is one user.      
> > > 
> > > If that is the case, can we let VFIO only copy its own UAPI fields
> > > while simply passing the user pointer of IOMMU UAPI structure to
> > > IOMMU driver for further size check and copy? Otherwise we are
> > > entering a dead end that VFIO doesn't want to parse a structure
> > > which is not defined by him while using version to represent the
> > > black box size is considered as a discarded scheme and doesn't
> > > scale well...   
> > I think this could be an other viable option. Let me try to summarize
> > since this has been a long discussion since the original version.
> > 
> > Problem statements:
> > 1. When launching vIOMMU in the guest, how can we ensure the host has
> > compatible support upfront? as compared to fail later.

This sounds like a feature/extension interface, both KVM and vfio have
them to allow userspace to check support of specific features.

> > 2. As UAPI data gets extended (both in size and flags), how can we
> > know the size to copy

For vfio we of course use the argsz/flags trick where the user tells us
how big the buffer is and flags in the header tell us what fields
beyond the base specification are enabled.  This can get tricky to
extend and there can be confusion whether a flag indicates the presence
of a field or the validity of a field.

We also have interfaces where the ioctl is a header plus a data blob
where flags tell us what the data is.  These can serve double duty as a
extension check too as we've done for VFIO_DEVICE_FEATURE.  This
doesn't really support extension of a defined feature though, rather
we'd be more likely to create a set of flags that indicate the data
object is feature-v2 and redefine the structure, or of course we
revisit the entire featuring question within the structure of that data
blob.

We also implement capability chains, though they're more meant for
passing data to the user, where the user provides a buffer and we link
capabilities together within that buffer for the user to walk.  We've
defined a mechanism through -ENOSPC and argsz to tell the user how
large a buffer is necessary.  I dare mention we have a version per
capability as these are largely modeled after capability chains in PCI
config space.  We haven't actually incremented any versions, but I
imagine we'd do so like PCI, maintaining backwards compatibility and
only defining unused bits and adding fields as the version increases.

Is the objection to a global version or to any version fields?  I don't
really understand the global version, I'd think a mechanism to check
extensions plus a per structure flags/version would be preferred.  The
former should resolve how userspace can test support for features
requiring multiple interfaces.  A global version also implies that
we're only ever adding features and never removing.  For example,
feature Foo is added in version 4, but it's replaced by feature Bar in
version 5, now userspace can't simply test version >= 4 must include
feature Foo.

It seems to me that version and flags can also be complimentary, for
example a field might be defined by a version but a flag could indicate
if it's implemented.  With only the flag, we'd infer the field from the
flag, with only the version we'd need to assume the field is always
implemented.  So I have a hard time making a blanket statement that all
versions fields should be avoided.
 
> > 3. Maintain backward compatibility while allowing extensions?
> > 
> > I think we all agreed that using flags (capability or types) is the
> > way to address #3. As Christoph pointed out, version number should
> > not be used for this purpose.
> > 
> > So for problem 1 & 2, we have the following options:
> > 1. Have a version-size mapping as proposed in this set. VFIO copies
> > from user the correct size based on version-type lookup. Processing
> > of the data is based on flags in IOMMU driver.
> > 
> > 2. VFIO copy its own minsz then pass the user pointer to IOMMU driver
> > for further copy_from_user based on flags. (by Kevin)
> > 
> > 3. Adopt VFIO argsz scheme, caller fills in argsz for the offset the
> > variable size union. VFIO do not check argsz in that it requires IOMMU
> > specific knowledge. IOMMU driver Use flags to handle the variable
> > size.(by Alex). I think this what we have in Yi's VFIO & QEMU patch.
> > argsz filled by QEMU includes bind_data.
> > 
> > 4. Do not use a unified version, have a fixed size of all UAPI
> > structures, padding in struct and union. (Wasteful, not preferred per
> > V1 discussion)
> > 
> > For both 2 & 3, a unified version is not used, each API
> > treated separately. vIOMMU will be launched w/o assurance of
> > compatibility of all APIs. Fault handling may be more complex in
> > normal operations.
> > 
> > Appreciate everyone's input. Joerg and Alex, could you help to make a
> > decision here?

As above, I think using a global API version number to imply support
for a feature is doomed to fail, we should instead expose an interface
to check for specific features.  In any of the proposed solutions, the
IOMMU driver is ultimately responsible for validating the user data, so
do we want vfio performing the copy_from_user() to an object that could
later be assumed to be sanitized, or should vfio just pass a user
pointer to make it obvious that the consumer is responsible for all the
user protections?  Seems like the latter.  That still really doesn't
address what's in that user data blob yet, but the vfio interface could
be:

struct {
	__u32 argsz;
	__u32 flags;
	__u8  data[];
}

Where flags might be partitioned like we do for DEVICE_FEATURE to
indicate the format of data and what vfio should do with it, and data
might simply be defined as a (__u64 __user *).

This user pointer would then likely be an IOMMU UAPI struct, so I've
only just gotten back the the IOMMU UAPI question at hand, but I don't
really see the disadvantage to including both version and flags fields
per structure.  Perhaps this is choice 1. above, but with a version at
a per structure level indicating the backwards compatible size and
layout of the structure and flags being used to indicate support for
optional features within those fields.  Is a version field still taboo
for such a use case?  Thanks,

Alex

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

Hi Alex,
Thanks a lot for the feedback, my comments inline.

On Mon, 13 Apr 2020 16:21:29 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon, 13 Apr 2020 13:41:57 -0700
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > Hi All,
> > 
> > Just a gentle reminder, any feedback on the options I listed below?
> > New ideas will be even better.
> > 
> > Christoph, does the explanation make sense to you? We do have the
> > capability/flag based scheme for IOMMU API extension, the version is
> > mainly used for size lookup. Compatibility checking is another use
> > of the version, it makes checking easy when a vIOMMU is launched.
> > 
> > Thanks,
> > 
> > Jacob
> > 
> > On Thu, 2 Apr 2020 11:36:04 -0700
> > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> >   
> > > On Wed, 1 Apr 2020 05:32:21 +0000
> > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > >     
> > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > Sent: Tuesday, March 31, 2020 11:55 PM
> > > > > 
> > > > > On Tue, 31 Mar 2020 06:06:38 +0000
> > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > > >         
> > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > > > >
> > > > > > > On Mon, 30 Mar 2020 05:40:40 +0000
> > > > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > > > > >        
> > > > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > > > >
> > > > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > > > Christoph Hellwig <hch@infradead.org> wrote:
> > > > > > > > >        
> > > > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian,
> > > > > > > > > > Kevin wrote:        
> > > > > > > > > > > If those API calls are inter-dependent for
> > > > > > > > > > > composing a feature (e.g. SVA), shouldn't we need
> > > > > > > > > > > a way to check them together before exposing the
> > > > > > > > > > > feature to the guest, e.g. through a
> > > > > > > > > > > iommu_get_uapi_capabilities interface?        
> > > > > > > > > >
> > > > > > > > > > Yes, that makes sense.  The important bit is to
> > > > > > > > > > have a capability flags and not version
> > > > > > > > > > numbers.        
> > > > > > > > >
> > > > > > > > > The challenge is that there are two consumers in the
> > > > > > > > > kernel for this. 1. VFIO only look for compatibility,
> > > > > > > > > and size of each data struct such that it can
> > > > > > > > > copy_from_user.
> > > > > > > > >
> > > > > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > > > > >
> > > > > > > > > For 2, I agree and we do plan to use the capability
> > > > > > > > > flags to check content and maintain backward
> > > > > > > > > compatibility etc.
> > > > > > > > >
> > > > > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > > > > capability flags.        
> > > > > > > >
> > > > > > > > Can you elaborate the difficulty in VFIO? if, as
> > > > > > > > Christoph Hellwig pointed out, version number is
> > > > > > > > already avoided everywhere, it is interesting to know
> > > > > > > > whether this work becomes a real exception or just
> > > > > > > > requires a different mindset.     
> > > > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only
> > > > > > > needs to do two things:
> > > > > > > 1. is the UAPI compatible?
> > > > > > > 2. what is the size to copy?
> > > > > > >
> > > > > > > If you look at the version number, this is really a
> > > > > > > "version as size" lookup, as provided by the helper
> > > > > > > function in this patch. An example can be the newly
> > > > > > > introduced clone3 syscall.
> > > > > > > https://lwn.net/Articles/792628/ In clone3, new version
> > > > > > > must have new size. The slight difference here is that,
> > > > > > > unlike clone3, we have multiple data structures instead
> > > > > > > of a single struct clone_args {}. And each struct has
> > > > > > > flags to enumerate its contents besides size.        
> > > > > >
> > > > > > Thanks for providing that link. However clone3 doesn't
> > > > > > include a version field to do "version as size" lookup.
> > > > > > Instead, as you said, it includes a size parameter which
> > > > > > sounds like the option 3 (argsz) listed below.
> > > > > >        
> > > > > Right, there is no version in clone3. size = version. I view
> > > > > this as a 1:1 lookup.
> > > > >         
> > > > > > >
> > > > > > > Besides breaching data abstraction, if VFIO has to check
> > > > > > > IOMMU flags to determine the sizes, it has many
> > > > > > > combinations.
> > > > > > >
> > > > > > > We also separate the responsibilities into two parts
> > > > > > > 1. compatibility - version, size by VFIO
> > > > > > > 2. sanity check - capability flags - by IOMMU        
> > > > > >
> > > > > > I feel argsz+flags approach can perfectly meet above
> > > > > > requirement. The userspace set the size and flags for
> > > > > > whatever capabilities it uses, and VFIO simply copies the
> > > > > > parameters by size and pass to IOMMU for further sanity
> > > > > > check. Of course the assumption is that we do provide an
> > > > > > interface for userspace to enumerate all supported
> > > > > > capabilities.       
> > > > > You cannot trust user for argsz. the size to be copied from
> > > > > user must be based on knowledge in kernel. That is why we
> > > > > have this version to size lookup.
> > > > > 
> > > > > In VFIO, the size to copy is based on knowledge of each VFIO
> > > > > UAPI structures and VFIO flags. But here the flags are IOMMU
> > > > > UAPI flags. As you pointed out in another thread, VFIO is one
> > > > > user.        
> > > > 
> > > > If that is the case, can we let VFIO only copy its own UAPI
> > > > fields while simply passing the user pointer of IOMMU UAPI
> > > > structure to IOMMU driver for further size check and copy?
> > > > Otherwise we are entering a dead end that VFIO doesn't want to
> > > > parse a structure which is not defined by him while using
> > > > version to represent the black box size is considered as a
> > > > discarded scheme and doesn't scale well...     
> > > I think this could be an other viable option. Let me try to
> > > summarize since this has been a long discussion since the
> > > original version.
> > > 
> > > Problem statements:
> > > 1. When launching vIOMMU in the guest, how can we ensure the host
> > > has compatible support upfront? as compared to fail later.  
> 
> This sounds like a feature/extension interface, both KVM and vfio have
> them to allow userspace to check support of specific features.
> 
Yes, the specific features are the APIs:
- bind guest PASID
- cache invalidation
- page requests & response

> > > 2. As UAPI data gets extended (both in size and flags), how can we
> > > know the size to copy  
> 
> For vfio we of course use the argsz/flags trick where the user tells
> us how big the buffer is and flags in the header tell us what fields
> beyond the base specification are enabled.  This can get tricky to
> extend and there can be confusion whether a flag indicates the
> presence of a field or the validity of a field.
> 
> We also have interfaces where the ioctl is a header plus a data blob
> where flags tell us what the data is.  These can serve double duty as
> a extension check too as we've done for VFIO_DEVICE_FEATURE.  This
> doesn't really support extension of a defined feature though, rather
> we'd be more likely to create a set of flags that indicate the data
> object is feature-v2 and redefine the structure, or of course we
> revisit the entire featuring question within the structure of that
> data blob.
> 
> We also implement capability chains, though they're more meant for
> passing data to the user, where the user provides a buffer and we link
> capabilities together within that buffer for the user to walk.  We've
> defined a mechanism through -ENOSPC and argsz to tell the user how
> large a buffer is necessary.  I dare mention we have a version per
> capability as these are largely modeled after capability chains in PCI
> config space.  We haven't actually incremented any versions, but I
> imagine we'd do so like PCI, maintaining backwards compatibility and
> only defining unused bits and adding fields as the version increases.
> 
I guess capability chain is more suitable since the IOCTL uses
container FD instead of device FD in VFIO_DEVICE_FEATURE?

We can give that a try by treating IOMMU UAPIs as capabilities. That
would address problem #1. We really need to check compatibility upfront
in that there is no way to fail some of the UAPIs. e.g. unbind guest
PASID, cache invalidation.

> Is the objection to a global version or to any version fields?  I
> don't really understand the global version, I'd think a mechanism to
> check extensions plus a per structure flags/version would be
> preferred.  The former should resolve how userspace can test support
> for features requiring multiple interfaces.
Currently we already have individual version & flags per UAPI data. The
reason why I introduced a global/unifier is to simplify the
compatibility checking. Global version is optional.
With individual version & flags, user may have to keep track of
combinations of per structure versions.

>  A global version also
> implies that we're only ever adding features and never removing.  For
> example, feature Foo is added in version 4, but it's replaced by
> feature Bar in version 5, now userspace can't simply test version >=
> 4 must include feature Foo.
> 
Yes, this is why I was hoping to stick with the rule: open for
extension, closed to modification.
It also makes the code backward compatible easy since there old code
would have no change when adding new features.

> It seems to me that version and flags can also be complimentary, for
> example a field might be defined by a version but a flag could
> indicate if it's implemented.  With only the flag, we'd infer the
> field from the flag, with only the version we'd need to assume the
> field is always implemented.  So I have a hard time making a blanket
> statement that all versions fields should be avoided.
>  
> > > 3. Maintain backward compatibility while allowing extensions?
> > > 
> > > I think we all agreed that using flags (capability or types) is
> > > the way to address #3. As Christoph pointed out, version number
> > > should not be used for this purpose.
> > > 
> > > So for problem 1 & 2, we have the following options:
> > > 1. Have a version-size mapping as proposed in this set. VFIO
> > > copies from user the correct size based on version-type lookup.
> > > Processing of the data is based on flags in IOMMU driver.
> > > 
> > > 2. VFIO copy its own minsz then pass the user pointer to IOMMU
> > > driver for further copy_from_user based on flags. (by Kevin)
> > > 
> > > 3. Adopt VFIO argsz scheme, caller fills in argsz for the offset
> > > the variable size union. VFIO do not check argsz in that it
> > > requires IOMMU specific knowledge. IOMMU driver Use flags to
> > > handle the variable size.(by Alex). I think this what we have in
> > > Yi's VFIO & QEMU patch. argsz filled by QEMU includes bind_data.
> > > 
> > > 4. Do not use a unified version, have a fixed size of all UAPI
> > > structures, padding in struct and union. (Wasteful, not preferred
> > > per V1 discussion)
> > > 
> > > For both 2 & 3, a unified version is not used, each API
> > > treated separately. vIOMMU will be launched w/o assurance of
> > > compatibility of all APIs. Fault handling may be more complex in
> > > normal operations.
> > > 
> > > Appreciate everyone's input. Joerg and Alex, could you help to
> > > make a decision here?  
> 
> As above, I think using a global API version number to imply support
> for a feature is doomed to fail, we should instead expose an interface
> to check for specific features.
I agree. I feel we can use the capability chain at container level as
you mentioned, right?

>  In any of the proposed solutions, the
> IOMMU driver is ultimately responsible for validating the user data,
> so do we want vfio performing the copy_from_user() to an object that
> could later be assumed to be sanitized, or should vfio just pass a
> user pointer to make it obvious that the consumer is responsible for
> all the user protections?  Seems like the latter.
I like the latter as well.

>  That still really
> doesn't address what's in that user data blob yet, but the vfio
> interface could be:
> 
> struct {
> 	__u32 argsz;
> 	__u32 flags;
> 	__u8  data[];
> }
> 
> Where flags might be partitioned like we do for DEVICE_FEATURE to
> indicate the format of data and what vfio should do with it, and data
> might simply be defined as a (__u64 __user *).
> 
So, __user * will be passed to IOMMU driver if VFIO checks minsz
include flags and they are valid.
IOMMU driver can copy the rest based on the mandatory version/minsz and
flags in the IOMMU uAPI structs.
Does it sound right? This is really choice #2.

> This user pointer would then likely be an IOMMU UAPI struct, so I've
> only just gotten back the the IOMMU UAPI question at hand, but I don't
> really see the disadvantage to including both version and flags fields
> per structure.  Perhaps this is choice 1. above, but with a version at
> a per structure level indicating the backwards compatible size and
> layout of the structure and flags being used to indicate support for
> optional features within those fields.

Per structure version & flags is what we have in the mainline. It
applies to both choice 1 & 2. The global/unified version is just a
re-interpretation of the versions such that we have a monolithic
incrementing version. Again, global version is optional.

>  Is a version field still taboo
> for such a use case?  Thanks,

I will leave that to Christoph :)

Thanks

Jacob
> 
> Alex
> 

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Christoph Hellwig]

On Mon, Apr 13, 2020 at 01:41:57PM -0700, Jacob Pan wrote:
> Hi All,
> 
> Just a gentle reminder, any feedback on the options I listed below? New
> ideas will be even better.
> 
> Christoph, does the explanation make sense to you? We do have the
> capability/flag based scheme for IOMMU API extension, the version is
> mainly used for size lookup. Compatibility checking is another use of
> the version, it makes checking easy when a vIOMMU is launched.

No.  If you truely need different versions use different ioctl
identifiers.  If it really is just the size pass the size and not a
version.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Christoph Hellwig]

On Mon, Apr 13, 2020 at 04:21:29PM -0600, Alex Williamson wrote:
> Is the objection to a global version or to any version fields?  I don't
> really understand the global version, I'd think a mechanism to check
> extensions plus a per structure flags/version would be preferred.  The
> former should resolve how userspace can test support for features
> requiring multiple interfaces.  A global version also implies that
> we're only ever adding features and never removing.  For example,
> feature Foo is added in version 4, but it's replaced by feature Bar in
> version 5, now userspace can't simply test version >= 4 must include
> feature Foo.

The objection is to versions vs the much more sensible struct size +
capability flags.  Making it global just increases the problems with
a version for all of the above reasons.

> It seems to me that version and flags can also be complimentary, for
> example a field might be defined by a version but a flag could indicate
> if it's implemented.  With only the flag, we'd infer the field from the
> flag, with only the version we'd need to assume the field is always
> implemented.  So I have a hard time making a blanket statement that all
> versions fields should be avoided.

s/version/struct size/, but otherwise agreed.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

On Tue, 14 Apr 2020 01:11:07 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Apr 13, 2020 at 01:41:57PM -0700, Jacob Pan wrote:
> > Hi All,
> > 
> > Just a gentle reminder, any feedback on the options I listed below?
> > New ideas will be even better.
> > 
> > Christoph, does the explanation make sense to you? We do have the
> > capability/flag based scheme for IOMMU API extension, the version is
> > mainly used for size lookup. Compatibility checking is another use
> > of the version, it makes checking easy when a vIOMMU is launched.  
> 
> No.  If you truely need different versions use different ioctl
> identifiers.
OK. I will drop the global version and keep the current per API/IOCTL
struct.

>  If it really is just the size pass the size and not a
> version.
OK, I think we have a path forward. I will remove the size-version
lookup.

My concern was that since we cannot trust the size provided by
userspace. We must sanity check the argsz based on knowledge of the
struct within the kernel. AFAIK, VFIO does this check by looking at
offsetofend(user_struct, last_element). But in this case, VFIO is not
the end consumer, and last_element can be a variable size union.
So we'd better let IOMMU driver deal with it.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Alex Williamson]

On Mon, 13 Apr 2020 22:05:15 -0700
Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:

> Hi Alex,
> Thanks a lot for the feedback, my comments inline.
> 
> On Mon, 13 Apr 2020 16:21:29 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > On Mon, 13 Apr 2020 13:41:57 -0700
> > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> >   
> > > Hi All,
> > > 
> > > Just a gentle reminder, any feedback on the options I listed below?
> > > New ideas will be even better.
> > > 
> > > Christoph, does the explanation make sense to you? We do have the
> > > capability/flag based scheme for IOMMU API extension, the version is
> > > mainly used for size lookup. Compatibility checking is another use
> > > of the version, it makes checking easy when a vIOMMU is launched.
> > > 
> > > Thanks,
> > > 
> > > Jacob
> > > 
> > > On Thu, 2 Apr 2020 11:36:04 -0700
> > > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> > >     
> > > > On Wed, 1 Apr 2020 05:32:21 +0000
> > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > >       
> > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > Sent: Tuesday, March 31, 2020 11:55 PM
> > > > > > 
> > > > > > On Tue, 31 Mar 2020 06:06:38 +0000
> > > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > > > >           
> > > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > > > > >
> > > > > > > > On Mon, 30 Mar 2020 05:40:40 +0000
> > > > > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > > > > > >          
> > > > > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > > > > >
> > > > > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > > > > Christoph Hellwig <hch@infradead.org> wrote:
> > > > > > > > > >          
> > > > > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian,
> > > > > > > > > > > Kevin wrote:          
> > > > > > > > > > > > If those API calls are inter-dependent for
> > > > > > > > > > > > composing a feature (e.g. SVA), shouldn't we need
> > > > > > > > > > > > a way to check them together before exposing the
> > > > > > > > > > > > feature to the guest, e.g. through a
> > > > > > > > > > > > iommu_get_uapi_capabilities interface?          
> > > > > > > > > > >
> > > > > > > > > > > Yes, that makes sense.  The important bit is to
> > > > > > > > > > > have a capability flags and not version
> > > > > > > > > > > numbers.          
> > > > > > > > > >
> > > > > > > > > > The challenge is that there are two consumers in the
> > > > > > > > > > kernel for this. 1. VFIO only look for compatibility,
> > > > > > > > > > and size of each data struct such that it can
> > > > > > > > > > copy_from_user.
> > > > > > > > > >
> > > > > > > > > > 2. IOMMU driver, the "real consumer" of the content.
> > > > > > > > > >
> > > > > > > > > > For 2, I agree and we do plan to use the capability
> > > > > > > > > > flags to check content and maintain backward
> > > > > > > > > > compatibility etc.
> > > > > > > > > >
> > > > > > > > > > For VFIO, it is difficult to do size look up based on
> > > > > > > > > > capability flags.          
> > > > > > > > >
> > > > > > > > > Can you elaborate the difficulty in VFIO? if, as
> > > > > > > > > Christoph Hellwig pointed out, version number is
> > > > > > > > > already avoided everywhere, it is interesting to know
> > > > > > > > > whether this work becomes a real exception or just
> > > > > > > > > requires a different mindset.       
> > > > > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it only
> > > > > > > > needs to do two things:
> > > > > > > > 1. is the UAPI compatible?
> > > > > > > > 2. what is the size to copy?
> > > > > > > >
> > > > > > > > If you look at the version number, this is really a
> > > > > > > > "version as size" lookup, as provided by the helper
> > > > > > > > function in this patch. An example can be the newly
> > > > > > > > introduced clone3 syscall.
> > > > > > > > https://lwn.net/Articles/792628/ In clone3, new version
> > > > > > > > must have new size. The slight difference here is that,
> > > > > > > > unlike clone3, we have multiple data structures instead
> > > > > > > > of a single struct clone_args {}. And each struct has
> > > > > > > > flags to enumerate its contents besides size.          
> > > > > > >
> > > > > > > Thanks for providing that link. However clone3 doesn't
> > > > > > > include a version field to do "version as size" lookup.
> > > > > > > Instead, as you said, it includes a size parameter which
> > > > > > > sounds like the option 3 (argsz) listed below.
> > > > > > >          
> > > > > > Right, there is no version in clone3. size = version. I view
> > > > > > this as a 1:1 lookup.
> > > > > >           
> > > > > > > >
> > > > > > > > Besides breaching data abstraction, if VFIO has to check
> > > > > > > > IOMMU flags to determine the sizes, it has many
> > > > > > > > combinations.
> > > > > > > >
> > > > > > > > We also separate the responsibilities into two parts
> > > > > > > > 1. compatibility - version, size by VFIO
> > > > > > > > 2. sanity check - capability flags - by IOMMU          
> > > > > > >
> > > > > > > I feel argsz+flags approach can perfectly meet above
> > > > > > > requirement. The userspace set the size and flags for
> > > > > > > whatever capabilities it uses, and VFIO simply copies the
> > > > > > > parameters by size and pass to IOMMU for further sanity
> > > > > > > check. Of course the assumption is that we do provide an
> > > > > > > interface for userspace to enumerate all supported
> > > > > > > capabilities.         
> > > > > > You cannot trust user for argsz. the size to be copied from
> > > > > > user must be based on knowledge in kernel. That is why we
> > > > > > have this version to size lookup.
> > > > > > 
> > > > > > In VFIO, the size to copy is based on knowledge of each VFIO
> > > > > > UAPI structures and VFIO flags. But here the flags are IOMMU
> > > > > > UAPI flags. As you pointed out in another thread, VFIO is one
> > > > > > user.          
> > > > > 
> > > > > If that is the case, can we let VFIO only copy its own UAPI
> > > > > fields while simply passing the user pointer of IOMMU UAPI
> > > > > structure to IOMMU driver for further size check and copy?
> > > > > Otherwise we are entering a dead end that VFIO doesn't want to
> > > > > parse a structure which is not defined by him while using
> > > > > version to represent the black box size is considered as a
> > > > > discarded scheme and doesn't scale well...       
> > > > I think this could be an other viable option. Let me try to
> > > > summarize since this has been a long discussion since the
> > > > original version.
> > > > 
> > > > Problem statements:
> > > > 1. When launching vIOMMU in the guest, how can we ensure the host
> > > > has compatible support upfront? as compared to fail later.    
> > 
> > This sounds like a feature/extension interface, both KVM and vfio have
> > them to allow userspace to check support of specific features.
> >   
> Yes, the specific features are the APIs:
> - bind guest PASID
> - cache invalidation
> - page requests & response
> 
> > > > 2. As UAPI data gets extended (both in size and flags), how can we
> > > > know the size to copy    
> > 
> > For vfio we of course use the argsz/flags trick where the user tells
> > us how big the buffer is and flags in the header tell us what fields
> > beyond the base specification are enabled.  This can get tricky to
> > extend and there can be confusion whether a flag indicates the
> > presence of a field or the validity of a field.
> > 
> > We also have interfaces where the ioctl is a header plus a data blob
> > where flags tell us what the data is.  These can serve double duty as
> > a extension check too as we've done for VFIO_DEVICE_FEATURE.  This
> > doesn't really support extension of a defined feature though, rather
> > we'd be more likely to create a set of flags that indicate the data
> > object is feature-v2 and redefine the structure, or of course we
> > revisit the entire featuring question within the structure of that
> > data blob.
> > 
> > We also implement capability chains, though they're more meant for
> > passing data to the user, where the user provides a buffer and we link
> > capabilities together within that buffer for the user to walk.  We've
> > defined a mechanism through -ENOSPC and argsz to tell the user how
> > large a buffer is necessary.  I dare mention we have a version per
> > capability as these are largely modeled after capability chains in PCI
> > config space.  We haven't actually incremented any versions, but I
> > imagine we'd do so like PCI, maintaining backwards compatibility and
> > only defining unused bits and adding fields as the version increases.
> >   
> I guess capability chain is more suitable since the IOCTL uses
> container FD instead of device FD in VFIO_DEVICE_FEATURE?
> 
> We can give that a try by treating IOMMU UAPIs as capabilities. That
> would address problem #1. We really need to check compatibility upfront
> in that there is no way to fail some of the UAPIs. e.g. unbind guest
> PASID, cache invalidation.
> 
> > Is the objection to a global version or to any version fields?  I
> > don't really understand the global version, I'd think a mechanism to
> > check extensions plus a per structure flags/version would be
> > preferred.  The former should resolve how userspace can test support
> > for features requiring multiple interfaces.  
> Currently we already have individual version & flags per UAPI data. The
> reason why I introduced a global/unifier is to simplify the
> compatibility checking. Global version is optional.
> With individual version & flags, user may have to keep track of
> combinations of per structure versions.

A user would only need to meet the minimum uapi matching the feature
they want to support, right?
 
> >  A global version also
> > implies that we're only ever adding features and never removing.  For
> > example, feature Foo is added in version 4, but it's replaced by
> > feature Bar in version 5, now userspace can't simply test version >=
> > 4 must include feature Foo.
> >   
> Yes, this is why I was hoping to stick with the rule: open for
> extension, closed to modification.
> It also makes the code backward compatible easy since there old code
> would have no change when adding new features.

But we need a way to deprecate things.  The version interface is not
good for that.

> > It seems to me that version and flags can also be complimentary, for
> > example a field might be defined by a version but a flag could
> > indicate if it's implemented.  With only the flag, we'd infer the
> > field from the flag, with only the version we'd need to assume the
> > field is always implemented.  So I have a hard time making a blanket
> > statement that all versions fields should be avoided.
> >    
> > > > 3. Maintain backward compatibility while allowing extensions?
> > > > 
> > > > I think we all agreed that using flags (capability or types) is
> > > > the way to address #3. As Christoph pointed out, version number
> > > > should not be used for this purpose.
> > > > 
> > > > So for problem 1 & 2, we have the following options:
> > > > 1. Have a version-size mapping as proposed in this set. VFIO
> > > > copies from user the correct size based on version-type lookup.
> > > > Processing of the data is based on flags in IOMMU driver.
> > > > 
> > > > 2. VFIO copy its own minsz then pass the user pointer to IOMMU
> > > > driver for further copy_from_user based on flags. (by Kevin)
> > > > 
> > > > 3. Adopt VFIO argsz scheme, caller fills in argsz for the offset
> > > > the variable size union. VFIO do not check argsz in that it
> > > > requires IOMMU specific knowledge. IOMMU driver Use flags to
> > > > handle the variable size.(by Alex). I think this what we have in
> > > > Yi's VFIO & QEMU patch. argsz filled by QEMU includes bind_data.
> > > > 
> > > > 4. Do not use a unified version, have a fixed size of all UAPI
> > > > structures, padding in struct and union. (Wasteful, not preferred
> > > > per V1 discussion)
> > > > 
> > > > For both 2 & 3, a unified version is not used, each API
> > > > treated separately. vIOMMU will be launched w/o assurance of
> > > > compatibility of all APIs. Fault handling may be more complex in
> > > > normal operations.
> > > > 
> > > > Appreciate everyone's input. Joerg and Alex, could you help to
> > > > make a decision here?    
> > 
> > As above, I think using a global API version number to imply support
> > for a feature is doomed to fail, we should instead expose an interface
> > to check for specific features.  
> I agree. I feel we can use the capability chain at container level as
> you mentioned, right?

We could either do a capability chain on the IOMMU_GET_INFO ioctl or we
have the CHECK_EXTENSION ioctl.  The latter is quite a bit easier to
implement, but limited in what it can expose and the semantics around
checking extensions before vs after an IOMMU model is set can be
confusing.  I didn't have a strong preference between them when
suggesting Kirti use one of these to indicate dirty page tracking, but
it would be nice if both features used the same mechanism

> >  In any of the proposed solutions, the
> > IOMMU driver is ultimately responsible for validating the user data,
> > so do we want vfio performing the copy_from_user() to an object that
> > could later be assumed to be sanitized, or should vfio just pass a
> > user pointer to make it obvious that the consumer is responsible for
> > all the user protections?  Seems like the latter.  
> I like the latter as well.
> 
> >  That still really
> > doesn't address what's in that user data blob yet, but the vfio
> > interface could be:
> > 
> > struct {
> > 	__u32 argsz;
> > 	__u32 flags;
> > 	__u8  data[];
> > }
> > 
> > Where flags might be partitioned like we do for DEVICE_FEATURE to
> > indicate the format of data and what vfio should do with it, and data
> > might simply be defined as a (__u64 __user *).
> >   
> So, __user * will be passed to IOMMU driver if VFIO checks minsz
> include flags and they are valid.
> IOMMU driver can copy the rest based on the mandatory version/minsz and
> flags in the IOMMU uAPI structs.
> Does it sound right? This is really choice #2.

Sounds like each IOMMU UAPI struct just needs to have an embedded size
and flags field, but yes.
 
> > This user pointer would then likely be an IOMMU UAPI struct, so I've
> > only just gotten back the the IOMMU UAPI question at hand, but I don't
> > really see the disadvantage to including both version and flags fields
> > per structure.  Perhaps this is choice 1. above, but with a version at
> > a per structure level indicating the backwards compatible size and
> > layout of the structure and flags being used to indicate support for
> > optional features within those fields.  
> 
> Per structure version & flags is what we have in the mainline. It
> applies to both choice 1 & 2. The global/unified version is just a
> re-interpretation of the versions such that we have a monolithic
> incrementing version. Again, global version is optional.
> 
> >  Is a version field still taboo
> > for such a use case?  Thanks,  
> 
> I will leave that to Christoph :)

Looks like if we drop the global version in favor of an
extension/feature check and embed size and flags fields in each IOMMU
UAPI struct, Christoph will be satisfied.  Thanks,

Alex

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

On Tue, 14 Apr 2020 10:13:58 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon, 13 Apr 2020 22:05:15 -0700
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > Hi Alex,
> > Thanks a lot for the feedback, my comments inline.
> > 
> > On Mon, 13 Apr 2020 16:21:29 -0600
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >   
> > > On Mon, 13 Apr 2020 13:41:57 -0700
> > > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> > >     
> > > > Hi All,
> > > > 
> > > > Just a gentle reminder, any feedback on the options I listed
> > > > below? New ideas will be even better.
> > > > 
> > > > Christoph, does the explanation make sense to you? We do have
> > > > the capability/flag based scheme for IOMMU API extension, the
> > > > version is mainly used for size lookup. Compatibility checking
> > > > is another use of the version, it makes checking easy when a
> > > > vIOMMU is launched.
> > > > 
> > > > Thanks,
> > > > 
> > > > Jacob
> > > > 
> > > > On Thu, 2 Apr 2020 11:36:04 -0700
> > > > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> > > >       
> > > > > On Wed, 1 Apr 2020 05:32:21 +0000
> > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > > >         
> > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > > Sent: Tuesday, March 31, 2020 11:55 PM
> > > > > > > 
> > > > > > > On Tue, 31 Mar 2020 06:06:38 +0000
> > > > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > > > > >             
> > > > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > > > > Sent: Tuesday, March 31, 2020 12:08 AM
> > > > > > > > >
> > > > > > > > > On Mon, 30 Mar 2020 05:40:40 +0000
> > > > > > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > > > > > > > >            
> > > > > > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > > > > > > > > Sent: Saturday, March 28, 2020 7:54 AM
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 27 Mar 2020 00:47:02 -0700
> > > > > > > > > > > Christoph Hellwig <hch@infradead.org> wrote:
> > > > > > > > > > >            
> > > > > > > > > > > > On Fri, Mar 27, 2020 at 02:49:55AM +0000, Tian,
> > > > > > > > > > > > Kevin wrote:            
> > > > > > > > > > > > > If those API calls are inter-dependent for
> > > > > > > > > > > > > composing a feature (e.g. SVA), shouldn't we
> > > > > > > > > > > > > need a way to check them together before
> > > > > > > > > > > > > exposing the feature to the guest, e.g.
> > > > > > > > > > > > > through a iommu_get_uapi_capabilities
> > > > > > > > > > > > > interface?            
> > > > > > > > > > > >
> > > > > > > > > > > > Yes, that makes sense.  The important bit is to
> > > > > > > > > > > > have a capability flags and not version
> > > > > > > > > > > > numbers.            
> > > > > > > > > > >
> > > > > > > > > > > The challenge is that there are two consumers in
> > > > > > > > > > > the kernel for this. 1. VFIO only look for
> > > > > > > > > > > compatibility, and size of each data struct such
> > > > > > > > > > > that it can copy_from_user.
> > > > > > > > > > >
> > > > > > > > > > > 2. IOMMU driver, the "real consumer" of the
> > > > > > > > > > > content.
> > > > > > > > > > >
> > > > > > > > > > > For 2, I agree and we do plan to use the
> > > > > > > > > > > capability flags to check content and maintain
> > > > > > > > > > > backward compatibility etc.
> > > > > > > > > > >
> > > > > > > > > > > For VFIO, it is difficult to do size look up
> > > > > > > > > > > based on capability flags.            
> > > > > > > > > >
> > > > > > > > > > Can you elaborate the difficulty in VFIO? if, as
> > > > > > > > > > Christoph Hellwig pointed out, version number is
> > > > > > > > > > already avoided everywhere, it is interesting to
> > > > > > > > > > know whether this work becomes a real exception or
> > > > > > > > > > just requires a different mindset.         
> > > > > > > > > From VFIO p.o.v. the IOMMU UAPI data is opaque, it
> > > > > > > > > only needs to do two things:
> > > > > > > > > 1. is the UAPI compatible?
> > > > > > > > > 2. what is the size to copy?
> > > > > > > > >
> > > > > > > > > If you look at the version number, this is really a
> > > > > > > > > "version as size" lookup, as provided by the helper
> > > > > > > > > function in this patch. An example can be the newly
> > > > > > > > > introduced clone3 syscall.
> > > > > > > > > https://lwn.net/Articles/792628/ In clone3, new
> > > > > > > > > version must have new size. The slight difference
> > > > > > > > > here is that, unlike clone3, we have multiple data
> > > > > > > > > structures instead of a single struct clone_args {}.
> > > > > > > > > And each struct has flags to enumerate its contents
> > > > > > > > > besides size.            
> > > > > > > >
> > > > > > > > Thanks for providing that link. However clone3 doesn't
> > > > > > > > include a version field to do "version as size" lookup.
> > > > > > > > Instead, as you said, it includes a size parameter which
> > > > > > > > sounds like the option 3 (argsz) listed below.
> > > > > > > >            
> > > > > > > Right, there is no version in clone3. size = version. I
> > > > > > > view this as a 1:1 lookup.
> > > > > > >             
> > > > > > > > >
> > > > > > > > > Besides breaching data abstraction, if VFIO has to
> > > > > > > > > check IOMMU flags to determine the sizes, it has many
> > > > > > > > > combinations.
> > > > > > > > >
> > > > > > > > > We also separate the responsibilities into two parts
> > > > > > > > > 1. compatibility - version, size by VFIO
> > > > > > > > > 2. sanity check - capability flags - by
> > > > > > > > > IOMMU            
> > > > > > > >
> > > > > > > > I feel argsz+flags approach can perfectly meet above
> > > > > > > > requirement. The userspace set the size and flags for
> > > > > > > > whatever capabilities it uses, and VFIO simply copies
> > > > > > > > the parameters by size and pass to IOMMU for further
> > > > > > > > sanity check. Of course the assumption is that we do
> > > > > > > > provide an interface for userspace to enumerate all
> > > > > > > > supported capabilities.           
> > > > > > > You cannot trust user for argsz. the size to be copied
> > > > > > > from user must be based on knowledge in kernel. That is
> > > > > > > why we have this version to size lookup.
> > > > > > > 
> > > > > > > In VFIO, the size to copy is based on knowledge of each
> > > > > > > VFIO UAPI structures and VFIO flags. But here the flags
> > > > > > > are IOMMU UAPI flags. As you pointed out in another
> > > > > > > thread, VFIO is one user.            
> > > > > > 
> > > > > > If that is the case, can we let VFIO only copy its own UAPI
> > > > > > fields while simply passing the user pointer of IOMMU UAPI
> > > > > > structure to IOMMU driver for further size check and copy?
> > > > > > Otherwise we are entering a dead end that VFIO doesn't want
> > > > > > to parse a structure which is not defined by him while using
> > > > > > version to represent the black box size is considered as a
> > > > > > discarded scheme and doesn't scale well...         
> > > > > I think this could be an other viable option. Let me try to
> > > > > summarize since this has been a long discussion since the
> > > > > original version.
> > > > > 
> > > > > Problem statements:
> > > > > 1. When launching vIOMMU in the guest, how can we ensure the
> > > > > host has compatible support upfront? as compared to fail
> > > > > later.      
> > > 
> > > This sounds like a feature/extension interface, both KVM and vfio
> > > have them to allow userspace to check support of specific
> > > features. 
> > Yes, the specific features are the APIs:
> > - bind guest PASID
> > - cache invalidation
> > - page requests & response
> >   
> > > > > 2. As UAPI data gets extended (both in size and flags), how
> > > > > can we know the size to copy      
> > > 
> > > For vfio we of course use the argsz/flags trick where the user
> > > tells us how big the buffer is and flags in the header tell us
> > > what fields beyond the base specification are enabled.  This can
> > > get tricky to extend and there can be confusion whether a flag
> > > indicates the presence of a field or the validity of a field.
> > > 
> > > We also have interfaces where the ioctl is a header plus a data
> > > blob where flags tell us what the data is.  These can serve
> > > double duty as a extension check too as we've done for
> > > VFIO_DEVICE_FEATURE.  This doesn't really support extension of a
> > > defined feature though, rather we'd be more likely to create a
> > > set of flags that indicate the data object is feature-v2 and
> > > redefine the structure, or of course we revisit the entire
> > > featuring question within the structure of that data blob.
> > > 
> > > We also implement capability chains, though they're more meant for
> > > passing data to the user, where the user provides a buffer and we
> > > link capabilities together within that buffer for the user to
> > > walk.  We've defined a mechanism through -ENOSPC and argsz to
> > > tell the user how large a buffer is necessary.  I dare mention we
> > > have a version per capability as these are largely modeled after
> > > capability chains in PCI config space.  We haven't actually
> > > incremented any versions, but I imagine we'd do so like PCI,
> > > maintaining backwards compatibility and only defining unused bits
> > > and adding fields as the version increases. 
> > I guess capability chain is more suitable since the IOCTL uses
> > container FD instead of device FD in VFIO_DEVICE_FEATURE?
> > 
> > We can give that a try by treating IOMMU UAPIs as capabilities. That
> > would address problem #1. We really need to check compatibility
> > upfront in that there is no way to fail some of the UAPIs. e.g.
> > unbind guest PASID, cache invalidation.
> >   
> > > Is the objection to a global version or to any version fields?  I
> > > don't really understand the global version, I'd think a mechanism
> > > to check extensions plus a per structure flags/version would be
> > > preferred.  The former should resolve how userspace can test
> > > support for features requiring multiple interfaces.    
> > Currently we already have individual version & flags per UAPI data.
> > The reason why I introduced a global/unifier is to simplify the
> > compatibility checking. Global version is optional.
> > With individual version & flags, user may have to keep track of
> > combinations of per structure versions.  
> 
> A user would only need to meet the minimum uapi matching the feature
> they want to support, right?
> 
Yes, then no need for checking all combinations. I guess validation
cases have to cover all legal combinations, but not an issue.

> > >  A global version also
> > > implies that we're only ever adding features and never removing.
> > > For example, feature Foo is added in version 4, but it's replaced
> > > by feature Bar in version 5, now userspace can't simply test
> > > version >= 4 must include feature Foo.
> > >     
> > Yes, this is why I was hoping to stick with the rule: open for
> > extension, closed to modification.
> > It also makes the code backward compatible easy since there old code
> > would have no change when adding new features.  
> 
> But we need a way to deprecate things.  The version interface is not
> good for that.
> 
Agreed, flags are good for deprecation.

> > > It seems to me that version and flags can also be complimentary,
> > > for example a field might be defined by a version but a flag could
> > > indicate if it's implemented.  With only the flag, we'd infer the
> > > field from the flag, with only the version we'd need to assume the
> > > field is always implemented.  So I have a hard time making a
> > > blanket statement that all versions fields should be avoided.
> > >      
> > > > > 3. Maintain backward compatibility while allowing extensions?
> > > > > 
> > > > > I think we all agreed that using flags (capability or types)
> > > > > is the way to address #3. As Christoph pointed out, version
> > > > > number should not be used for this purpose.
> > > > > 
> > > > > So for problem 1 & 2, we have the following options:
> > > > > 1. Have a version-size mapping as proposed in this set. VFIO
> > > > > copies from user the correct size based on version-type
> > > > > lookup. Processing of the data is based on flags in IOMMU
> > > > > driver.
> > > > > 
> > > > > 2. VFIO copy its own minsz then pass the user pointer to IOMMU
> > > > > driver for further copy_from_user based on flags. (by Kevin)
> > > > > 
> > > > > 3. Adopt VFIO argsz scheme, caller fills in argsz for the
> > > > > offset the variable size union. VFIO do not check argsz in
> > > > > that it requires IOMMU specific knowledge. IOMMU driver Use
> > > > > flags to handle the variable size.(by Alex). I think this
> > > > > what we have in Yi's VFIO & QEMU patch. argsz filled by QEMU
> > > > > includes bind_data.
> > > > > 
> > > > > 4. Do not use a unified version, have a fixed size of all UAPI
> > > > > structures, padding in struct and union. (Wasteful, not
> > > > > preferred per V1 discussion)
> > > > > 
> > > > > For both 2 & 3, a unified version is not used, each API
> > > > > treated separately. vIOMMU will be launched w/o assurance of
> > > > > compatibility of all APIs. Fault handling may be more complex
> > > > > in normal operations.
> > > > > 
> > > > > Appreciate everyone's input. Joerg and Alex, could you help to
> > > > > make a decision here?      
> > > 
> > > As above, I think using a global API version number to imply
> > > support for a feature is doomed to fail, we should instead expose
> > > an interface to check for specific features.    
> > I agree. I feel we can use the capability chain at container level
> > as you mentioned, right?  
> 
> We could either do a capability chain on the IOMMU_GET_INFO ioctl or
> we have the CHECK_EXTENSION ioctl.  The latter is quite a bit easier
> to implement, but limited in what it can expose and the semantics
> around checking extensions before vs after an IOMMU model is set can
> be confusing.  I didn't have a strong preference between them when
> suggesting Kirti use one of these to indicate dirty page tracking, but
> it would be nice if both features used the same mechanism
> 
Sounds good. Just to clarify, similar to Yi's patch that uses
CHECK_EXTENSION
https://lkml.org/lkml/2020/3/22/109

But instead of checking for the global UAPI version, we check for each
feature. i.e.
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index b959d0a37c4f..639e3c970b31 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -46,6 +46,9 @@
  * code will taint the host kernel and should be used with extrem
  */
 #define VFIO_NOIOMMU_IOMMU             8
+#define VFIO_IOMMU_BIND_GPASID         9
+#define VFIO_IOMMU_INVALIDATE          10
+#define VFIO_IOMMU_PAGE_REQUEST        11

Applications such as QEMU would include uapi/iommu.h, which has the
IOMMU UAPI extensions defined.
e.g.
#define IOMMU_GPASID_BIND_VERSION_1

When VM starts, it will check all uAPI extensions returned by the
kernel against what is in its header, e.g.

extension = ioctl(s->container, VFIO_CHECK_EXTENSION,
VFIO_IOMMU_BIND_GPASID);
if (extension >= IOMMU_GPASID_BIND_VERSION_1) {
	/* Kernel has newer extensions, we are good */
} else {
	stop_viommu();
}

> > >  In any of the proposed solutions, the
> > > IOMMU driver is ultimately responsible for validating the user
> > > data, so do we want vfio performing the copy_from_user() to an
> > > object that could later be assumed to be sanitized, or should
> > > vfio just pass a user pointer to make it obvious that the
> > > consumer is responsible for all the user protections?  Seems like
> > > the latter.    
> > I like the latter as well.
> >   
> > >  That still really
> > > doesn't address what's in that user data blob yet, but the vfio
> > > interface could be:
> > > 
> > > struct {
> > > 	__u32 argsz;
> > > 	__u32 flags;
> > > 	__u8  data[];
> > > }
> > > 
> > > Where flags might be partitioned like we do for DEVICE_FEATURE to
> > > indicate the format of data and what vfio should do with it, and
> > > data might simply be defined as a (__u64 __user *).
> > >     
> > So, __user * will be passed to IOMMU driver if VFIO checks minsz
> > include flags and they are valid.
> > IOMMU driver can copy the rest based on the mandatory version/minsz
> > and flags in the IOMMU uAPI structs.
> > Does it sound right? This is really choice #2.  
> 
> Sounds like each IOMMU UAPI struct just needs to have an embedded size
> and flags field, but yes.
> 
Yes, an argsz field can be added to each UAPI. There are already flags
or the equivalent. IOMMU driver can process the __user * based on the
argsz, flags, check argsz against offsetofend(iommu_uapi_struct,
last_element), etc.;

> > > This user pointer would then likely be an IOMMU UAPI struct, so
> > > I've only just gotten back the the IOMMU UAPI question at hand,
> > > but I don't really see the disadvantage to including both version
> > > and flags fields per structure.  Perhaps this is choice 1. above,
> > > but with a version at a per structure level indicating the
> > > backwards compatible size and layout of the structure and flags
> > > being used to indicate support for optional features within those
> > > fields.    
> > 
> > Per structure version & flags is what we have in the mainline. It
> > applies to both choice 1 & 2. The global/unified version is just a
> > re-interpretation of the versions such that we have a monolithic
> > incrementing version. Again, global version is optional.
> >   
> > >  Is a version field still taboo
> > > for such a use case?  Thanks,    
> > 
> > I will leave that to Christoph :)  
> 
> Looks like if we drop the global version in favor of an
> extension/feature check and embed size and flags fields in each IOMMU
> UAPI struct, Christoph will be satisfied.  Thanks,
> 
Agreed, we will work towards it. Thanks a lot!

> Alex
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

On Tue, 14 Apr 2020 10:13:04 -0700
Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:

> > > >  In any of the proposed solutions, the
> > > > IOMMU driver is ultimately responsible for validating the user
> > > > data, so do we want vfio performing the copy_from_user() to an
> > > > object that could later be assumed to be sanitized, or should
> > > > vfio just pass a user pointer to make it obvious that the
> > > > consumer is responsible for all the user protections?  Seems
> > > > like the latter.      
> > > I like the latter as well.
> > >     
On a second thought, I think the former is better. Two reasons:

1. IOMMU API such as page_response is also used in baremetal. So it is
not suitable to pass a __user *.
https://www.spinics.net/lists/arm-kernel/msg798677.html

2. Some data are in the mandatory (fixed offset, never removed or
extended) portion of the uAPI structure. It is simpler for VFIO to
extract that and pass it to IOMMU API. For example, the PASID value used
for unbind_gpasid(). VFIO also need to sanitize the PASID value to make
sure it belongs to the same VM that did the allocation.


> > > >  That still really
> > > > doesn't address what's in that user data blob yet, but the vfio
> > > > interface could be:
> > > > 
> > > > struct {
> > > > 	__u32 argsz;
> > > > 	__u32 flags;
> > > > 	__u8  data[];
> > > > }
> > > > 
> > > > Where flags might be partitioned like we do for DEVICE_FEATURE
> > > > to indicate the format of data and what vfio should do with it,
> > > > and data might simply be defined as a (__u64 __user *).
> > > >       
> > > So, __user * will be passed to IOMMU driver if VFIO checks minsz
> > > include flags and they are valid.
> > > IOMMU driver can copy the rest based on the mandatory
> > > version/minsz and flags in the IOMMU uAPI structs.
> > > Does it sound right? This is really choice #2.    
> > 
> > Sounds like each IOMMU UAPI struct just needs to have an embedded
> > size and flags field, but yes.
> >   
> Yes, an argsz field can be added to each UAPI. There are already flags
> or the equivalent. IOMMU driver can process the __user * based on the
> argsz, flags, check argsz against offsetofend(iommu_uapi_struct,
> last_element), etc.;
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Wednesday, April 15, 2020 6:32 AM
> 
> On Tue, 14 Apr 2020 10:13:04 -0700
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > > > >  In any of the proposed solutions, the
> > > > > IOMMU driver is ultimately responsible for validating the user
> > > > > data, so do we want vfio performing the copy_from_user() to an
> > > > > object that could later be assumed to be sanitized, or should
> > > > > vfio just pass a user pointer to make it obvious that the
> > > > > consumer is responsible for all the user protections?  Seems
> > > > > like the latter.
> > > > I like the latter as well.
> > > >
> On a second thought, I think the former is better. Two reasons:
> 
> 1. IOMMU API such as page_response is also used in baremetal. So it is
> not suitable to pass a __user *.
> https://www.spinics.net/lists/arm-kernel/msg798677.html

You can have a wrapped version accepting a __user* and an internal
version for kernel pointers.

> 
> 2. Some data are in the mandatory (fixed offset, never removed or
> extended) portion of the uAPI structure. It is simpler for VFIO to
> extract that and pass it to IOMMU API. For example, the PASID value used
> for unbind_gpasid(). VFIO also need to sanitize the PASID value to make
> sure it belongs to the same VM that did the allocation.

I don't think this makes much difference. If anyway you still plan to
let IOMMU driver parse some user pointers, why not making a clear
split to have it sparse all IOMMU specific fields?

Thanks
Kevin

> 
> 
> > > > >  That still really
> > > > > doesn't address what's in that user data blob yet, but the vfio
> > > > > interface could be:
> > > > >
> > > > > struct {
> > > > > 	__u32 argsz;
> > > > > 	__u32 flags;
> > > > > 	__u8  data[];
> > > > > }
> > > > >
> > > > > Where flags might be partitioned like we do for DEVICE_FEATURE
> > > > > to indicate the format of data and what vfio should do with it,
> > > > > and data might simply be defined as a (__u64 __user *).
> > > > >
> > > > So, __user * will be passed to IOMMU driver if VFIO checks minsz
> > > > include flags and they are valid.
> > > > IOMMU driver can copy the rest based on the mandatory
> > > > version/minsz and flags in the IOMMU uAPI structs.
> > > > Does it sound right? This is really choice #2.
> > >
> > > Sounds like each IOMMU UAPI struct just needs to have an embedded
> > > size and flags field, but yes.
> > >
> > Yes, an argsz field can be added to each UAPI. There are already flags
> > or the equivalent. IOMMU driver can process the __user * based on the
> > argsz, flags, check argsz against offsetofend(iommu_uapi_struct,
> > last_element), etc.;
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Jacob Pan]

On Tue, 14 Apr 2020 23:47:40 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Wednesday, April 15, 2020 6:32 AM
> > 
> > On Tue, 14 Apr 2020 10:13:04 -0700
> > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> >   
> > > > > >  In any of the proposed solutions, the
> > > > > > IOMMU driver is ultimately responsible for validating the
> > > > > > user data, so do we want vfio performing the
> > > > > > copy_from_user() to an object that could later be assumed
> > > > > > to be sanitized, or should vfio just pass a user pointer to
> > > > > > make it obvious that the consumer is responsible for all
> > > > > > the user protections?  Seems like the latter.  
> > > > > I like the latter as well.
> > > > >  
> > On a second thought, I think the former is better. Two reasons:
> > 
> > 1. IOMMU API such as page_response is also used in baremetal. So it
> > is not suitable to pass a __user *.
> > https://www.spinics.net/lists/arm-kernel/msg798677.html  
> 
> You can have a wrapped version accepting a __user* and an internal
> version for kernel pointers.
> 
I have thought about that also but the problem is that some of the
flags are processed in the vendor IOMMU ops so it is hard to do that in
a generic wrapper.

> > 
> > 2. Some data are in the mandatory (fixed offset, never removed or
> > extended) portion of the uAPI structure. It is simpler for VFIO to
> > extract that and pass it to IOMMU API. For example, the PASID value
> > used for unbind_gpasid(). VFIO also need to sanitize the PASID
> > value to make sure it belongs to the same VM that did the
> > allocation.  
> 
> I don't think this makes much difference. If anyway you still plan to
> let IOMMU driver parse some user pointers, why not making a clear
> split to have it sparse all IOMMU specific fields?
>
The plan is not to have IOMMU driver parse user pointers. This is the
"former" case in Alex's comment. I.e. vfio performing the
copy_from_user based on argsz in IOMMU uAPI.
 
> Thanks
> Kevin
> 
> > 
> >   
> > > > > >  That still really
> > > > > > doesn't address what's in that user data blob yet, but the
> > > > > > vfio interface could be:
> > > > > >
> > > > > > struct {
> > > > > > 	__u32 argsz;
> > > > > > 	__u32 flags;
> > > > > > 	__u8  data[];
> > > > > > }
> > > > > >
> > > > > > Where flags might be partitioned like we do for
> > > > > > DEVICE_FEATURE to indicate the format of data and what vfio
> > > > > > should do with it, and data might simply be defined as a
> > > > > > (__u64 __user *). 
> > > > > So, __user * will be passed to IOMMU driver if VFIO checks
> > > > > minsz include flags and they are valid.
> > > > > IOMMU driver can copy the rest based on the mandatory
> > > > > version/minsz and flags in the IOMMU uAPI structs.
> > > > > Does it sound right? This is really choice #2.  
> > > >
> > > > Sounds like each IOMMU UAPI struct just needs to have an
> > > > embedded size and flags field, but yes.
> > > >  
> > > Yes, an argsz field can be added to each UAPI. There are already
> > > flags or the equivalent. IOMMU driver can process the __user *
> > > based on the argsz, flags, check argsz against
> > > offsetofend(iommu_uapi_struct, last_element), etc.;  

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Wednesday, April 15, 2020 11:39 PM
> 
> On Tue, 14 Apr 2020 23:47:40 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
> > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > Sent: Wednesday, April 15, 2020 6:32 AM
> > >
> > > On Tue, 14 Apr 2020 10:13:04 -0700
> > > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> > >
> > > > > > >  In any of the proposed solutions, the
> > > > > > > IOMMU driver is ultimately responsible for validating the
> > > > > > > user data, so do we want vfio performing the
> > > > > > > copy_from_user() to an object that could later be assumed
> > > > > > > to be sanitized, or should vfio just pass a user pointer to
> > > > > > > make it obvious that the consumer is responsible for all
> > > > > > > the user protections?  Seems like the latter.
> > > > > > I like the latter as well.
> > > > > >
> > > On a second thought, I think the former is better. Two reasons:
> > >
> > > 1. IOMMU API such as page_response is also used in baremetal. So it
> > > is not suitable to pass a __user *.
> > > https://www.spinics.net/lists/arm-kernel/msg798677.html
> >
> > You can have a wrapped version accepting a __user* and an internal
> > version for kernel pointers.
> >
> I have thought about that also but the problem is that some of the
> flags are processed in the vendor IOMMU ops so it is hard to do that in
> a generic wrapper.

All vendor IOMMU ops are defined in the same header file, so they
can be verified in one common IOMMU wrapper, just like how you
dealt with it in VFIO originally...

> 
> > >
> > > 2. Some data are in the mandatory (fixed offset, never removed or
> > > extended) portion of the uAPI structure. It is simpler for VFIO to
> > > extract that and pass it to IOMMU API. For example, the PASID value
> > > used for unbind_gpasid(). VFIO also need to sanitize the PASID
> > > value to make sure it belongs to the same VM that did the
> > > allocation.
> >
> > I don't think this makes much difference. If anyway you still plan to
> > let IOMMU driver parse some user pointers, why not making a clear
> > split to have it sparse all IOMMU specific fields?
> >
> The plan is not to have IOMMU driver parse user pointers. This is the
> "former" case in Alex's comment. I.e. vfio performing the
> copy_from_user based on argsz in IOMMU uAPI.
> 

I'm confused. I thought Alex proposed the latter one:
---[quote]
> So, __user * will be passed to IOMMU driver if VFIO checks minsz
> include flags and they are valid.
> IOMMU driver can copy the rest based on the mandatory version/minsz and
> flags in the IOMMU uAPI structs.
> Does it sound right? This is really choice #2.

Sounds like each IOMMU UAPI struct just needs to have an embedded size
and flags field, but yes.
----

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu