[PATCH 0/3] IOMMU user API enhancement

[PATCH 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 extension 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 different 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.

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       | 25 +++++++++++++++++++-
 include/linux/iommu.h       |  6 +++++
 include/uapi/linux/iommu.h  | 57 ++++++++++++++++++++++++++++++++++++++++-----
 5 files changed, 84 insertions(+), 9 deletions(-)

-- 
2.7.4

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

[Jacob Pan]

Define a unified UAPI version to be used for compatibility
checks between user and kernel.

Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 include/uapi/linux/iommu.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index fcafb6401430..65a26c2519ee 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -8,6 +8,54 @@
 
 #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.
+ *
+ * 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.
+ *
+ * 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 to any structure
+ *    results in version bump up.
+ *
+ * 2. Data structures are open to extension but closed to modification.
+ *    New fields must be added at the end of each data structure with
+ *    64bit alignment. 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

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

[Auger Eric]

Hi Jacob,
On 1/29/20 7:02 AM, Jacob Pan wrote:
> Define a unified UAPI version to be used for compatibility
> checks between user and kernel.
> 
> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>  include/uapi/linux/iommu.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index fcafb6401430..65a26c2519ee 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -8,6 +8,54 @@
>  
>  #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.
> + *
> + * 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.
I would rather put the above justification in the commit msg and not here.
> + *
> + * 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 to any structure
s/to to/to
> + *    results in version bump up.
in a version number increment?
> + *
> + * 2. Data structures are open to extension but closed to modification.> + *    New fields must be added at the end of each data structure with
> + *    64bit alignment. 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 */
> 
Thanks

Eric

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

[Jacob Pan]

On Thu, 6 Feb 2020 11:14:27 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Jacob,
> On 1/29/20 7:02 AM, Jacob Pan wrote:
> > Define a unified UAPI version to be used for compatibility
> > checks between user and kernel.
> > 
> > Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >  include/uapi/linux/iommu.h | 48
> > ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48
> > insertions(+)
> > 
> > diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> > index fcafb6401430..65a26c2519ee 100644
> > --- a/include/uapi/linux/iommu.h
> > +++ b/include/uapi/linux/iommu.h
> > @@ -8,6 +8,54 @@
> >  
> >  #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.
> > + *
> > + * 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.  
> I would rather put the above justification in the commit msg and not
> here.
make sense.

> > + *
> > + * 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 to any
> > structure  
> s/to to/to
will fix.

> > + *    results in version bump up.  
> in a version number increment?
sounds good, more specific.

> > + *
> > + * 2. Data structures are open to extension but closed to
> > modification.> + *    New fields must be added at the end of each
> > data structure with
> > + *    64bit alignment. 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 */
> >   
> Thanks
> 
> Eric
> 

[PATCH 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 b3778e86dc32..b17b338ada34 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5763,8 +5763,9 @@ static int intel_iommu_sva_invalidate(struct iommu_domain *domain,
 	int ret = 0;
 	u64 size;
 
+	/* 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 7a87d2e2e0ad..c756b43e959c 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 fdd40756dbc1..7dd51c5d2ba1 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1094,7 +1094,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 65a26c2519ee..5e410761dfa3 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -183,7 +183,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
@@ -191,7 +191,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;
@@ -266,7 +265,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
@@ -294,7 +293,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 */
@@ -338,7 +336,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
@@ -355,7 +353,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

[PATCH 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.

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 | 22 ++++++++++++++++++++++
 include/linux/iommu.h |  6 ++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7dd51c5d2ba1..9e5de9abebdf 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1696,6 +1696,28 @@ 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, the offsetofend()
+ * will identify the new sizes.
+ */
+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)
+{
+	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 9718c109ea0a..416fe02160ba 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -500,6 +500,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);
@@ -885,6 +886,11 @@ static inline int iommu_page_response(struct device *dev,
 	return -ENODEV;
 }
 
+static 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

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

[Alex Williamson]

On Tue, 28 Jan 2020 22:02:04 -0800
Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:

> 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.
> 
> 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 | 22 ++++++++++++++++++++++
>  include/linux/iommu.h |  6 ++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 7dd51c5d2ba1..9e5de9abebdf 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1696,6 +1696,28 @@ 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, the offsetofend()
> + * will identify the new sizes.
> + */
> +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)
> +{

Seems like this is asking for a bounds check,

  if (type >= NR_IOMMU_UAPI_TYPE || version > IOMMU_UAPI_VERSION)
  	return -EINVAL;

If we add new types in future versions, I assume we'd back fill the
table with -EINVAL as well (rather than zero).  Thanks,

Alex

> +	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 9718c109ea0a..416fe02160ba 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -500,6 +500,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);
> @@ -885,6 +886,11 @@ static inline int iommu_page_response(struct device *dev,
>  	return -ENODEV;
>  }
>  
> +static int iommu_uapi_get_data_size(int type, int version)
> +{
> +	return -ENODEV;
> +}
> +
>  static inline int iommu_group_id(struct iommu_group *group)
>  {
>  	return -ENODEV;

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

[Alex Williamson]

On Wed, 29 Jan 2020 14:40:46 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Tue, 28 Jan 2020 22:02:04 -0800
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > 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.
> > 
> > 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 | 22 ++++++++++++++++++++++
> >  include/linux/iommu.h |  6 ++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 7dd51c5d2ba1..9e5de9abebdf 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1696,6 +1696,28 @@ 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, the offsetofend()
> > + * will identify the new sizes.
> > + */
> > +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)},

This seems prone to errors in future revisions.  Both of the above
reference the end of fields within an anonymous union.  When a new
field is added, it's not necessarily the newest field that needs to be
listed here, but the largest at the time.  So should the current
version always use sizeof instead (or name the union so we can
reference it)?  I'm not sure of an error proof way to make sure we keep
the N-1 version consistent when we add a new version though.  More
comments?

Also, is the 12-bytes of padding in struct iommu_gpasid_bind_data
excessive with this new versioning scheme?  Per rule #2 I'm not sure if
we're allowed to repurpose those padding bytes, but if we add fields to
the end of the structure as the scheme suggests, we're stuck with not
being able to expand the union for new fields.

Thanks,
Alex

> > +	/* IOMMU_UAPI_PAGE_RESP */
> > +	{offsetofend(struct iommu_page_response, code)},
> > +};
> > +
> > +int iommu_uapi_get_data_size(int type, int version)
> > +{  
> 
> Seems like this is asking for a bounds check,
> 
>   if (type >= NR_IOMMU_UAPI_TYPE || version > IOMMU_UAPI_VERSION)
>   	return -EINVAL;
> 
> If we add new types in future versions, I assume we'd back fill the
> table with -EINVAL as well (rather than zero).  Thanks,
> 
> Alex
> 
> > +	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 9718c109ea0a..416fe02160ba 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -500,6 +500,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);
> > @@ -885,6 +886,11 @@ static inline int iommu_page_response(struct device *dev,
> >  	return -ENODEV;
> >  }
> >  
> > +static int iommu_uapi_get_data_size(int type, int version)
> > +{
> > +	return -ENODEV;
> > +}
> > +
> >  static inline int iommu_group_id(struct iommu_group *group)
> >  {
> >  	return -ENODEV;  
> 

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

[Jacob Pan]

On Wed, 29 Jan 2020 15:19:51 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Wed, 29 Jan 2020 14:40:46 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > On Tue, 28 Jan 2020 22:02:04 -0800
> > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> >   
> > > 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.
> > > 
> > > 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 | 22 ++++++++++++++++++++++
> > >  include/linux/iommu.h |  6 ++++++
> > >  2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 7dd51c5d2ba1..9e5de9abebdf 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -1696,6 +1696,28 @@ 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, the
> > > offsetofend()
> > > + * will identify the new sizes.
> > > + */
> > > +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)},  
> 
> This seems prone to errors in future revisions.  Both of the above
> reference the end of fields within an anonymous union.  When a new
> field is added, it's not necessarily the newest field that needs to be
> listed here, but the largest at the time.
> So should the current
> version always use sizeof instead (or name the union so we can
> reference it)?  I'm not sure of an error proof way to make sure we
> keep the N-1 version consistent when we add a new version though.
> More comments?
> 
yes. we must be very careful the new size has to be the largest of the
union not the newest. I agree that using sizeof() would make current
version size straightforward. But we have to find the size for N-1
with offsetofend, seems more risk to the existing N-1 code when bump up
version.

