IOMMU Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/3] IOMMU user API enhancement
@ 2020-03-25 23:17 Jacob Pan
  2020-03-25 23:17 ` [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities Jacob Pan
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Jacob Pan @ 2020-03-25 23:17 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson, Lu Baolu, iommu, LKML,
	David Woodhouse, Jean-Philippe Brucker
  Cc: Tian, Kevin, Raj Ashok

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-25 23:17 [PATCH v2 0/3] IOMMU user API enhancement Jacob Pan
@ 2020-03-25 23:17 ` Jacob Pan
  2020-03-26  9:23   ` Christoph Hellwig
  2020-03-25 23:17 ` [PATCH v2 2/3] iommu/uapi: Use unified UAPI version Jacob Pan
  2020-03-25 23:17 ` [PATCH v2 3/3] iommu/uapi: Add helper function for size lookup Jacob Pan
  2 siblings, 1 reply; 14+ messages in thread
From: Jacob Pan @ 2020-03-25 23:17 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson, Lu Baolu, iommu, LKML,
	David Woodhouse, Jean-Philippe Brucker
  Cc: Tian, Kevin, Raj Ashok

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 2/3] iommu/uapi: Use unified UAPI version
  2020-03-25 23:17 [PATCH v2 0/3] IOMMU user API enhancement Jacob Pan
  2020-03-25 23:17 ` [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities Jacob Pan
@ 2020-03-25 23:17 ` Jacob Pan
  2020-03-25 23:17 ` [PATCH v2 3/3] iommu/uapi: Add helper function for size lookup Jacob Pan
  2 siblings, 0 replies; 14+ messages in thread
From: Jacob Pan @ 2020-03-25 23:17 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson, Lu Baolu, iommu, LKML,
	David Woodhouse, Jean-Philippe Brucker
  Cc: Tian, Kevin, Raj Ashok

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 3/3] iommu/uapi: Add helper function for size lookup
  2020-03-25 23:17 [PATCH v2 0/3] IOMMU user API enhancement Jacob Pan
  2020-03-25 23:17 ` [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities Jacob Pan
  2020-03-25 23:17 ` [PATCH v2 2/3] iommu/uapi: Use unified UAPI version Jacob Pan
