Skip to contentSkip to conversation

Remove VT-d virtual command interface

PATCH Superseded Jacob Pan 4 participants 22 messages Feb 10, 2023 – Feb 15, 2023

Subsystems (2)

v1 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Jacob Pan

author
#0 lore raw

cover Remove VT-d virtual command interface

Hi all,

This patch set removes unused VT-d virtual command interface followed by clean up in the IOASID code.

This has only been tested on x86 platforms, need help with testing on ARM SMMU and other architectures.

I also hope to collect feedback on the upcoming clean up and enhancements below:
1. Consolidate VT-d private SVA PASID array with IOASID common code
2. As we retain the global IOASID allocator, we need some level of resource management. I want to restart the effort to include IOASID under misc cgroup.
(https://lore.kernel.org/linux-iommu/20210303160205.151d114e@jacob-builder/)

Thanks,

Jacob

Jacob Pan (2):
iommu/vt-d: Remove virtual command interface
iommu/ioasid: Remove custom IOASID allocator

 drivers/iommu/intel/cap_audit.c |   2 -
 drivers/iommu/intel/dmar.c      |   2 -
 drivers/iommu/intel/iommu.c     |  85 ---------
 drivers/iommu/intel/iommu.h     |   8 -
 drivers/iommu/ioasid.c          | 293 +-------------------------------
 include/linux/ioasid.h          |  28 ---
 6 files changed, 9 insertions(+), 409 deletions(-)
Signature · 1 line
2.25.1
author
#1 lore raw

PATCH 1/2 iommu/vt-d: Remove virtual command interface

Virtual command interface was introduced to allow using host PASIDs inside VMs. It is unused and abandoned due to architectural change.

With this patch, we can safely remove this feature and the related helpers.

---
 drivers/iommu/intel/cap_audit.c |  2 -
 drivers/iommu/intel/dmar.c      |  2 -
 drivers/iommu/intel/iommu.c     | 85 ---------------------------------
 drivers/iommu/intel/iommu.h     |  8 ----
 4 files changed, 97 deletions(-)
drivers/iommu/intel/cap_audit.c +0 -2
index 806986696841..9862dc20b35e 100644
@@ -54,7 +54,6 @@ static inline void check_dmar_capabilities(struct intel_iommu *a,
 	CHECK_FEATURE_MISMATCH(a, b, ecap, slts, ECAP_SLTS_MASK);
 	CHECK_FEATURE_MISMATCH(a, b, ecap, nwfs, ECAP_NWFS_MASK);
 	CHECK_FEATURE_MISMATCH(a, b, ecap, slads, ECAP_SLADS_MASK);
-	CHECK_FEATURE_MISMATCH(a, b, ecap, vcs, ECAP_VCS_MASK);
 	CHECK_FEATURE_MISMATCH(a, b, ecap, smts, ECAP_SMTS_MASK);
 	CHECK_FEATURE_MISMATCH(a, b, ecap, pds, ECAP_PDS_MASK);
 	CHECK_FEATURE_MISMATCH(a, b, ecap, dit, ECAP_DIT_MASK);
@@ -101,7 +100,6 @@ static int cap_audit_hotplug(struct intel_iommu *iommu, enum cap_audit_type type
 	CHECK_FEATURE_MISMATCH_HOTPLUG(iommu, ecap, slts, ECAP_SLTS_MASK);
 	CHECK_FEATURE_MISMATCH_HOTPLUG(iommu, ecap, nwfs, ECAP_NWFS_MASK);
 	CHECK_FEATURE_MISMATCH_HOTPLUG(iommu, ecap, slads, ECAP_SLADS_MASK);
-	CHECK_FEATURE_MISMATCH_HOTPLUG(iommu, ecap, vcs, ECAP_VCS_MASK);
 	CHECK_FEATURE_MISMATCH_HOTPLUG(iommu, ecap, smts, ECAP_SMTS_MASK);
 	CHECK_FEATURE_MISMATCH_HOTPLUG(iommu, ecap, pds, ECAP_PDS_MASK);
 	CHECK_FEATURE_MISMATCH_HOTPLUG(iommu, ecap, dit, ECAP_DIT_MASK);
drivers/iommu/intel/dmar.c +0 -2
index b00a0ceb2d13..bf0bfe5ba7a7 100644
@@ -989,8 +989,6 @@ static int map_iommu(struct intel_iommu *iommu, u64 phys_addr)
 		warn_invalid_dmar(phys_addr, " returns all ones");
 		goto unmap;
 	}
-	if (ecap_vcs(iommu->ecap))
-		iommu->vccap = dmar_readq(iommu->reg + DMAR_VCCAP_REG);
 
 	/* the registers might be more than one page */
 	map_size = max_t(int, ecap_max_iotlb_offset(iommu->ecap),
drivers/iommu/intel/iommu.c +0 -85

103 lines not on this page · raw message

drivers/iommu/intel/iommu.h +0 -8
index 06e61e474856..6bdfbead82c4 100644
@@ -184,7 +184,6 @@
 #define ecap_flts(e)		(((e) >> 47) & 0x1)
 #define ecap_slts(e)		(((e) >> 46) & 0x1)
 #define ecap_slads(e)		(((e) >> 45) & 0x1)
-#define ecap_vcs(e)		(((e) >> 44) & 0x1)
 #define ecap_smts(e)		(((e) >> 43) & 0x1)
 #define ecap_dit(e)		(((e) >> 41) & 0x1)
 #define ecap_pds(e)		(((e) >> 42) & 0x1)
@@ -210,9 +209,6 @@
 #define ecap_max_handle_mask(e) (((e) >> 20) & 0xf)
 #define ecap_sc_support(e)	(((e) >> 7) & 0x1) /* Snooping Control */
 
-/* Virtual command interface capability */
-#define vccap_pasid(v)		(((v) & DMA_VCS_PAS)) /* PASID allocation */
-
 /* IOTLB_REG */
 #define DMA_TLB_FLUSH_GRANU_OFFSET  60
 #define DMA_TLB_GLOBAL_FLUSH (((u64)1) << 60)
@@ -307,8 +303,6 @@
 #define DMA_PRS_PPR	((u32)1)
 #define DMA_PRS_PRO	((u32)2)
 
-#define DMA_VCS_PAS	((u64)1)
-
 #define IOMMU_WAIT_OP(iommu, offset, op, cond, sts)			\
 do {									\
 	cycles_t start_time = get_cycles();				\
@@ -560,7 +554,6 @@ struct intel_iommu {
 	u64		reg_size; /* size of hw register set */
 	u64		cap;
 	u64		ecap;
-	u64		vccap;
 	u32		gcmd; /* Holds TE, EAFL. Don't need SRTP, SFL, WBF */
 	raw_spinlock_t	register_lock; /* protect register handling */
 	int		seq_id;	/* sequence id of the iommu */
@@ -583,7 +576,6 @@ struct intel_iommu {
 	unsigned char prq_name[16];    /* Name for PRQ interrupt */
 	unsigned long prq_seq_number;
 	struct completion prq_complete;
-	struct ioasid_allocator_ops pasid_allocator; /* Custom allocator for PASIDs */
 #endif
 	struct iopf_queue *iopf_queue;
 	unsigned char iopfq_name[16];
Signature · 1 line
2.25.1
Reviewed-by on v1 1/2
#2 lore raw

RE: [PATCH 1/2] iommu/vt-d: Remove virtual command interface

Quoted 9 lines

From: Jacob Pan <[email protected]>
Sent: Saturday, February 11, 2023 7:02 AM

Virtual command interface was introduced to allow using host PASIDs inside VMs. It is unused and abandoned due to architectural change.

With this patch, we can safely remove this feature and the related helpers.

Reviewed-by on v1 1/2
#3 lore raw

Re: [PATCH 1/2] iommu/vt-d: Remove virtual command interface

In reply to Jacob Pan
Quoted 5 lines · Jacob Pan

Virtual command interface was introduced to allow using host PASIDs inside VMs. It is unused and abandoned due to architectural change.

With this patch, we can safely remove this feature and the related helpers.

Quoted 7 lines
---
 drivers/iommu/intel/cap_audit.c |  2 -
 drivers/iommu/intel/dmar.c      |  2 -
 drivers/iommu/intel/iommu.c     | 85 ---------------------------------
 drivers/iommu/intel/iommu.h     |  8 ----
 4 files changed, 97 deletions(-)

Jason

author
#4 lore raw

PATCH 2/2 iommu/ioasid: Remove custom IOASID allocator

In reply to Jacob Pan

Custom allocator feature was introduced to support VT-d's virtual command, an enlightened interface designed for VMs to allocate PASIDs from the host.

As we remove/withdraw the VT-d virtual command feature, the sole user of custom allocator, we can safely remove the custom allocator as well. Effectively, this will return IOASID core to the original simple global namespace allocator.

---
 drivers/iommu/ioasid.c | 293 ++---------------------------------------
 include/linux/ioasid.h |  28 ----
 2 files changed, 9 insertions(+), 312 deletions(-)
drivers/iommu/ioasid.c +9 -284
index a786c034907c..85715e171db2 100644
@@ -16,246 +16,7 @@ struct ioasid_data {
 	void *private;
 	struct rcu_head rcu;
 };
-
-/*
- * struct ioasid_allocator_data - Internal data structure to hold information
- * about an allocator. There are two types of allocators:
- *
- * - Default allocator always has its own XArray to track the IOASIDs allocated.
- * - Custom allocators may share allocation helpers with different private data.
- *   Custom allocators that share the same helper functions also share the same
- *   XArray.
- * Rules:
- * 1. Default allocator is always available, not dynamically registered. This is
- *    to prevent race conditions with early boot code that want to register
- *    custom allocators or allocate IOASIDs.
- * 2. Custom allocators take precedence over the default allocator.
- * 3. When all custom allocators sharing the same helper functions are
- *    unregistered (e.g. due to hotplug), all outstanding IOASIDs must be
- *    freed. Otherwise, outstanding IOASIDs will be lost and orphaned.
- * 4. When switching between custom allocators sharing the same helper
- *    functions, outstanding IOASIDs are preserved.
- * 5. When switching between custom allocator and default allocator, all IOASIDs
- *    must be freed to ensure unadulterated space for the new allocator.
- *
- * @ops:	allocator helper functions and its data
- * @list:	registered custom allocators
- * @slist:	allocators share the same ops but different data
- * @flags:	attributes of the allocator
- * @xa:		xarray holds the IOASID space
- * @rcu:	used for kfree_rcu when unregistering allocator
- */
-struct ioasid_allocator_data {
-	struct ioasid_allocator_ops *ops;
-	struct list_head list;
-	struct list_head slist;
-#define IOASID_ALLOCATOR_CUSTOM BIT(0) /* Needs framework to track results */
-	unsigned long flags;
-	struct xarray xa;
-	struct rcu_head rcu;
-};
-
-static DEFINE_SPINLOCK(ioasid_allocator_lock);
-static LIST_HEAD(allocators_list);
-
-static ioasid_t default_alloc(ioasid_t min, ioasid_t max, void *opaque);
-static void default_free(ioasid_t ioasid, void *opaque);
-
-static struct ioasid_allocator_ops default_ops = {
-	.alloc = default_alloc,
-	.free = default_free,
-};
-
-static struct ioasid_allocator_data default_allocator = {
-	.ops = &default_ops,
-	.flags = 0,
-	.xa = XARRAY_INIT(ioasid_xa, XA_FLAGS_ALLOC),
-};
-
-static struct ioasid_allocator_data *active_allocator = &default_allocator;
-
-static ioasid_t default_alloc(ioasid_t min, ioasid_t max, void *opaque)
-{
-	ioasid_t id;
-
-	if (xa_alloc(&default_allocator.xa, &id, opaque, XA_LIMIT(min, max), GFP_ATOMIC)) {
-		pr_err("Failed to alloc ioasid from %d to %d\n", min, max);
-		return INVALID_IOASID;
-	}
-
-	return id;
-}
-
-static void default_free(ioasid_t ioasid, void *opaque)
-{
-	struct ioasid_data *ioasid_data;
-
-	ioasid_data = xa_erase(&default_allocator.xa, ioasid);
-	kfree_rcu(ioasid_data, rcu);
-}
-
-/* Allocate and initialize a new custom allocator with its helper functions */
-static struct ioasid_allocator_data *ioasid_alloc_allocator(struct ioasid_allocator_ops *ops)
-{
-	struct ioasid_allocator_data *ia_data;
-
-	ia_data = kzalloc(sizeof(*ia_data), GFP_ATOMIC);
-	if (!ia_data)
-		return NULL;
-
-	xa_init_flags(&ia_data->xa, XA_FLAGS_ALLOC);
-	INIT_LIST_HEAD(&ia_data->slist);
-	ia_data->flags |= IOASID_ALLOCATOR_CUSTOM;
-	ia_data->ops = ops;
-
-	/* For tracking custom allocators that share the same ops */
-	list_add_tail(&ops->list, &ia_data->slist);
-
-	return ia_data;
-}
-
-static bool use_same_ops(struct ioasid_allocator_ops *a, struct ioasid_allocator_ops *b)
-{
-	return (a->free == b->free) && (a->alloc == b->alloc);
-}
-
-/**
- * ioasid_register_allocator - register a custom allocator
- * @ops: the custom allocator ops to be registered
- *
- * Custom allocators take precedence over the default xarray based allocator.
- * Private data associated with the IOASID allocated by the custom allocators
- * are managed by IOASID framework similar to data stored in xa by default
- * allocator.
- *
- * There can be multiple allocators registered but only one is active. In case
- * of runtime removal of a custom allocator, the next one is activated based
- * on the registration ordering.
- *
- * Multiple allocators can share the same alloc() function, in this case the
- * IOASID space is shared.
- */
-int ioasid_register_allocator(struct ioasid_allocator_ops *ops)
-{
-	struct ioasid_allocator_data *ia_data;
-	struct ioasid_allocator_data *pallocator;
-	int ret = 0;
-
-	spin_lock(&ioasid_allocator_lock);
-
-	ia_data = ioasid_alloc_allocator(ops);
-	if (!ia_data) {
-		ret = -ENOMEM;
-		goto out_unlock;
-	}
-
-	/*
-	 * No particular preference, we activate the first one and keep
-	 * the later registered allocators in a list in case the first one gets
-	 * removed due to hotplug.
-	 */
-	if (list_empty(&allocators_list)) {
-		WARN_ON(active_allocator != &default_allocator);
-		/* Use this new allocator if default is not active */
-		if (xa_empty(&active_allocator->xa)) {
-			rcu_assign_pointer(active_allocator, ia_data);
-			list_add_tail(&ia_data->list, &allocators_list);
-			goto out_unlock;
-		}
-		pr_warn("Default allocator active with outstanding IOASID\n");
-		ret = -EAGAIN;
-		goto out_free;
-	}
-
-	/* Check if the allocator is already registered */
-	list_for_each_entry(pallocator, &allocators_list, list) {
-		if (pallocator->ops == ops) {
-			pr_err("IOASID allocator already registered\n");
-			ret = -EEXIST;
-			goto out_free;
-		} else if (use_same_ops(pallocator->ops, ops)) {
-			/*
-			 * If the new allocator shares the same ops,
-			 * then they will share the same IOASID space.
-			 * We should put them under the same xarray.
-			 */
-			list_add_tail(&ops->list, &pallocator->slist);
-			goto out_free;
-		}
-	}
-	list_add_tail(&ia_data->list, &allocators_list);
-
-	spin_unlock(&ioasid_allocator_lock);
-	return 0;
-out_free:
-	kfree(ia_data);
-out_unlock:
-	spin_unlock(&ioasid_allocator_lock);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(ioasid_register_allocator);
-
-/**
- * ioasid_unregister_allocator - Remove a custom IOASID allocator ops
- * @ops: the custom allocator to be removed
- *
- * Remove an allocator from the list, activate the next allocator in
- * the order it was registered. Or revert to default allocator if all
- * custom allocators are unregistered without outstanding IOASIDs.
- */
-void ioasid_unregister_allocator(struct ioasid_allocator_ops *ops)
-{
-	struct ioasid_allocator_data *pallocator;
-	struct ioasid_allocator_ops *sops;
-
-	spin_lock(&ioasid_allocator_lock);
-	if (list_empty(&allocators_list)) {
-		pr_warn("No custom IOASID allocators active!\n");
-		goto exit_unlock;
-	}
-
-	list_for_each_entry(pallocator, &allocators_list, list) {
-		if (!use_same_ops(pallocator->ops, ops))
-			continue;
-
-		if (list_is_singular(&pallocator->slist)) {
-			/* No shared helper functions */
-			list_del(&pallocator->list);
-			/*
-			 * All IOASIDs should have been freed before
-			 * the last allocator that shares the same ops
-			 * is unregistered.
-			 */
-			WARN_ON(!xa_empty(&pallocator->xa));
-			if (list_empty(&allocators_list)) {
-				pr_info("No custom IOASID allocators, switch to default.\n");
-				rcu_assign_pointer(active_allocator, &default_allocator);
-			} else if (pallocator == active_allocator) {
-				rcu_assign_pointer(active_allocator,
-						list_first_entry(&allocators_list,
-								struct ioasid_allocator_data, list));
-				pr_info("IOASID allocator changed");
-			}
-			kfree_rcu(pallocator, rcu);
-			break;
-		}
-		/*
-		 * Find the matching shared ops to delete,
-		 * but keep outstanding IOASIDs
-		 */
-		list_for_each_entry(sops, &pallocator->slist, list) {
-			if (sops == ops) {
-				list_del(&ops->list);
-				break;
-			}
-		}
-		break;
-	}
-
-exit_unlock:
-	spin_unlock(&ioasid_allocator_lock);
-}
-EXPORT_SYMBOL_GPL(ioasid_unregister_allocator);
+static DEFINE_XARRAY_ALLOC(ioasid_xa);
 
 /**
  * ioasid_set_data - Set private data for an allocated ioasid
@@ -270,13 +31,13 @@ int ioasid_set_data(ioasid_t ioasid, void *data)
 	struct ioasid_data *ioasid_data;
 	int ret = 0;
 
-	spin_lock(&ioasid_allocator_lock);
-	ioasid_data = xa_load(&active_allocator->xa, ioasid);
+	xa_lock(&ioasid_xa);
+	ioasid_data = xa_load(&ioasid_xa, ioasid);
 	if (ioasid_data)
 		rcu_assign_pointer(ioasid_data->private, data);
 	else
 		ret = -ENOENT;
-	spin_unlock(&ioasid_allocator_lock);
+	xa_unlock(&ioasid_xa);
 
 	/*
 	 * Wait for readers to stop accessing the old private data, so the
@@ -305,7 +66,6 @@ ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, ioasid_t max,
 		      void *private)
 {
 	struct ioasid_data *data;
-	void *adata;
 	ioasid_t id;
 
 	data = kzalloc(sizeof(*data), GFP_ATOMIC);
@@ -314,32 +74,13 @@ ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, ioasid_t max,
 
 	data->set = set;
 	data->private = private;
-
-	/*
-	 * Custom allocator needs allocator data to perform platform specific
-	 * operations.
-	 */
-	spin_lock(&ioasid_allocator_lock);
-	adata = active_allocator->flags & IOASID_ALLOCATOR_CUSTOM ? active_allocator->ops->pdata : data;
-	id = active_allocator->ops->alloc(min, max, adata);
-	if (id == INVALID_IOASID) {
-		pr_err("Failed ASID allocation %lu\n", active_allocator->flags);
-		goto exit_free;
-	}
-
-	if ((active_allocator->flags & IOASID_ALLOCATOR_CUSTOM) &&
-	     xa_alloc(&active_allocator->xa, &id, data, XA_LIMIT(id, id), GFP_ATOMIC)) {
-		/* Custom allocator needs framework to store and track allocation results */
-		pr_err("Failed to alloc ioasid from %d\n", id);
-		active_allocator->ops->free(id, active_allocator->ops->pdata);
+	if (xa_alloc(&ioasid_xa, &id, data, XA_LIMIT(min, max), GFP_ATOMIC)) {
+		pr_err("Failed to alloc ioasid from %d to %d\n", min, max);
 		goto exit_free;
 	}
 	data->id = id;
-
-	spin_unlock(&ioasid_allocator_lock);
 	return id;
 exit_free:
-	spin_unlock(&ioasid_allocator_lock);
 	kfree(data);
 	return INVALID_IOASID;
 }
@@ -353,22 +94,8 @@ void ioasid_free(ioasid_t ioasid)
 {
 	struct ioasid_data *ioasid_data;
 
-	spin_lock(&ioasid_allocator_lock);
-	ioasid_data = xa_load(&active_allocator->xa, ioasid);
-	if (!ioasid_data) {
-		pr_err("Trying to free unknown IOASID %u\n", ioasid);
-		goto exit_unlock;
-	}
-
-	active_allocator->ops->free(ioasid, active_allocator->ops->pdata);
-	/* Custom allocator needs additional steps to free the xa element */
-	if (active_allocator->flags & IOASID_ALLOCATOR_CUSTOM) {
-		ioasid_data = xa_erase(&active_allocator->xa, ioasid);
-		kfree_rcu(ioasid_data, rcu);
-	}
-
-exit_unlock:
-	spin_unlock(&ioasid_allocator_lock);
+	ioasid_data = xa_erase(&ioasid_xa, ioasid);
+	kfree_rcu(ioasid_data, rcu);
 }
 EXPORT_SYMBOL_GPL(ioasid_free);
 
@@ -391,11 +118,9 @@ void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
 {
 	void *priv;
 	struct ioasid_data *ioasid_data;
-	struct ioasid_allocator_data *idata;
 
 	rcu_read_lock();
-	idata = rcu_dereference(active_allocator);
-	ioasid_data = xa_load(&idata->xa, ioasid);
+	ioasid_data = xa_load(&ioasid_xa, ioasid);
 	if (!ioasid_data) {
 		priv = ERR_PTR(-ENOENT);
 		goto unlock;
include/linux/ioasid.h +0 -28
index af1c9d62e642..fdfa70857227 100644
@@ -7,28 +7,11 @@
 
 #define INVALID_IOASID ((ioasid_t)-1)
 typedef unsigned int ioasid_t;
-typedef ioasid_t (*ioasid_alloc_fn_t)(ioasid_t min, ioasid_t max, void *data);
-typedef void (*ioasid_free_fn_t)(ioasid_t ioasid, void *data);
 
 struct ioasid_set {
 	int dummy;
 };
 
-/**
- * struct ioasid_allocator_ops - IOASID allocator helper functions and data
- *
- * @alloc:	helper function to allocate IOASID
- * @free:	helper function to free IOASID
- * @list:	for tracking ops that share helper functions but not data
- * @pdata:	data belong to the allocator, provided when calling alloc()
- */
-struct ioasid_allocator_ops {
-	ioasid_alloc_fn_t alloc;
-	ioasid_free_fn_t free;
-	struct list_head list;
-	void *pdata;
-};
-
 #define DECLARE_IOASID_SET(name) struct ioasid_set name = { 0 }
 
 #if IS_ENABLED(CONFIG_IOASID)
@@ -37,8 +20,6 @@ ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, ioasid_t max,
 void ioasid_free(ioasid_t ioasid);
 void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
 		  bool (*getter)(void *));
-int ioasid_register_allocator(struct ioasid_allocator_ops *allocator);
-void ioasid_unregister_allocator(struct ioasid_allocator_ops *allocator);
 int ioasid_set_data(ioasid_t ioasid, void *data);
 static inline bool pasid_valid(ioasid_t ioasid)
 {
@@ -60,15 +41,6 @@ static inline void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
 	return NULL;
 }
 
-static inline int ioasid_register_allocator(struct ioasid_allocator_ops *allocator)
-{
-	return -ENOTSUPP;
-}
-
-static inline void ioasid_unregister_allocator(struct ioasid_allocator_ops *allocator)
-{
-}
-
 static inline int ioasid_set_data(ioasid_t ioasid, void *data)
 {
 	return -ENOTSUPP;
Signature · 1 line
2.25.1
Reviewed-by on v1 2/2
#5 lore raw

RE: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 7 lines

From: Jacob Pan <[email protected]>
Sent: Saturday, February 11, 2023 7:02 AM

Custom allocator feature was introduced to support VT-d's virtual command, an enlightened interface designed for VMs to allocate PASIDs from the host.

Quoted 6 lines

As we remove/withdraw the VT-d virtual command feature, the sole user of custom allocator, we can safely remove the custom allocator as well. Effectively, this will return IOASID core to the original simple global namespace allocator.

Reviewed-by on v1 2/2
#6 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

In reply to Jacob Pan
Quoted 10 lines · Jacob Pan

Custom allocator feature was introduced to support VT-d's virtual command, an enlightened interface designed for VMs to allocate PASIDs from the host.

As we remove/withdraw the VT-d virtual command feature, the sole user of custom allocator, we can safely remove the custom allocator as well. Effectively, this will return IOASID core to the original simple global namespace allocator.

You can also drop the spinlock.h include. With that:

On a related note, it looks like 100b8a14a370 ("iommu/vt-d: Add pasid private data helpers") removed the last user of ioasid_set_data(). I guess that could be dropped too, unless you plan to still use it?

We could also merge ioasid.c into iommu-sva.c at this point, since I haven't seen any interest for having multiple IOASID sets on Arm, but I'm not sure what the current plan is for vSVA on x86.

Thanks,
Jean

#7 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 16 lines · Jean-Philippe Brucker
Quoted 10 lines · Jacob Pan

Custom allocator feature was introduced to support VT-d's virtual command, an enlightened interface designed for VMs to allocate PASIDs from the host.

As we remove/withdraw the VT-d virtual command feature, the sole user of custom allocator, we can safely remove the custom allocator as well. Effectively, this will return IOASID core to the original simple global namespace allocator.

You can also drop the spinlock.h include. With that:

Quoted 7 lines

On a related note, it looks like 100b8a14a370 ("iommu/vt-d: Add pasid private data helpers") removed the last user of ioasid_set_data(). I guess that could be dropped too, unless you plan to still use it?

We could also merge ioasid.c into iommu-sva.c at this point, since I haven't seen any interest for having multiple IOASID sets on Arm, but I'm not sure what the current plan is for vSVA on x86.

Once the customer allocator is removed this is bascially a thin wrapper around xarray

So anything that needs multiple pasid spaces should just create it on its own directly with xarray

This is bascially a shared xarray for a global pasid space.

Jason

author
#8 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Hi Jason,

Quoted 10 lines · Jason Gunthorpe
Quoted 3 lines

We could also merge ioasid.c into iommu-sva.c at this point, since I haven't seen any interest for having multiple IOASID sets on Arm, but I'm not sure what the current plan is for vSVA on x86.

Once the customer allocator is removed this is bascially a thin wrapper around xarray

So anything that needs multiple pasid spaces should just create it on its own directly with xarray

Just wanted to double check that for devices on VT-d platforms that support both SVA and DMA API with PASID, we will still need a single global pasid space (due to IOTLB tagging). So this is not the "multiple pasid spaces" case here, right?

As I replied to Jean as well, we could use the ioasid_set to separate SVA and DMA API PASIDs.

Thanks,

Jacob

#9 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 11 lines · Jacob Pan

Hi Jason,

Quoted 7 lines · Jason Gunthorpe
Quoted 3 lines

We could also merge ioasid.c into iommu-sva.c at this point, since I haven't seen any interest for having multiple IOASID sets on Arm, but I'm not sure what the current plan is for vSVA on x86.

Once the customer allocator is removed this is bascially a thin wrapper around xarray

Quoted 6 lines
Quoted 3 lines

So anything that needs multiple pasid spaces should just create it on its own directly with xarray

Just wanted to double check that for devices on VT-d platforms that support both SVA and DMA API with PASID, we will still need a single global pasid space (due to IOTLB tagging).

Each driver chooses the PASID allocator it wants to use.

If the driver uses SVA then the driver must select the global SVA allocator. If the driver needs PASID's for other purposes then it must get them from the SVA allocator too.

Quoted 2 lines

As I replied to Jean as well, we could use the ioasid_set to separate SVA and DMA API PASIDs.

That doesn't make sense to me..

Jason

author
#10 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

In reply to Jean-Philippe Brucker

Hi Jean-Philippe,

Quoted 5 lines · Jean-Philippe Brucker
Quoted 4 lines · Jacob Pan

Custom allocator feature was introduced to support VT-d's virtual command, an enlightened interface designed for VMs to allocate PASIDs from the host.

Quoted 9 lines
Quoted 6 lines

As we remove/withdraw the VT-d virtual command feature, the sole user of custom allocator, we can safely remove the custom allocator as well. Effectively, this will return IOASID core to the original simple global namespace allocator.

You can also drop the spinlock.h include. With that:

good catch, thanks

Quoted 6 lines

On a related note, it looks like 100b8a14a370 ("iommu/vt-d: Add pasid private data helpers") removed the last user of ioasid_set_data(). I guess that could be dropped too, unless you plan to still use it?

You are right, will remove.
I was planning on the other way around which will convert VT-d's private pasid data helpers to common ioasid code, but when I look closer the private pasid xa is just holding a list of pasid/mm which could be per iommu not global. Another cleanup I suppose.

Quoted 3 lines

We could also merge ioasid.c into iommu-sva.c at this point, since I haven't seen any interest for having multiple IOASID sets on Arm, but I'm not sure what the current plan is for vSVA on x86.

VT-d do plan to use global PASIDs for DMA API with PASIDs since the work submited via ENQCMDS must use a PASID must != RIDPASID.
https://lore.kernel.org/lkml/[email protected]/T/

So I was thinking a separate ioasid_set for devices that allocates global PASIDs for DMA API usage. ioasid_set will be useful here for limiting lookup and resource management. e.g. PASIDs used under in-kernel DMA API are not subject to cgroups.

Quoted 4 lines

Thanks,
Jean

Thanks,

Jacob

#11 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 9 lines · Jacob Pan
Quoted 4 lines

On a related note, it looks like 100b8a14a370 ("iommu/vt-d: Add pasid private data helpers") removed the last user of ioasid_set_data(). I guess that could be dropped too, unless you plan to still use it?

You are right, will remove.
I was planning on the other way around which will convert VT-d's private pasid data helpers to common ioasid code, but when I look closer the private pasid xa is just holding a list of pasid/mm which could be per iommu not global. Another cleanup I suppose.

Yes that should probably be common to SVA. I'm planning to take another look at SVA on the SMMU side following the recent API changes, and from a quick glance the problem that VT-d's private helpers solves is common.

Quoted 11 lines
Quoted 3 lines

We could also merge ioasid.c into iommu-sva.c at this point, since I haven't seen any interest for having multiple IOASID sets on Arm, but I'm not sure what the current plan is for vSVA on x86.

VT-d do plan to use global PASIDs for DMA API with PASIDs since the work submited via ENQCMDS must use a PASID must != RIDPASID.
https://lore.kernel.org/lkml/[email protected]/T/

So I was thinking a separate ioasid_set for devices that allocates global PASIDs for DMA API usage. ioasid_set will be useful here for limiting lookup and resource management. e.g. PASIDs used under in-kernel DMA API are not subject to cgroups.

Ok. Yes that was the goal of ioasid_set

Thanks,
Jean

#12 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 11 lines · Jean-Philippe Brucker
Quoted 9 lines · Jacob Pan
Quoted 4 lines

On a related note, it looks like 100b8a14a370 ("iommu/vt-d: Add pasid private data helpers") removed the last user of ioasid_set_data(). I guess that could be dropped too, unless you plan to still use it?

You are right, will remove.
I was planning on the other way around which will convert VT-d's private pasid data helpers to common ioasid code, but when I look closer the private pasid xa is just holding a list of pasid/mm which could be per iommu not global. Another cleanup I suppose.

Quoted 4 lines

Yes that should probably be common to SVA. I'm planning to take another look at SVA on the SMMU side following the recent API changes, and from a quick glance the problem that VT-d's private helpers solves is common.

I think there is something really wrong that the drivers are somehow tied to the mm_struct in some deep way.

The flow should have the SVA iommu_domain be created and associated with a mm_struct in the driver.

At this instant the iommu_domain should extract the top of page table from the mm_struct and from then on the driver never touches the mm_struct again. The top of page table is just like any other iommu_domain except it isn't mappable and it isn't allocated/freed.

The driver will establish a mmu_notifier in the driver's private iommu_domain space to capture invalidations.

Page fault is handled via common code

Any invalidations act exactly like any other invalidation - the driver goes through all the cache tags associated with the iommu_domain and cleans them.

What have I missed that requires more common code or tracking a list of pasid/mm?

PASID attachment and invalidation is general, and has nothing to do with SVA.

If the core code provies help for anything it should be to help the driver keep track of what cache tags are linked to which iommu_domain.

The comingling of SVA and PASID has made a big mess in the drivers we need to unwind.

Jason

#13 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

In reply to Jacob Pan
Quoted 26 lines · Jacob Pan

Hi Jean-Philippe,

Quoted 5 lines · Jean-Philippe Brucker
Quoted 4 lines · Jacob Pan

Custom allocator feature was introduced to support VT-d's virtual command, an enlightened interface designed for VMs to allocate PASIDs from the host.

Quoted 9 lines
Quoted 6 lines

As we remove/withdraw the VT-d virtual command feature, the sole user of custom allocator, we can safely remove the custom allocator as well. Effectively, this will return IOASID core to the original simple global namespace allocator.

You can also drop the spinlock.h include. With that:

good catch, thanks

Quoted 6 lines

On a related note, it looks like 100b8a14a370 ("iommu/vt-d: Add pasid private data helpers") removed the last user of ioasid_set_data(). I guess that could be dropped too, unless you plan to still use it?

Quoted 5 lines

You are right, will remove.
I was planning on the other way around which will convert VT-d's private pasid data helpers to common ioasid code, but when I look closer the private pasid xa is just holding a list of pasid/mm which could be per iommu not global. Another cleanup I suppose.

Please lets just delete this entire ioasid thing, it has no purpose anymore at all.

I did a sketch on how it do it here:

https://github.com/jgunthorpe/linux/commits/iommu_remove_ioasid

I wasn't very careful or elegant with the last patch, can you tidy it up and repost this as your v2?

Your DMA API PASID thing will simply need one new API to alloc/free a PASID from the iommu_global_pasid_ida

Thanks,
Jason

author
#14 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Hi Jason,

Quoted 36 lines · Jason Gunthorpe
Quoted 26 lines · Jacob Pan

Hi Jean-Philippe,

Quoted 5 lines · Jean-Philippe Brucker
Quoted 4 lines · Jacob Pan

Custom allocator feature was introduced to support VT-d's virtual command, an enlightened interface designed for VMs to allocate PASIDs from the host.

Quoted 9 lines
Quoted 6 lines

As we remove/withdraw the VT-d virtual command feature, the sole user of custom allocator, we can safely remove the custom allocator as well. Effectively, this will return IOASID core to the original simple global namespace allocator.

You can also drop the spinlock.h include. With that:

good catch, thanks

Quoted 6 lines

On a related note, it looks like 100b8a14a370 ("iommu/vt-d: Add pasid private data helpers") removed the last user of ioasid_set_data(). I guess that could be dropped too, unless you plan to still use it?

Quoted 5 lines

You are right, will remove.
I was planning on the other way around which will convert VT-d's private pasid data helpers to common ioasid code, but when I look closer the private pasid xa is just holding a list of pasid/mm which could be per iommu not global. Another cleanup I suppose.

Please lets just delete this entire ioasid thing, it has no purpose anymore at all.

Quoted 7 lines

I did a sketch on how it do it here:

https://github.com/jgunthorpe/linux/commits/iommu_remove_ioasid

I wasn't very careful or elegant with the last patch, can you tidy it up and repost this as your v2?

yes, will do.

Quoted 2 lines

Your DMA API PASID thing will simply need one new API to alloc/free a PASID from the iommu_global_pasid_ida

It should satisfy what we need right now.
Just wondering if we were to do resource management of global PASIDs, say with the new misc cgroup controller, do we plan to expand in iommu sva code? If yes, do we keep DMA API PASID in a separate range/set?

Thanks,

Jacob

#15 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 6 lines · Jacob Pan
Quoted 2 lines

Your DMA API PASID thing will simply need one new API to alloc/free a PASID from the iommu_global_pasid_ida

It should satisfy what we need right now.
Just wondering if we were to do resource management of global PASIDs, say with the new misc cgroup controller, do we plan to expand in iommu sva code? If yes, do we keep DMA API PASID in a separate range/set?

I would say all shared PASIDs held by userspace should be captured by by a resource limit, it doesn't matter if they are global PASIDs or device local shared PASIDs.

So if a cgroup comes it is just a matter of putting charges in the right place which is auditable by looking at calls to attach pasid functions.

Jason

author
#16 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Hi Jason,

Quoted 11 lines · Jason Gunthorpe
Quoted 6 lines · Jacob Pan
Quoted 2 lines

Your DMA API PASID thing will simply need one new API to alloc/free a PASID from the iommu_global_pasid_ida

It should satisfy what we need right now.
Just wondering if we were to do resource management of global PASIDs, say with the new misc cgroup controller, do we plan to expand in iommu sva code? If yes, do we keep DMA API PASID in a separate range/set?

I would say all shared PASIDs held by userspace should be captured by by a resource limit, it doesn't matter if they are global PASIDs or device local shared PASIDs.

agreed, I was just thinking in-kernel DMA PASID is not held by userspace, might be good to keep them in separate pool, thus keeping ioasid_set.

Quoted 3 lines

So if a cgroup comes it is just a matter of putting charges in the right place which is auditable by looking at calls to attach pasid functions.

shouldn't we charge cg during allocation? Or it might be too early for iommufd so we have to wait until attach?

Thanks,

Jacob

#17 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 12 lines · Jacob Pan

Hi Jason,

Quoted 8 lines · Jason Gunthorpe
Quoted 6 lines · Jacob Pan
Quoted 2 lines

Your DMA API PASID thing will simply need one new API to alloc/free a PASID from the iommu_global_pasid_ida

It should satisfy what we need right now.
Just wondering if we were to do resource management of global PASIDs, say with the new misc cgroup controller, do we plan to expand in iommu sva code? If yes, do we keep DMA API PASID in a separate range/set?

Quoted 5 lines
Quoted 3 lines

I would say all shared PASIDs held by userspace should be captured by by a resource limit, it doesn't matter if they are global PASIDs or device local shared PASIDs.

agreed, I was just thinking in-kernel DMA PASID is not held by userspace, might be good to keep them in separate pool, thus keeping ioasid_set.

Then you just don't charge a cgroup when you get these, you won't have a cgroup anyhow in that context.

The "ioasid_set" is really the xarray, and you don't have an option to use two xarrays with the same RID, so we definately wouldn't have two pools.

Quoted 5 lines
Quoted 3 lines

So if a cgroup comes it is just a matter of putting charges in the right place which is auditable by looking at calls to attach pasid functions.

shouldn't we charge cg during allocation? Or it might be too early for iommufd so we have to wait until attach?

We need to sort this all out. I would expect that iommufd will have an allocation API.

Jason

#18 lore raw

RE: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 12 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, February 14, 2023 7:46 AM

Quoted 5 lines
Quoted 3 lines

So if a cgroup comes it is just a matter of putting charges in the right place which is auditable by looking at calls to attach pasid functions.

shouldn't we charge cg during allocation? Or it might be too early for iommufd so we have to wait until attach?

We need to sort this all out. I would expect that iommufd will have an allocation API.

yes.

btw presumably iommu_global_pasid_ida should be move to iommu.c since it will be used beyond sva.

#19 lore raw

Re: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 9 lines · Kevin
Quoted 9 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, February 14, 2023 7:46 AM

Quoted 5 lines
Quoted 3 lines

So if a cgroup comes it is just a matter of putting charges in the right place which is auditable by looking at calls to attach pasid functions.

shouldn't we charge cg during allocation? Or it might be too early for iommufd so we have to wait until attach?

Quoted 8 lines
Quoted 3 lines

We need to sort this all out. I would expect that iommufd will have an allocation API.

yes.

btw presumably iommu_global_pasid_ida should be move to iommu.c since it will be used beyond sva.

Yeah, maybe, I'd think about moving it when we get patches to use it more than SVA.

Jason

#20 lore raw

RE: [PATCH 2/2] iommu/ioasid: Remove custom IOASID allocator

Quoted 17 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, February 14, 2023 10:56 PM

Quoted 13 lines · Kevin
Quoted 12 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, February 14, 2023 7:46 AM

Quoted 5 lines
Quoted 3 lines

So if a cgroup comes it is just a matter of putting charges in the right place which is auditable by looking at calls to attach pasid functions.

shouldn't we charge cg during allocation? Or it might be too early for iommufd so we have to wait until attach?

We need to sort this all out. I would expect that iommufd will have an allocation API.

Quoted 8 lines
Quoted 4 lines

yes.

btw presumably iommu_global_pasid_ida should be move to iommu.c since it will be used beyond sva.

Yeah, maybe, I'd think about moving it when we get patches to use it more than SVA.

that also works.

#21 lore raw
In reply to Jacob Pan
Quoted 11 lines

From: Jacob Pan <[email protected]>
Sent: Saturday, February 11, 2023 7:02 AM

Hi all,

This patch set removes unused VT-d virtual command interface followed by clean up in the IOASID code.

This has only been tested on x86 platforms, need help with testing on ARM SMMU and other architectures.

Quoted 9 lines

I also hope to collect feedback on the upcoming clean up and enhancements below:
1. Consolidate VT-d private SVA PASID array with IOASID common code
2. As we retain the global IOASID allocator, we need some level of resource management. I want to restart the effort to include IOASID under misc cgroup.
(https://lore.kernel.org/linux-iommu/20210303160205.151d114e@jacob-
builder/)

this should go with adding global pasid allocator to iommufd.

before that we don't have such cgroup requirement.