I was thinking this array also serve as documentation of the revision
history, but as you pointed out offsetofend may not be the newest
member of the union. So it cannot document which member was added in
which version. Seems more comments is the only way.

How about comments as below with example future extensions?

/**
 * 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.
 *
 * Current version: V1
 * +--------------+---------------+
 * | Type /       |       V1      |
 * | UAPI Version |               |
 * +==============+===============+
 * | BIND_GPASID  |       vtd     |
 * +--------------+---------------+
 * | CACHE_INVAL  |  addr_info    |
 * +--------------+---------------+
 * | PAGE_RESP    |  code         |
 * +--------------+---------------+
 *
 * Future extension examples:
 *
 * V2 adds new members to bind_gpasid and cache_invalidate API but not
 * page response.
 * +--------------+---------------+---------------+
 * | 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|
 * +--------------+---------------+---------------+---------------+
 *

> Also, is the 12-bytes of padding in struct iommu_gpasid_bind_data
> excessive with this new versioning scheme?  Per rule #2 I'm not sure
> if we're allowed to repurpose those padding bytes, but if we add
> fields to the end of the structure as the scheme suggests, we're
> stuck with not being able to expand the union for new fields.
> 
> Thanks,
> Alex
> 
> > > +	/* IOMMU_UAPI_PAGE_RESP */
> > > +	{offsetofend(struct iommu_page_response, code)},
> > > +};
> > > +
> > > +int iommu_uapi_get_data_size(int type, int version)
> > > +{    
> > 
> > Seems like this is asking for a bounds check,
> > 
> >   if (type >= NR_IOMMU_UAPI_TYPE || version > IOMMU_UAPI_VERSION)
> >   	return -EINVAL;
> > 
> > If we add new types in future versions, I assume we'd back fill the
> > table with -EINVAL as well (rather than zero).  Thanks,
> > 
> > Alex
> >   
> > > +	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 9718c109ea0a..416fe02160ba 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -500,6 +500,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); @@ -885,6 +886,11 @@ static inline int
> > > iommu_page_response(struct device *dev, return -ENODEV;
> > >  }
> > >  
> > > +static int iommu_uapi_get_data_size(int type, int version)
> > > +{
> > > +	return -ENODEV;
> > > +}
> > > +
> > >  static inline int iommu_group_id(struct iommu_group *group)
> > >  {
> > >  	return -ENODEV;    
> >   
> 

[Jacob Pan]

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

[Jacob Pan]

Hi Alex,
Sorry I missed this part in the previous reply. Comments below.

On Wed, 29 Jan 2020 15:19:51 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> Also, is the 12-bytes of padding in struct iommu_gpasid_bind_data
> excessive with this new versioning scheme?  Per rule #2 I'm not sure
> if we're allowed to repurpose those padding bytes,
We can still use the padding bytes as long as there is a new flag bit
to indicate the validity of the new filed within the padding.
I should have made it clear in rule #2 when mentioning the flags bits.
Should define what extension constitutes.
How about this?
"
 * 2. Data structures are open to extension but closed to modification.
 *    Extension should leverage the padding bytes first where a new
 *    flag bit is required to indicate the validity of each new member.
 *    The above rule for padding bytes also applies to adding new union
 *    members.
 *    After padding bytes are exhausted, new fields must be added at the
 *    end of each data structure with 64bit alignment. Flag bits can be
 *    added without size change but existing ones cannot be altered.
 *
"
So if we add new field by doing re-purpose of padding bytes, size
lookup result will remain the same. New code would recognize the new
flag, old code stays the same.

VFIO layer checks for UAPI compatibility and size to copy, version
sanity check and flag usage are done in the IOMMU code.

> but if we add
> fields to the end of the structure as the scheme suggests, we're
> stuck with not being able to expand the union for new fields.
Good point, it does sound contradictory. I hope the rewritten rule #2
address that.
Adding data after the union should be extremely rare. Do you see any
issues with the example below?
 
 offsetofend() can still find the right size.
e.g.
V1
struct iommu_gpasid_bind_data {
	__u32 version;
#define IOMMU_PASID_FORMAT_INTEL_VTD	1
	__u32 format;
#define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
	__u64 flags;
	__u64 gpgd;
	__u64 hpasid;
	__u64 gpasid;
	__u32 addr_width;
	__u8  padding[12];
	/* Vendor specific data */
	union {
		struct iommu_gpasid_bind_data_vtd vtd;
	};
};

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)}, ...
};