@ 2020-03-25 23:17 ` Jacob Pan
  2 siblings, 0 replies; 14+ messages in thread
From: Jacob Pan @ 2020-03-25 23:17 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson, Lu Baolu, iommu, LKML,
	David Woodhouse, Jean-Philippe Brucker
  Cc: Tian, Kevin, Raj Ashok

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-25 23:17 ` [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities Jacob Pan
@ 2020-03-26  9:23   ` Christoph Hellwig
  2020-03-26 16:44     ` Jacob Pan
  0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2020-03-26  9:23 UTC (permalink / raw)
  To: Jacob Pan
  Cc: Tian, Kevin, Raj Ashok, Jean-Philippe Brucker, LKML, iommu,
	Alex Williamson, David Woodhouse

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-26  9:23   ` Christoph Hellwig
@ 2020-03-26 16:44     ` Jacob Pan
  2020-03-27  2:49       ` Tian, Kevin
  0 siblings, 1 reply; 14+ messages in thread
From: Jacob Pan @ 2020-03-26 16:44 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Tian, Kevin, Raj Ashok, Jean-Philippe Brucker, LKML, iommu,
	Alex Williamson, David Woodhouse

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-26 16:44     ` Jacob Pan
@ 2020-03-27  2:49       ` Tian, Kevin
  2020-03-27  7:47         ` Christoph Hellwig
  0 siblings, 1 reply; 14+ messages in thread
From: Tian, Kevin @ 2020-03-27  2:49 UTC (permalink / raw)
  To: Jacob Pan, Christoph Hellwig
  Cc: Raj, Ashok, Jean-Philippe Brucker, LKML, iommu, Alex Williamson,
	David Woodhouse

> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-27  2:49       ` Tian, Kevin
@ 2020-03-27  7:47         ` Christoph Hellwig
  2020-03-27 23:53           ` Jacob Pan
  0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2020-03-27  7:47 UTC (permalink / raw)
  To: Tian, Kevin
  Cc: Raj, Ashok, Jean-Philippe Brucker, iommu, LKML, Alex Williamson,
	David Woodhouse

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-27  7:47         ` Christoph Hellwig
@ 2020-03-27 23:53           ` Jacob Pan
  2020-03-30  5:40             ` Tian, Kevin
  0 siblings, 1 reply; 14+ messages in thread
From: Jacob Pan @ 2020-03-27 23:53 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Tian, Kevin, Raj, Ashok, Jean-Philippe Brucker, LKML, iommu,
	Alex Williamson, David Woodhouse

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-27 23:53           ` Jacob Pan
@ 2020-03-30  5:40             ` Tian, Kevin
  2020-03-30 16:07               ` Jacob Pan
  0 siblings, 1 reply; 14+ messages in thread
From: Tian, Kevin @ 2020-03-30  5:40 UTC (permalink / raw)
  To: Jacob Pan, Christoph Hellwig
  Cc: Raj, Ashok, Jean-Philippe Brucker, LKML, iommu, Alex Williamson,
	David Woodhouse

> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-30  5:40             ` Tian, Kevin
@ 2020-03-30 16:07               ` Jacob Pan
  2020-03-31  6:06                 ` Tian, Kevin
  0 siblings, 1 reply; 14+ messages in thread
From: Jacob Pan @ 2020-03-30 16:07 UTC (permalink / raw)
  To: Tian, Kevin
  Cc: Raj, Ashok, Jean-Philippe Brucker, LKML, iommu, Alex Williamson,
	David Woodhouse

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-30 16:07               ` Jacob Pan
@ 2020-03-31  6:06                 ` Tian, Kevin
  2020-03-31 15:54                   ` Jacob Pan
  0 siblings, 1 reply; 14+ messages in thread
From: Tian, Kevin @ 2020-03-31  6:06 UTC (permalink / raw)
  To: Jacob Pan
  Cc: Raj, Ashok, Jean-Philippe Brucker, LKML, iommu, Alex Williamson,
	David Woodhouse

> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-31  6:06                 ` Tian, Kevin
@ 2020-03-31 15:54                   ` Jacob Pan
  2020-04-01  5:32                     ` Tian, Kevin
  0 siblings, 1 reply; 14+ messages in thread
From: Jacob Pan @ 2020-03-31 15:54 UTC (permalink / raw)
  To: Tian, Kevin
  Cc: Raj, Ashok, Jean-Philippe Brucker, LKML, iommu, Alex Williamson,
	David Woodhouse

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
  2020-03-31 15:54                   ` Jacob Pan
@ 2020-04-01  5:32                     ` Tian, Kevin
  0 siblings, 0 replies; 14+ messages in thread
From: Tian, Kevin @ 2020-04-01  5:32 UTC (permalink / raw)
  To: Jacob Pan
  Cc: Raj, Ashok, Jean-Philippe Brucker, LKML, iommu, Alex Williamson,
	David Woodhouse

> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-25 23:17 [PATCH v2 0/3] IOMMU user API enhancement Jacob Pan
2020-03-25 23:17 ` [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities Jacob Pan
2020-03-26  9:23   ` Christoph Hellwig
2020-03-26 16:44     ` Jacob Pan
2020-03-27  2:49       ` Tian, Kevin
2020-03-27  7:47         ` Christoph Hellwig
2020-03-27 23:53           ` Jacob Pan
2020-03-30  5:40             ` Tian, Kevin
2020-03-30 16:07               ` Jacob Pan
2020-03-31  6:06                 ` Tian, Kevin
2020-03-31 15:54                   ` Jacob Pan
2020-04-01  5:32                     ` Tian, Kevin
2020-03-25 23:17 ` [PATCH v2 2/3] iommu/uapi: Use unified UAPI version Jacob Pan
2020-03-25 23:17 ` [PATCH v2 3/3] iommu/uapi: Add helper function for size lookup Jacob Pan

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git