V2, Add new_member at the end (forget padding for now).
struct iommu_gpasid_bind_data {
	__u32 version;
#define IOMMU_PASID_FORMAT_INTEL_VTD	1
	__u32 format;
#define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
#define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new member added */
	__u64 flags;
	__u64 gpgd;
	__u64 hpasid;
	__u64 gpasid;
	__u32 addr_width;
	__u8  padding[12];
	/* Vendor specific data */
	union {
		struct iommu_gpasid_bind_data_vtd vtd;
	};
	__u64 new_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), offsetofend(struct iommu_gpasid_bind_data,new_member)},

};

V3, Add smmu to the union,larger than vtd

struct iommu_gpasid_bind_data {
	__u32 version;
#define IOMMU_PASID_FORMAT_INTEL_VTD	1
#define IOMMU_PASID_FORMAT_INTEL_SMMU	2
	__u32 format;
#define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
#define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new member added */
#define IOMMU_SVA_SMMU_SUPP	(1 << 2) /* SMMU data supported */
	__u64 flags;
	__u64 gpgd;
	__u64 hpasid;
	__u64 gpasid;
	__u32 addr_width;
	__u8  padding[12];
	/* Vendor specific data */
	union {
		struct iommu_gpasid_bind_data_vtd vtd;
		struct iommu_gpasid_bind_data_smmu smmu;
	};
	__u64 new_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),
	offsetofend(struct iommu_gpasid_bind_data, new_member),
	offsetofend(struct iommu_gpasid_bind_data, new_member)},
...
};

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

[Alex Williamson]

On Fri, 31 Jan 2020 15:51:25 -0800
Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:

> Hi Alex,
> Sorry I missed this part in the previous reply. Comments below.
> 
> On Wed, 29 Jan 2020 15:19:51 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > Also, is the 12-bytes of padding in struct iommu_gpasid_bind_data
> > excessive with this new versioning scheme?  Per rule #2 I'm not sure
> > if we're allowed to repurpose those padding bytes,  
> We can still use the padding bytes as long as there is a new flag bit
> to indicate the validity of the new filed within the padding.
> I should have made it clear in rule #2 when mentioning the flags bits.
> Should define what extension constitutes.
> How about this?
> "
>  * 2. Data structures are open to extension but closed to modification.
>  *    Extension should leverage the padding bytes first where a new
>  *    flag bit is required to indicate the validity of each new member.
>  *    The above rule for padding bytes also applies to adding new union
>  *    members.
>  *    After padding bytes are exhausted, new fields must be added at the
>  *    end of each data structure with 64bit alignment. Flag bits can be
>  *    added without size change but existing ones cannot be altered.
>  *
> "
> So if we add new field by doing re-purpose of padding bytes, size
> lookup result will remain the same. New code would recognize the new
> flag, old code stays the same.
> 
> VFIO layer checks for UAPI compatibility and size to copy, version
> sanity check and flag usage are done in the IOMMU code.
> 
> > but if we add
> > fields to the end of the structure as the scheme suggests, we're
> > stuck with not being able to expand the union for new fields.  
> Good point, it does sound contradictory. I hope the rewritten rule #2
> address that.
> Adding data after the union should be extremely rare. Do you see any
> issues with the example below?
>  
>  offsetofend() can still find the right size.
> e.g.
> V1
> struct iommu_gpasid_bind_data {
> 	__u32 version;
> #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> 	__u32 format;
> #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> 	__u64 flags;
> 	__u64 gpgd;
> 	__u64 hpasid;
> 	__u64 gpasid;
> 	__u32 addr_width;
> 	__u8  padding[12];
> 	/* Vendor specific data */
> 	union {
> 		struct iommu_gpasid_bind_data_vtd vtd;
> 	};
> };
> 
> 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)}, ...
> };
> 
> V2, Add new_member at the end (forget padding for now).
> struct iommu_gpasid_bind_data {
> 	__u32 version;
> #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> 	__u32 format;
> #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new member added */
> 	__u64 flags;
> 	__u64 gpgd;
> 	__u64 hpasid;
> 	__u64 gpasid;
> 	__u32 addr_width;
> 	__u8  padding[12];
> 	/* Vendor specific data */
> 	union {
> 		struct iommu_gpasid_bind_data_vtd vtd;
> 	};
> 	__u64 new_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), offsetofend(struct iommu_gpasid_bind_data,new_member)},
> 
> };
> 
> V3, Add smmu to the union,larger than vtd
> 
> struct iommu_gpasid_bind_data {
> 	__u32 version;
> #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> #define IOMMU_PASID_FORMAT_INTEL_SMMU	2
> 	__u32 format;
> #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new member added */
> #define IOMMU_SVA_SMMU_SUPP	(1 << 2) /* SMMU data supported */
> 	__u64 flags;
> 	__u64 gpgd;
> 	__u64 hpasid;
> 	__u64 gpasid;
> 	__u32 addr_width;
> 	__u8  padding[12];
> 	/* Vendor specific data */
> 	union {
> 		struct iommu_gpasid_bind_data_vtd vtd;
> 		struct iommu_gpasid_bind_data_smmu smmu;
> 	};
> 	__u64 new_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),
> 	offsetofend(struct iommu_gpasid_bind_data, new_member),
> 	offsetofend(struct iommu_gpasid_bind_data, new_member)},
> ...
> };
> 

How are you not breaking rule #3, "Versions are backward compatible"
with this?  If the kernel is at version 3 and userspace is at version 2
then new_member exists at different offsets of the structure.  The
kernels iommu_uapi_data_size for V2 changed between version 2 and 3.
Thanks,

Alex

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

[Jacob Pan]

On Mon, 3 Feb 2020 11:27:08 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Fri, 31 Jan 2020 15:51:25 -0800
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > Hi Alex,
> > Sorry I missed this part in the previous reply. Comments below.
> > 
> > On Wed, 29 Jan 2020 15:19:51 -0700
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >   
> > > Also, is the 12-bytes of padding in struct iommu_gpasid_bind_data
> > > excessive with this new versioning scheme?  Per rule #2 I'm not
> > > sure if we're allowed to repurpose those padding bytes,    
> > We can still use the padding bytes as long as there is a new flag
> > bit to indicate the validity of the new filed within the padding.
> > I should have made it clear in rule #2 when mentioning the flags
> > bits. Should define what extension constitutes.
> > How about this?
> > "
> >  * 2. Data structures are open to extension but closed to
> > modification.
> >  *    Extension should leverage the padding bytes first where a new
> >  *    flag bit is required to indicate the validity of each new
> > member.
> >  *    The above rule for padding bytes also applies to adding new
> > union
> >  *    members.
> >  *    After padding bytes are exhausted, new fields must be added
> > at the
> >  *    end of each data structure with 64bit alignment. Flag bits
> > can be
> >  *    added without size change but existing ones cannot be altered.
> >  *
> > "
> > So if we add new field by doing re-purpose of padding bytes, size
> > lookup result will remain the same. New code would recognize the new
> > flag, old code stays the same.
> > 
> > VFIO layer checks for UAPI compatibility and size to copy, version
> > sanity check and flag usage are done in the IOMMU code.
> >   
> > > but if we add
> > > fields to the end of the structure as the scheme suggests, we're
> > > stuck with not being able to expand the union for new fields.    
> > Good point, it does sound contradictory. I hope the rewritten rule
> > #2 address that.
> > Adding data after the union should be extremely rare. Do you see any
> > issues with the example below?
> >  
> >  offsetofend() can still find the right size.
> > e.g.
> > V1
> > struct iommu_gpasid_bind_data {
> > 	__u32 version;
> > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > 	__u32 format;
> > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> > 	__u64 flags;
> > 	__u64 gpgd;
> > 	__u64 hpasid;
> > 	__u64 gpasid;
> > 	__u32 addr_width;
> > 	__u8  padding[12];
> > 	/* Vendor specific data */
> > 	union {
> > 		struct iommu_gpasid_bind_data_vtd vtd;
> > 	};
> > };
> > 
> > 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)}, ...
> > };
> > 
> > V2, Add new_member at the end (forget padding for now).
> > struct iommu_gpasid_bind_data {
> > 	__u32 version;
> > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > 	__u32 format;
> > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> > #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new member added */
> > 	__u64 flags;
> > 	__u64 gpgd;
> > 	__u64 hpasid;
> > 	__u64 gpasid;
> > 	__u32 addr_width;
> > 	__u8  padding[12];
> > 	/* Vendor specific data */
> > 	union {
> > 		struct iommu_gpasid_bind_data_vtd vtd;
> > 	};
> > 	__u64 new_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), offsetofend(struct
> > iommu_gpasid_bind_data,new_member)},
> > 
> > };
> > 
> > V3, Add smmu to the union,larger than vtd
> > 
> > struct iommu_gpasid_bind_data {
> > 	__u32 version;
> > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > #define IOMMU_PASID_FORMAT_INTEL_SMMU	2
> > 	__u32 format;
> > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> > #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new member added */
> > #define IOMMU_SVA_SMMU_SUPP	(1 << 2) /* SMMU data supported
> > */ __u64 flags;
> > 	__u64 gpgd;
> > 	__u64 hpasid;
> > 	__u64 gpasid;
> > 	__u32 addr_width;
> > 	__u8  padding[12];
> > 	/* Vendor specific data */
> > 	union {
> > 		struct iommu_gpasid_bind_data_vtd vtd;
> > 		struct iommu_gpasid_bind_data_smmu smmu;
> > 	};
> > 	__u64 new_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),
> > 	offsetofend(struct iommu_gpasid_bind_data, new_member),
> > 	offsetofend(struct iommu_gpasid_bind_data, new_member)},
> > ...
> > };
> >   
> 
> How are you not breaking rule #3, "Versions are backward compatible"
> with this?  If the kernel is at version 3 and userspace is at version
> 2 then new_member exists at different offsets of the structure.  The
> kernels iommu_uapi_data_size for V2 changed between version 2 and 3.
> Thanks,
> 
You are right. if we want to add new member to the end of the structure
as well as expanding union, I think we have to fix the size of the
union. Would this work? (just an example for one struct)


@@ -344,6 +348,11 @@ struct iommu_gpasid_bind_data_vtd {
  * @gpasid:    Process address space ID used for the guest mm in guest
IOMMU
  * @addr_width:        Guest virtual address width
  * @padding:   Reserved for future use (should be zero)
+ * @dummy      Reserve space for vendor specific data in the union. New
+ *             members added to the union cannot exceed the size of
dummy.
+ *             The fixed size union is needed to allow further
expansion
+ *             after the end of the union while still maintain backward
+ *             compatibility.
  * @vtd:       Intel VT-d specific data
  *
  * Guest to host PASID mapping can be an identity or non-identity,
where guest @@ -365,6 +374,7 @@ struct iommu_gpasid_bind_data {
        __u8  padding[12];
        /* Vendor specific data */
        union {
+               __u8 dummy[128];
                struct iommu_gpasid_bind_data_vtd vtd;
        };
 };

> Alex
> 

[Jacob Pan]

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

[Alex Williamson]

On Mon, 3 Feb 2020 12:41:43 -0800
Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:

> On Mon, 3 Feb 2020 11:27:08 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > On Fri, 31 Jan 2020 15:51:25 -0800
> > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> >   
> > > Hi Alex,
> > > Sorry I missed this part in the previous reply. Comments below.
> > > 
> > > On Wed, 29 Jan 2020 15:19:51 -0700
> > > Alex Williamson <alex.williamson@redhat.com> wrote:
> > >     
> > > > Also, is the 12-bytes of padding in struct iommu_gpasid_bind_data
> > > > excessive with this new versioning scheme?  Per rule #2 I'm not
> > > > sure if we're allowed to repurpose those padding bytes,      
> > > We can still use the padding bytes as long as there is a new flag
> > > bit to indicate the validity of the new filed within the padding.
> > > I should have made it clear in rule #2 when mentioning the flags
> > > bits. Should define what extension constitutes.
> > > How about this?
> > > "
> > >  * 2. Data structures are open to extension but closed to
> > > modification.
> > >  *    Extension should leverage the padding bytes first where a new
> > >  *    flag bit is required to indicate the validity of each new
> > > member.
> > >  *    The above rule for padding bytes also applies to adding new
> > > union
> > >  *    members.
> > >  *    After padding bytes are exhausted, new fields must be added
> > > at the
> > >  *    end of each data structure with 64bit alignment. Flag bits
> > > can be
> > >  *    added without size change but existing ones cannot be altered.
> > >  *
> > > "
> > > So if we add new field by doing re-purpose of padding bytes, size
> > > lookup result will remain the same. New code would recognize the new
> > > flag, old code stays the same.
> > > 
> > > VFIO layer checks for UAPI compatibility and size to copy, version
> > > sanity check and flag usage are done in the IOMMU code.
> > >     
> > > > but if we add
> > > > fields to the end of the structure as the scheme suggests, we're
> > > > stuck with not being able to expand the union for new fields.      
> > > Good point, it does sound contradictory. I hope the rewritten rule
> > > #2 address that.
> > > Adding data after the union should be extremely rare. Do you see any
> > > issues with the example below?
> > >  
> > >  offsetofend() can still find the right size.
> > > e.g.
> > > V1
> > > struct iommu_gpasid_bind_data {
> > > 	__u32 version;
> > > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > > 	__u32 format;
> > > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> > > 	__u64 flags;
> > > 	__u64 gpgd;
> > > 	__u64 hpasid;
> > > 	__u64 gpasid;
> > > 	__u32 addr_width;
> > > 	__u8  padding[12];
> > > 	/* Vendor specific data */
> > > 	union {
> > > 		struct iommu_gpasid_bind_data_vtd vtd;
> > > 	};
> > > };
> > > 
> > > 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)}, ...
> > > };
> > > 
> > > V2, Add new_member at the end (forget padding for now).
> > > struct iommu_gpasid_bind_data {
> > > 	__u32 version;
> > > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > > 	__u32 format;
> > > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> > > #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new member added */
> > > 	__u64 flags;
> > > 	__u64 gpgd;
> > > 	__u64 hpasid;
> > > 	__u64 gpasid;
> > > 	__u32 addr_width;
> > > 	__u8  padding[12];
> > > 	/* Vendor specific data */
> > > 	union {
> > > 		struct iommu_gpasid_bind_data_vtd vtd;
> > > 	};
> > > 	__u64 new_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), offsetofend(struct
> > > iommu_gpasid_bind_data,new_member)},
> > > 
> > > };
> > > 
> > > V3, Add smmu to the union,larger than vtd
> > > 
> > > struct iommu_gpasid_bind_data {
> > > 	__u32 version;
> > > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > > #define IOMMU_PASID_FORMAT_INTEL_SMMU	2
> > > 	__u32 format;
> > > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID valid */
> > > #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new member added */
> > > #define IOMMU_SVA_SMMU_SUPP	(1 << 2) /* SMMU data supported
> > > */ __u64 flags;
> > > 	__u64 gpgd;
> > > 	__u64 hpasid;
> > > 	__u64 gpasid;
> > > 	__u32 addr_width;
> > > 	__u8  padding[12];
> > > 	/* Vendor specific data */
> > > 	union {
> > > 		struct iommu_gpasid_bind_data_vtd vtd;
> > > 		struct iommu_gpasid_bind_data_smmu smmu;
> > > 	};
> > > 	__u64 new_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),
> > > 	offsetofend(struct iommu_gpasid_bind_data, new_member),
> > > 	offsetofend(struct iommu_gpasid_bind_data, new_member)},
> > > ...
> > > };
> > >     
> > 
> > How are you not breaking rule #3, "Versions are backward compatible"
> > with this?  If the kernel is at version 3 and userspace is at version
> > 2 then new_member exists at different offsets of the structure.  The
> > kernels iommu_uapi_data_size for V2 changed between version 2 and 3.
> > Thanks,
> >   
> You are right. if we want to add new member to the end of the structure
> as well as expanding union, I think we have to fix the size of the
> union. Would this work? (just an example for one struct)
> 
> 
> @@ -344,6 +348,11 @@ struct iommu_gpasid_bind_data_vtd {
>   * @gpasid:    Process address space ID used for the guest mm in guest
> IOMMU
>   * @addr_width:        Guest virtual address width
>   * @padding:   Reserved for future use (should be zero)
> + * @dummy      Reserve space for vendor specific data in the union. New
> + *             members added to the union cannot exceed the size of
> dummy.
> + *             The fixed size union is needed to allow further
> expansion
> + *             after the end of the union while still maintain backward
> + *             compatibility.
>   * @vtd:       Intel VT-d specific data
>   *
>   * Guest to host PASID mapping can be an identity or non-identity,
> where guest @@ -365,6 +374,7 @@ struct iommu_gpasid_bind_data {
>         __u8  padding[12];
>         /* Vendor specific data */
>         union {
> +               __u8 dummy[128];
>                 struct iommu_gpasid_bind_data_vtd vtd;
>         };
>  };

It's not the most space efficient thing and we're just guessing at what
might need to be added into that union in future, but it works... until
it doesn't ;)  One might also argue that we could inflate the padding
field even further to serve the same purpose.  The only other route I
can think of would be to let the user specify the offset of the
variable size data from the start of the structure, for example similar
to how we're laying out vfio migration region or how we do capabilities
in vfio ioctls.  This is where passing an argsz for each ioctl comes in
handy so we're not limited to a structure, we can link various
structures together in a chain.  Of course that requires work on both
the user and kernel side to pack and unpack, but it leaves a lot of
flexibility in extending it.  Thanks,

Alex

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

[Jacob Pan]

On Mon, 3 Feb 2020 14:12:36 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon, 3 Feb 2020 12:41:43 -0800
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > On Mon, 3 Feb 2020 11:27:08 -0700
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >   
> > > On Fri, 31 Jan 2020 15:51:25 -0800
> > > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> > >     
> > > > Hi Alex,
> > > > Sorry I missed this part in the previous reply. Comments below.
> > > > 
> > > > On Wed, 29 Jan 2020 15:19:51 -0700
> > > > Alex Williamson <alex.williamson@redhat.com> wrote:
> > > >       
> > > > > Also, is the 12-bytes of padding in struct
> > > > > iommu_gpasid_bind_data excessive with this new versioning
> > > > > scheme?  Per rule #2 I'm not sure if we're allowed to
> > > > > repurpose those padding bytes,        
> > > > We can still use the padding bytes as long as there is a new
> > > > flag bit to indicate the validity of the new filed within the
> > > > padding. I should have made it clear in rule #2 when mentioning
> > > > the flags bits. Should define what extension constitutes.
> > > > How about this?
> > > > "
> > > >  * 2. Data structures are open to extension but closed to
> > > > modification.
> > > >  *    Extension should leverage the padding bytes first where a
> > > > new
> > > >  *    flag bit is required to indicate the validity of each new
> > > > member.
> > > >  *    The above rule for padding bytes also applies to adding
> > > > new union
> > > >  *    members.
> > > >  *    After padding bytes are exhausted, new fields must be
> > > > added at the
> > > >  *    end of each data structure with 64bit alignment. Flag bits
> > > > can be
> > > >  *    added without size change but existing ones cannot be
> > > > altered. *
> > > > "
> > > > So if we add new field by doing re-purpose of padding bytes,
> > > > size lookup result will remain the same. New code would
> > > > recognize the new flag, old code stays the same.
> > > > 
> > > > VFIO layer checks for UAPI compatibility and size to copy,
> > > > version sanity check and flag usage are done in the IOMMU code.
> > > >       
> > > > > but if we add
> > > > > fields to the end of the structure as the scheme suggests,
> > > > > we're stuck with not being able to expand the union for new
> > > > > fields.        
> > > > Good point, it does sound contradictory. I hope the rewritten
> > > > rule #2 address that.
> > > > Adding data after the union should be extremely rare. Do you
> > > > see any issues with the example below?
> > > >  
> > > >  offsetofend() can still find the right size.
> > > > e.g.
> > > > V1
> > > > struct iommu_gpasid_bind_data {
> > > > 	__u32 version;
> > > > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > > > 	__u32 format;
> > > > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID
> > > > valid */ __u64 flags;
> > > > 	__u64 gpgd;
> > > > 	__u64 hpasid;
> > > > 	__u64 gpasid;
> > > > 	__u32 addr_width;
> > > > 	__u8  padding[12];
> > > > 	/* Vendor specific data */
> > > > 	union {
> > > > 		struct iommu_gpasid_bind_data_vtd vtd;
> > > > 	};
> > > > };
> > > > 
> > > > 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)}, ...
> > > > };
> > > > 
> > > > V2, Add new_member at the end (forget padding for now).
> > > > struct iommu_gpasid_bind_data {
> > > > 	__u32 version;
> > > > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > > > 	__u32 format;
> > > > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID
> > > > valid */ #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new
> > > > member added */ __u64 flags;
> > > > 	__u64 gpgd;
> > > > 	__u64 hpasid;
> > > > 	__u64 gpasid;
> > > > 	__u32 addr_width;
> > > > 	__u8  padding[12];
> > > > 	/* Vendor specific data */
> > > > 	union {
> > > > 		struct iommu_gpasid_bind_data_vtd vtd;
> > > > 	};
> > > > 	__u64 new_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), offsetofend(struct
> > > > iommu_gpasid_bind_data,new_member)},
> > > > 
> > > > };
> > > > 
> > > > V3, Add smmu to the union,larger than vtd
> > > > 
> > > > struct iommu_gpasid_bind_data {
> > > > 	__u32 version;
> > > > #define IOMMU_PASID_FORMAT_INTEL_VTD	1
> > > > #define IOMMU_PASID_FORMAT_INTEL_SMMU	2
> > > > 	__u32 format;
> > > > #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID
> > > > valid */ #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new
> > > > member added */ #define IOMMU_SVA_SMMU_SUPP	(1 << 2) /*
> > > > SMMU data supported */ __u64 flags;
> > > > 	__u64 gpgd;
> > > > 	__u64 hpasid;
> > > > 	__u64 gpasid;
> > > > 	__u32 addr_width;
> > > > 	__u8  padding[12];
> > > > 	/* Vendor specific data */
> > > > 	union {
> > > > 		struct iommu_gpasid_bind_data_vtd vtd;
> > > > 		struct iommu_gpasid_bind_data_smmu smmu;
> > > > 	};
> > > > 	__u64 new_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),
> > > > 	offsetofend(struct iommu_gpasid_bind_data, new_member),
> > > > 	offsetofend(struct iommu_gpasid_bind_data, new_member)},
> > > > ...
> > > > };
> > > >       
> > > 
> > > How are you not breaking rule #3, "Versions are backward
> > > compatible" with this?  If the kernel is at version 3 and
> > > userspace is at version 2 then new_member exists at different
> > > offsets of the structure.  The kernels iommu_uapi_data_size for
> > > V2 changed between version 2 and 3. Thanks,
> > >     
> > You are right. if we want to add new member to the end of the
> > structure as well as expanding union, I think we have to fix the
> > size of the union. Would this work? (just an example for one struct)
> > 
> > 
> > @@ -344,6 +348,11 @@ struct iommu_gpasid_bind_data_vtd {
> >   * @gpasid:    Process address space ID used for the guest mm in
> > guest IOMMU
> >   * @addr_width:        Guest virtual address width
> >   * @padding:   Reserved for future use (should be zero)
> > + * @dummy      Reserve space for vendor specific data in the
> > union. New
> > + *             members added to the union cannot exceed the size of
> > dummy.
> > + *             The fixed size union is needed to allow further
> > expansion
> > + *             after the end of the union while still maintain
> > backward
> > + *             compatibility.
> >   * @vtd:       Intel VT-d specific data
> >   *
> >   * Guest to host PASID mapping can be an identity or non-identity,
> > where guest @@ -365,6 +374,7 @@ struct iommu_gpasid_bind_data {
> >         __u8  padding[12];
> >         /* Vendor specific data */
> >         union {
> > +               __u8 dummy[128];
> >                 struct iommu_gpasid_bind_data_vtd vtd;
> >         };
> >  };  
> 
> It's not the most space efficient thing and we're just guessing at
> what might need to be added into that union in future, but it
> works... until it doesn't ;)  One might also argue that we could
> inflate the padding field even further to serve the same purpose.
That was our initial intention, the padding field is already inflated
to accommodate any foreseeable extensions :)

Extensions beyond union was deemed unlikely that is why we use the
union at the end.

The intention of this patchset is not to change that, but rather
clarify and simplify the version checking.

> The only other route I can think of would be to let the user specify
> the offset of the variable size data from the start of the structure,
> for example similar to how we're laying out vfio migration region or
> how we do capabilities in vfio ioctls.  This is where passing an
> argsz for each ioctl comes in handy so we're not limited to a
> structure, we can link various structures together in a chain.  Of
> course that requires work on both the user and kernel side to pack
> and unpack, but it leaves a lot of flexibility in extending it.
> Thanks,
> 
Yeah, that would work as well. I just feel IOMMU UAPI is unlikely to get
updated frequently, should be much less than adding new capabilities.
I think argsz could be viewed as the version field set by the
user, minsz is what kernel current code supports.

So let me summarize the options we have
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.

Jean, Eric, any comments? My preference is #1. In the apocalyptic event
when we run out of padding, perhaps we can introduce a new API_v2 :)

> Alex
> 

[Jacob Pan]

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

[Auger Eric]

Hi Jacob,
On 2/3/20 11:41 PM, Jacob Pan wrote:
> On Mon, 3 Feb 2020 14:12:36 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
>> On Mon, 3 Feb 2020 12:41:43 -0800
>> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
>>
>>> On Mon, 3 Feb 2020 11:27:08 -0700
>>> Alex Williamson <alex.williamson@redhat.com> wrote:
>>>   
>>>> On Fri, 31 Jan 2020 15:51:25 -0800
>>>> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
>>>>     
>>>>> Hi Alex,
>>>>> Sorry I missed this part in the previous reply. Comments below.
>>>>>
>>>>> On Wed, 29 Jan 2020 15:19:51 -0700
>>>>> Alex Williamson <alex.williamson@redhat.com> wrote:
>>>>>       
>>>>>> Also, is the 12-bytes of padding in struct
>>>>>> iommu_gpasid_bind_data excessive with this new versioning
>>>>>> scheme?  Per rule #2 I'm not sure if we're allowed to
>>>>>> repurpose those padding bytes,        
>>>>> We can still use the padding bytes as long as there is a new
>>>>> flag bit to indicate the validity of the new filed within the
>>>>> padding. I should have made it clear in rule #2 when mentioning
>>>>> the flags bits. Should define what extension constitutes.
>>>>> How about this?
>>>>> "
>>>>>  * 2. Data structures are open to extension but closed to
>>>>> modification.
>>>>>  *    Extension should leverage the padding bytes first where a
>>>>> new
>>>>>  *    flag bit is required to indicate the validity of each new
>>>>> member.
>>>>>  *    The above rule for padding bytes also applies to adding
>>>>> new union
>>>>>  *    members.
>>>>>  *    After padding bytes are exhausted, new fields must be
>>>>> added at the
>>>>>  *    end of each data structure with 64bit alignment. Flag bits
>>>>> can be
>>>>>  *    added without size change but existing ones cannot be
>>>>> altered. *
>>>>> "
>>>>> So if we add new field by doing re-purpose of padding bytes,
>>>>> size lookup result will remain the same. New code would
>>>>> recognize the new flag, old code stays the same.
>>>>>
>>>>> VFIO layer checks for UAPI compatibility and size to copy,
>>>>> version sanity check and flag usage are done in the IOMMU code.
>>>>>       
>>>>>> but if we add
>>>>>> fields to the end of the structure as the scheme suggests,
>>>>>> we're stuck with not being able to expand the union for new
>>>>>> fields.        
>>>>> Good point, it does sound contradictory. I hope the rewritten
>>>>> rule #2 address that.
>>>>> Adding data after the union should be extremely rare. Do you
>>>>> see any issues with the example below?
>>>>>  
>>>>>  offsetofend() can still find the right size.
>>>>> e.g.
>>>>> V1
>>>>> struct iommu_gpasid_bind_data {
>>>>> 	__u32 version;
>>>>> #define IOMMU_PASID_FORMAT_INTEL_VTD	1
>>>>> 	__u32 format;
>>>>> #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID
>>>>> valid */ __u64 flags;
>>>>> 	__u64 gpgd;
>>>>> 	__u64 hpasid;
>>>>> 	__u64 gpasid;
>>>>> 	__u32 addr_width;
>>>>> 	__u8  padding[12];
>>>>> 	/* Vendor specific data */
>>>>> 	union {
>>>>> 		struct iommu_gpasid_bind_data_vtd vtd;
>>>>> 	};
>>>>> };
>>>>>
>>>>> 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)}, ...
>>>>> };
>>>>>
>>>>> V2, Add new_member at the end (forget padding for now).
>>>>> struct iommu_gpasid_bind_data {
>>>>> 	__u32 version;
>>>>> #define IOMMU_PASID_FORMAT_INTEL_VTD	1
>>>>> 	__u32 format;
>>>>> #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID
>>>>> valid */ #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new
>>>>> member added */ __u64 flags;
>>>>> 	__u64 gpgd;
>>>>> 	__u64 hpasid;
>>>>> 	__u64 gpasid;
>>>>> 	__u32 addr_width;
>>>>> 	__u8  padding[12];
>>>>> 	/* Vendor specific data */
>>>>> 	union {
>>>>> 		struct iommu_gpasid_bind_data_vtd vtd;
>>>>> 	};
>>>>> 	__u64 new_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), offsetofend(struct
>>>>> iommu_gpasid_bind_data,new_member)},
>>>>>
>>>>> };
>>>>>
>>>>> V3, Add smmu to the union,larger than vtd
>>>>>
>>>>> struct iommu_gpasid_bind_data {
>>>>> 	__u32 version;
>>>>> #define IOMMU_PASID_FORMAT_INTEL_VTD	1
>>>>> #define IOMMU_PASID_FORMAT_INTEL_SMMU	2
>>>>> 	__u32 format;
>>>>> #define IOMMU_SVA_GPASID_VAL	(1 << 0) /* guest PASID
>>>>> valid */ #define IOMMU_NEW_MEMBER_VAL	(1 << 1) /* new
>>>>> member added */ #define IOMMU_SVA_SMMU_SUPP	(1 << 2) /*
>>>>> SMMU data supported */ __u64 flags;
>>>>> 	__u64 gpgd;
>>>>> 	__u64 hpasid;
>>>>> 	__u64 gpasid;
>>>>> 	__u32 addr_width;
>>>>> 	__u8  padding[12];
>>>>> 	/* Vendor specific data */
>>>>> 	union {
>>>>> 		struct iommu_gpasid_bind_data_vtd vtd;
>>>>> 		struct iommu_gpasid_bind_data_smmu smmu;
>>>>> 	};
>>>>> 	__u64 new_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),
>>>>> 	offsetofend(struct iommu_gpasid_bind_data, new_member),
>>>>> 	offsetofend(struct iommu_gpasid_bind_data, new_member)},
>>>>> ...
>>>>> };
>>>>>       
>>>>
>>>> How are you not breaking rule #3, "Versions are backward
>>>> compatible" with this?  If the kernel is at version 3 and
>>>> userspace is at version 2 then new_member exists at different
>>>> offsets of the structure.  The kernels iommu_uapi_data_size for
>>>> V2 changed between version 2 and 3. Thanks,
>>>>     
>>> You are right. if we want to add new member to the end of the
>>> structure as well as expanding union, I think we have to fix the
>>> size of the union. Would this work? (just an example for one struct)
>>>
>>>
>>> @@ -344,6 +348,11 @@ struct iommu_gpasid_bind_data_vtd {
>>>   * @gpasid:    Process address space ID used for the guest mm in
>>> guest IOMMU
>>>   * @addr_width:        Guest virtual address width
>>>   * @padding:   Reserved for future use (should be zero)
>>> + * @dummy      Reserve space for vendor specific data in the
>>> union. New
>>> + *             members added to the union cannot exceed the size of
>>> dummy.
>>> + *             The fixed size union is needed to allow further
>>> expansion
>>> + *             after the end of the union while still maintain
>>> backward
>>> + *             compatibility.
>>>   * @vtd:       Intel VT-d specific data
>>>   *
>>>   * Guest to host PASID mapping can be an identity or non-identity,
>>> where guest @@ -365,6 +374,7 @@ struct iommu_gpasid_bind_data {
>>>         __u8  padding[12];
>>>         /* Vendor specific data */
>>>         union {
>>> +               __u8 dummy[128];
>>>                 struct iommu_gpasid_bind_data_vtd vtd;
>>>         };
>>>  };  
>>
>> It's not the most space efficient thing and we're just guessing at
>> what might need to be added into that union in future, but it
>> works... until it doesn't ;)  One might also argue that we could
>> inflate the padding field even further to serve the same purpose.
> That was our initial intention, the padding field is already inflated
> to accommodate any foreseeable extensions :)
> 
> Extensions beyond union was deemed unlikely that is why we use the
> union at the end.
> 
> The intention of this patchset is not to change that, but rather
> clarify and simplify the version checking.
> 
>> The only other route I can think of would be to let the user specify
>> the offset of the variable size data from the start of the structure,
>> for example similar to how we're laying out vfio migration region or
>> how we do capabilities in vfio ioctls.  This is where passing an
>> argsz for each ioctl comes in handy so we're not limited to a
>> structure, we can link various structures together in a chain.  Of
>> course that requires work on both the user and kernel side to pack
>> and unpack, but it leaves a lot of flexibility in extending it.
>> Thanks,
>>
> Yeah, that would work as well. I just feel IOMMU UAPI is unlikely to get
> updated frequently, should be much less than adding new capabilities.
> I think argsz could be viewed as the version field set by the
> user, minsz is what kernel current code supports.
> 
> So let me summarize the options we have
> 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.
> 
> Jean, Eric, any comments? My preference is #1. In the apocalyptic event
> when we run out of padding, perhaps we can introduce a new API_v2 :)
I had #1 in mind too.

Thanks

Eric
> 
>> Alex
>>
> 
> [Jacob Pan]
> 

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

[Jean-Philippe Brucker]

On Mon, Feb 03, 2020 at 02:41:02PM -0800, Jacob Pan wrote:
> Yeah, that would work as well. I just feel IOMMU UAPI is unlikely to get
> updated frequently, should be much less than adding new capabilities.
> I think argsz could be viewed as the version field set by the
> user, minsz is what kernel current code supports.
> 
> So let me summarize the options we have
> 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.
> 
> Jean, Eric, any comments? My preference is #1. In the apocalyptic event
> when we run out of padding, perhaps we can introduce a new API_v2 :)

I agree, new extensions will most likely want to extend the vendor
specific structures at the end rather than introduce new common fields, so
I prefer #1 which avoids fixing the union size.

Thanks,
Jean

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

[Jacob Pan]

On Wed, 29 Jan 2020 14:40:46 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Tue, 28 Jan 2020 22:02:04 -0800
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > 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.
> > 
> > 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 | 22 ++++++++++++++++++++++
> >  include/linux/iommu.h |  6 ++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 7dd51c5d2ba1..9e5de9abebdf 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1696,6 +1696,28 @@ 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, the
> > offsetofend()
> > + * will identify the new sizes.
> > + */
> > +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)
> > +{  
> 
> Seems like this is asking for a bounds check,
> 
>   if (type >= NR_IOMMU_UAPI_TYPE || version > IOMMU_UAPI_VERSION)
>   	return -EINVAL;
> 
yes, agreed.
> If we add new types in future versions, I assume we'd back fill the
> table with -EINVAL as well (rather than zero).  Thanks,
> 
right, if the array increase due to new types, the older version with
the new type should be filled with -EINVAL.
Let me document this in the rules of extensions.

> Alex
> 
> > +	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 9718c109ea0a..416fe02160ba 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -500,6 +500,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); @@ -885,6 +886,11 @@ static inline int
> > iommu_page_response(struct device *dev, return -ENODEV;
> >  }
> >  
> > +static int iommu_uapi_get_data_size(int type, int version)
> > +{
> > +	return -ENODEV;
> > +}
> > +
> >  static inline int iommu_group_id(struct iommu_group *group)
> >  {
> >  	return -ENODEV;  
> 

[Jacob Pan]