MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Hi all,
This patch set removes unused VT-d virtual command interface followed by
removal of the IOASID infrastructure.
This has only been tested on x86 platforms, need help with testing on ARM
SMMU and other architectures.
Thanks,
Jacob
ChangeLog:
v4:
- keep mm_pasid helpers inline as much as we can for fork performance
- separate GFP_ATOMIC to GFP_KERNEL change for bisectability
v3:
- moved helper functions for PASID under SVA code, avoided circular inclusion
between mm.h and iommu.h
- deleted makefiles
- put rename under a different patch
Jacob Pan (4):
iommu/vt-d: Remove virtual command interface
iommu/sva: Move PASID helpers to sva code
iommu/sva: Use GFP_KERNEL for pasid allocation
iommu/ioasid: Rename INVALID_IOASID
Jason Gunthorpe (2):
iommu/sva: Stop using ioasid_set for SVA
iommu: Remove ioasid infrastructure
Documentation/x86/sva.rst | 2 +-
arch/x86/kernel/traps.c | 5 +-
drivers/dma/idxd/device.c | 8 +-
drivers/dma/idxd/idxd.h | 2 +-
drivers/dma/idxd/init.c | 2 +-
drivers/dma/idxd/irq.c | 2 +-
drivers/iommu/Kconfig | 5 -
drivers/iommu/Makefile | 1 -
drivers/iommu/intel/cap_audit.c | 2 -
drivers/iommu/intel/dmar.c | 6 +-
drivers/iommu/intel/iommu.c | 87 +------
drivers/iommu/intel/iommu.h | 9 -
drivers/iommu/intel/svm.c | 3 +-
drivers/iommu/ioasid.c | 422 --------------------------------
drivers/iommu/iommu-sva.c | 62 ++---
drivers/iommu/iommu-sva.h | 4 -
include/linux/ioasid.h | 83 -------
include/linux/iommu-helper.h | 1 +
include/linux/iommu.h | 8 +-
include/linux/sched/mm.h | 20 +-
mm/init-mm.c | 4 +-
21 files changed, 46 insertions(+), 692 deletions(-)
delete mode 100644 drivers/iommu/ioasid.c
delete mode 100644 include/linux/ioasid.h
--
2.25.1
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.
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jacob Pan <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Lu Baolu <[email protected]>
---
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(-)
diff --git a/drivers/iommu/intel/cap_audit.c b/drivers/iommu/intel/cap_audit.c
index 806986696841..9862dc20b35e 100644
--- a/drivers/iommu/intel/cap_audit.c
+++ b/drivers/iommu/intel/cap_audit.c
@@ -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);
diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index b00a0ceb2d13..bf0bfe5ba7a7 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -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),
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 59df7e42fd53..a295e80fdfe8 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1721,9 +1721,6 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
if (ecap_prs(iommu->ecap))
intel_svm_finish_prq(iommu);
}
- if (vccap_pasid(iommu->vccap))
- ioasid_unregister_allocator(&iommu->pasid_allocator);
-
#endif
}
@@ -2793,85 +2790,6 @@ static int copy_translation_tables(struct intel_iommu *iommu)
return ret;
}
-#ifdef CONFIG_INTEL_IOMMU_SVM
-static ioasid_t intel_vcmd_ioasid_alloc(ioasid_t min, ioasid_t max, void *data)
-{
- struct intel_iommu *iommu = data;
- ioasid_t ioasid;
-
- if (!iommu)
- return INVALID_IOASID;
- /*
- * VT-d virtual command interface always uses the full 20 bit
- * PASID range. Host can partition guest PASID range based on
- * policies but it is out of guest's control.
- */
- if (min < PASID_MIN || max > intel_pasid_max_id)
- return INVALID_IOASID;
-
- if (vcmd_alloc_pasid(iommu, &ioasid))
- return INVALID_IOASID;
-
- return ioasid;
-}
-
-static void intel_vcmd_ioasid_free(ioasid_t ioasid, void *data)
-{
- struct intel_iommu *iommu = data;
-
- if (!iommu)
- return;
- /*
- * Sanity check the ioasid owner is done at upper layer, e.g. VFIO
- * We can only free the PASID when all the devices are unbound.
- */
- if (ioasid_find(NULL, ioasid, NULL)) {
- pr_alert("Cannot free active IOASID %d\n", ioasid);
- return;
- }
- vcmd_free_pasid(iommu, ioasid);
-}
-
-static void register_pasid_allocator(struct intel_iommu *iommu)
-{
- /*
- * If we are running in the host, no need for custom allocator
- * in that PASIDs are allocated from the host system-wide.
- */
- if (!cap_caching_mode(iommu->cap))
- return;
-
- if (!sm_supported(iommu)) {
- pr_warn("VT-d Scalable Mode not enabled, no PASID allocation\n");
- return;
- }
-
- /*
- * Register a custom PASID allocator if we are running in a guest,
- * guest PASID must be obtained via virtual command interface.
- * There can be multiple vIOMMUs in each guest but only one allocator
- * is active. All vIOMMU allocators will eventually be calling the same
- * host allocator.
- */
- if (!vccap_pasid(iommu->vccap))
- return;
-
- pr_info("Register custom PASID allocator\n");
- iommu->pasid_allocator.alloc = intel_vcmd_ioasid_alloc;
- iommu->pasid_allocator.free = intel_vcmd_ioasid_free;
- iommu->pasid_allocator.pdata = (void *)iommu;
- if (ioasid_register_allocator(&iommu->pasid_allocator)) {
- pr_warn("Custom PASID allocator failed, scalable mode disabled\n");
- /*
- * Disable scalable mode on this IOMMU if there
- * is no custom allocator. Mixing SM capable vIOMMU
- * and non-SM vIOMMU are not supported.
- */
- intel_iommu_sm = 0;
- }
-}
-#endif
-
static int __init init_dmars(void)
{
struct dmar_drhd_unit *drhd;
@@ -2960,9 +2878,6 @@ static int __init init_dmars(void)
*/
for_each_active_iommu(iommu, drhd) {
iommu_flush_write_buffer(iommu);
-#ifdef CONFIG_INTEL_IOMMU_SVM
- register_pasid_allocator(iommu);
-#endif
iommu_set_root_entry(iommu);
}
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 06e61e474856..6bdfbead82c4 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -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];
--
2.25.1
Preparing to remove IOASID infrastructure, PASID management will be
under SVA code. Decouple mm code from IOASID. Use iommu-help.h instead
of iommu.h to prevent circular inclusion.
Signed-off-by: Jacob Pan <[email protected]>
---
v4: (Jason's comments)
- delete and open code mm_set_pasid
- keep mm_init_pasid() as inline for fork performance
---
drivers/iommu/iommu-sva.c | 10 +++++++++-
include/linux/ioasid.h | 2 +-
include/linux/iommu-helper.h | 1 +
include/linux/sched/mm.h | 18 ++----------------
4 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 24bf9b2b58aa..376b2a9e2543 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -44,7 +44,7 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
if (!pasid_valid(pasid))
ret = -ENOMEM;
else
- mm_pasid_set(mm, pasid);
+ mm->pasid = ret;
out:
mutex_unlock(&iommu_sva_lock);
return ret;
@@ -238,3 +238,11 @@ iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
return status;
}
+
+void mm_pasid_drop(struct mm_struct *mm)
+{
+ if (pasid_valid(mm->pasid)) {
+ ioasid_free(mm->pasid);
+ mm->pasid = INVALID_IOASID;
+ }
+}
diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
index af1c9d62e642..2c502e77ee78 100644
--- a/include/linux/ioasid.h
+++ b/include/linux/ioasid.h
@@ -4,8 +4,8 @@
#include <linux/types.h>
#include <linux/errno.h>
+#include <linux/iommu-helper.h>
-#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);
diff --git a/include/linux/iommu-helper.h b/include/linux/iommu-helper.h
index 74be34f3a20a..0aa922f6bfad 100644
--- a/include/linux/iommu-helper.h
+++ b/include/linux/iommu-helper.h
@@ -40,5 +40,6 @@ static inline unsigned long iommu_num_pages(unsigned long addr,
return DIV_ROUND_UP(size, io_page_size);
}
+#define INVALID_IOASID (-1U)
#endif
diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 2a243616f222..ae5a3f16b321 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -8,7 +8,7 @@
#include <linux/mm_types.h>
#include <linux/gfp.h>
#include <linux/sync_core.h>
-#include <linux/ioasid.h>
+#include <linux/iommu-helper.h>
/*
* Routines for handling mm_structs
@@ -456,23 +456,9 @@ static inline void mm_pasid_init(struct mm_struct *mm)
{
mm->pasid = INVALID_IOASID;
}
-
-/* Associate a PASID with an mm_struct: */
-static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid)
-{
- mm->pasid = pasid;
-}
-
-static inline void mm_pasid_drop(struct mm_struct *mm)
-{
- if (pasid_valid(mm->pasid)) {
- ioasid_free(mm->pasid);
- mm->pasid = INVALID_IOASID;
- }
-}
+void mm_pasid_drop(struct mm_struct *mm);
#else
static inline void mm_pasid_init(struct mm_struct *mm) {}
-static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid) {}
static inline void mm_pasid_drop(struct mm_struct *mm) {}
#endif
--
2.25.1
We’re not using spinlock-protected IOASID allocation anymore, there’s
no need for GFP_ATOMIC.
Signed-off-by: Jacob Pan <[email protected]>
---
drivers/iommu/iommu-sva.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 297852ae5e7c..8c92a145e15d 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -29,7 +29,7 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
goto out;
}
- ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_ATOMIC);
+ ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
if (ret < min)
goto out;
mm->pasid = ret;
--
2.25.1
From: Jason Gunthorpe <[email protected]>
Instead SVA drivers can use a simple global IDA to allocate PASIDs for
each mm_struct.
Future work would be to allow drivers using the SVA APIs to reserve global
PASIDs from this IDA for their internal use, eg with the DMA API PASID
support.
Signed-off-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
---
v4:
- Keep GFP_ATOMIC flag for PASID allocation, will changed to
GFP_KERNEL in a separate patch.
---
drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
drivers/iommu/iommu-sva.h | 3 --
2 files changed, 15 insertions(+), 50 deletions(-)
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 376b2a9e2543..297852ae5e7c 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -9,26 +9,13 @@
#include "iommu-sva.h"
static DEFINE_MUTEX(iommu_sva_lock);
-static DECLARE_IOASID_SET(iommu_sva_pasid);
+static DEFINE_IDA(iommu_global_pasid_ida);
-/**
- * iommu_sva_alloc_pasid - Allocate a PASID for the mm
- * @mm: the mm
- * @min: minimum PASID value (inclusive)
- * @max: maximum PASID value (inclusive)
- *
- * Try to allocate a PASID for this mm, or take a reference to the existing one
- * provided it fits within the [@min, @max] range. On success the PASID is
- * available in mm->pasid and will be available for the lifetime of the mm.
- *
- * Returns 0 on success and < 0 on error.
- */
-int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
+static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
{
- int ret = 0;
- ioasid_t pasid;
+ int ret;
- if (min == INVALID_IOASID || max == INVALID_IOASID ||
+ if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
min == 0 || max < min)
return -EINVAL;
@@ -37,39 +24,20 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
if (pasid_valid(mm->pasid)) {
if (mm->pasid < min || mm->pasid >= max)
ret = -EOVERFLOW;
+ else
+ ret = 0;
goto out;
}
- pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
- if (!pasid_valid(pasid))
- ret = -ENOMEM;
- else
- mm->pasid = ret;
+ ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_ATOMIC);
+ if (ret < min)
+ goto out;
+ mm->pasid = ret;
+ ret = 0;
out:
mutex_unlock(&iommu_sva_lock);
return ret;
}
-EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
-
-/* ioasid_find getter() requires a void * argument */
-static bool __mmget_not_zero(void *mm)
-{
- return mmget_not_zero(mm);
-}
-
-/**
- * iommu_sva_find() - Find mm associated to the given PASID
- * @pasid: Process Address Space ID assigned to the mm
- *
- * On success a reference to the mm is taken, and must be released with mmput().
- *
- * Returns the mm corresponding to this PASID, or an error if not found.
- */
-struct mm_struct *iommu_sva_find(ioasid_t pasid)
-{
- return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_find);
/**
* iommu_sva_bind_device() - Bind a process address space to a device
@@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
void mm_pasid_drop(struct mm_struct *mm)
{
- if (pasid_valid(mm->pasid)) {
- ioasid_free(mm->pasid);
- mm->pasid = INVALID_IOASID;
- }
+ if (likely(!pasid_valid(mm->pasid)))
+ return;
+
+ ida_free(&iommu_global_pasid_ida, mm->pasid);
}
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
index 7215a761b962..c22d0174ad61 100644
--- a/drivers/iommu/iommu-sva.h
+++ b/drivers/iommu/iommu-sva.h
@@ -8,9 +8,6 @@
#include <linux/ioasid.h>
#include <linux/mm_types.h>
-int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
-struct mm_struct *iommu_sva_find(ioasid_t pasid);
-
/* I/O Page fault */
struct device;
struct iommu_fault;
--
2.25.1
INVALID_IOASID and IOMMU_PASID_INVALID are duplicated. Rename
INVALID_IOASID and consolidate since we are moving away from IOASID
infrastructure.
Reviewed-by: Dave Jiang <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
---
Documentation/x86/sva.rst | 2 +-
arch/x86/kernel/traps.c | 5 ++++-
drivers/dma/idxd/device.c | 8 ++++----
drivers/dma/idxd/idxd.h | 1 +
drivers/dma/idxd/init.c | 2 +-
drivers/dma/idxd/irq.c | 2 +-
drivers/iommu/intel/dmar.c | 4 ++--
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/intel/svm.c | 2 +-
include/linux/ioasid.h | 5 +----
include/linux/iommu-helper.h | 2 +-
include/linux/iommu.h | 7 +++++--
include/linux/sched/mm.h | 2 +-
mm/init-mm.c | 4 ++--
14 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/Documentation/x86/sva.rst b/Documentation/x86/sva.rst
index 2e9b8b0f9a0f..33cb05005982 100644
--- a/Documentation/x86/sva.rst
+++ b/Documentation/x86/sva.rst
@@ -107,7 +107,7 @@ process share the same page tables, thus the same MSR value.
PASID Life Cycle Management
===========================
-PASID is initialized as INVALID_IOASID (-1) when a process is created.
+PASID is initialized as IOMMU_PASID_INVALID (-1) when a process is created.
Only processes that access SVA-capable devices need to have a PASID
allocated. This allocation happens when a process opens/binds an SVA-capable
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index d317dc3d06a3..d6fb03ebf548 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -40,7 +40,10 @@
#include <linux/io.h>
#include <linux/hardirq.h>
#include <linux/atomic.h>
-#include <linux/ioasid.h>
+
+#ifdef CONFIG_IOMMU_SVA
+#include <linux/iommu.h>
+#endif
#include <asm/stacktrace.h>
#include <asm/processor.h>
diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 29dbb0f52e18..125652a8bb29 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1194,7 +1194,7 @@ static void idxd_device_set_perm_entry(struct idxd_device *idxd,
{
union msix_perm mperm;
- if (ie->pasid == INVALID_IOASID)
+ if (ie->pasid == IOMMU_PASID_INVALID)
return;
mperm.bits = 0;
@@ -1224,7 +1224,7 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
idxd_device_clear_perm_entry(idxd, ie);
ie->vector = -1;
ie->int_handle = INVALID_INT_HANDLE;
- ie->pasid = INVALID_IOASID;
+ ie->pasid = IOMMU_PASID_INVALID;
}
int idxd_wq_request_irq(struct idxd_wq *wq)
@@ -1240,7 +1240,7 @@ int idxd_wq_request_irq(struct idxd_wq *wq)
ie = &wq->ie;
ie->vector = pci_irq_vector(pdev, ie->id);
- ie->pasid = device_pasid_enabled(idxd) ? idxd->pasid : INVALID_IOASID;
+ ie->pasid = device_pasid_enabled(idxd) ? idxd->pasid : IOMMU_PASID_INVALID;
idxd_device_set_perm_entry(idxd, ie);
rc = request_threaded_irq(ie->vector, NULL, idxd_wq_thread, 0, "idxd-portal", ie);
@@ -1265,7 +1265,7 @@ int idxd_wq_request_irq(struct idxd_wq *wq)
free_irq(ie->vector, ie);
err_irq:
idxd_device_clear_perm_entry(idxd, ie);
- ie->pasid = INVALID_IOASID;
+ ie->pasid = IOMMU_PASID_INVALID;
return rc;
}
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 7ced8d283d98..417e602a46b6 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -13,6 +13,7 @@
#include <linux/ioasid.h>
#include <linux/bitmap.h>
#include <linux/perf_event.h>
+#include <linux/iommu.h>
#include <uapi/linux/idxd.h>
#include "registers.h"
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 529ea09c9094..f30eef701970 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -105,7 +105,7 @@ static int idxd_setup_interrupts(struct idxd_device *idxd)
ie = idxd_get_ie(idxd, msix_idx);
ie->id = msix_idx;
ie->int_handle = INVALID_INT_HANDLE;
- ie->pasid = INVALID_IOASID;
+ ie->pasid = IOMMU_PASID_INVALID;
spin_lock_init(&ie->list_lock);
init_llist_head(&ie->pending_llist);
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index aa314ebec587..242f1f0b9f09 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -80,7 +80,7 @@ static void idxd_int_handle_revoke_drain(struct idxd_irq_entry *ie)
desc.opcode = DSA_OPCODE_DRAIN;
desc.priv = 1;
- if (ie->pasid != INVALID_IOASID)
+ if (ie->pasid != IOMMU_PASID_INVALID)
desc.pasid = ie->pasid;
desc.int_handle = ie->int_handle;
portal = idxd_wq_portal_addr(wq);
diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index bf0bfe5ba7a7..c567f94b66c7 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1933,7 +1933,7 @@ static int dmar_fault_do_one(struct intel_iommu *iommu, int type,
return 0;
}
- if (pasid == INVALID_IOASID)
+ if (pasid == IOMMU_PASID_INVALID)
pr_err("[%s NO_PASID] Request device [%02x:%02x.%d] fault addr 0x%llx [fault reason 0x%02x] %s\n",
type ? "DMA Read" : "DMA Write",
source_id >> 8, PCI_SLOT(source_id & 0xFF),
@@ -2014,7 +2014,7 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
if (!ratelimited)
/* Using pasid -1 if pasid is not present */
dmar_fault_do_one(iommu, type, fault_reason,
- pasid_present ? pasid : INVALID_IOASID,
+ pasid_present ? pasid : IOMMU_PASID_INVALID,
source_id, guest_addr);
fault_index++;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index a295e80fdfe8..10f657828d3a 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -876,7 +876,7 @@ void dmar_fault_dump_ptes(struct intel_iommu *iommu, u16 source_id,
return;
}
/* For request-without-pasid, get the pasid from context entry */
- if (intel_iommu_sm && pasid == INVALID_IOASID)
+ if (intel_iommu_sm && pasid == IOMMU_PASID_INVALID)
pasid = PASID_RID2PASID;
dir_index = pasid >> PASID_PDE_SHIFT;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index c76b66263467..be98af2fce06 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -274,7 +274,7 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
if (WARN_ON(!mutex_is_locked(&pasid_mutex)))
return -EINVAL;
- if (pasid == INVALID_IOASID || pasid >= PASID_MAX)
+ if (pasid == IOMMU_PASID_INVALID || pasid >= PASID_MAX)
return -EINVAL;
svm = pasid_private_find(pasid);
diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
index 2c502e77ee78..665a3773a409 100644
--- a/include/linux/ioasid.h
+++ b/include/linux/ioasid.h
@@ -7,6 +7,7 @@
#include <linux/iommu-helper.h>
typedef unsigned int ioasid_t;
+#define INVALID_IOASID ((ioasid_t)-1)
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);
@@ -40,10 +41,6 @@ void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
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)
-{
- return ioasid != INVALID_IOASID;
-}
#else /* !CONFIG_IOASID */
static inline ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min,
diff --git a/include/linux/iommu-helper.h b/include/linux/iommu-helper.h
index 0aa922f6bfad..0147430d8a6c 100644
--- a/include/linux/iommu-helper.h
+++ b/include/linux/iommu-helper.h
@@ -40,6 +40,6 @@ static inline unsigned long iommu_num_pages(unsigned long addr,
return DIV_ROUND_UP(size, io_page_size);
}
-#define INVALID_IOASID (-1U)
+#define IOMMU_PASID_INVALID (-1U)
#endif
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 46e1347bfa22..a694f6256d2d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -192,8 +192,11 @@ enum iommu_dev_features {
IOMMU_DEV_FEAT_IOPF,
};
-#define IOMMU_PASID_INVALID (-1U)
-
+typedef unsigned int ioasid_t;
+static inline bool pasid_valid(ioasid_t ioasid)
+{
+ return ioasid != IOMMU_PASID_INVALID;
+}
#ifdef CONFIG_IOMMU_API
/**
diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index ae5a3f16b321..0e0aa285a12f 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -454,7 +454,7 @@ static inline void membarrier_update_current_mm(struct mm_struct *next_mm)
#ifdef CONFIG_IOMMU_SVA
static inline void mm_pasid_init(struct mm_struct *mm)
{
- mm->pasid = INVALID_IOASID;
+ mm->pasid = IOMMU_PASID_INVALID;
}
void mm_pasid_drop(struct mm_struct *mm);
#else
diff --git a/mm/init-mm.c b/mm/init-mm.c
index c9327abb771c..a084039f55d8 100644
--- a/mm/init-mm.c
+++ b/mm/init-mm.c
@@ -10,7 +10,7 @@
#include <linux/atomic.h>
#include <linux/user_namespace.h>
-#include <linux/ioasid.h>
+#include <linux/iommu.h>
#include <asm/mmu.h>
#ifndef INIT_MM_CONTEXT
@@ -40,7 +40,7 @@ struct mm_struct init_mm = {
.user_ns = &init_user_ns,
.cpu_bitmap = CPU_BITS_NONE,
#ifdef CONFIG_IOMMU_SVA
- .pasid = INVALID_IOASID,
+ .pasid = IOMMU_PASID_INVALID,
#endif
INIT_MM_CONTEXT(init_mm)
};
--
2.25.1
From: Jason Gunthorpe <[email protected]>
This has no use anymore, delete it all.
Signed-off-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
---
v3: - put rename under a different patch
- delete makefile and Kconfig
v2:
- fix compile issue w/o CONFIG_IOMMU_SVA
- consolidate INVALID_IOASID w/ IOMMU_PASID_INVALID
---
drivers/dma/idxd/idxd.h | 1 -
drivers/iommu/Kconfig | 5 -
drivers/iommu/Makefile | 1 -
drivers/iommu/intel/iommu.h | 1 -
drivers/iommu/intel/svm.c | 1 -
drivers/iommu/ioasid.c | 422 ------------------------------------
drivers/iommu/iommu-sva.h | 1 -
include/linux/ioasid.h | 80 -------
include/linux/iommu.h | 1 -
9 files changed, 513 deletions(-)
delete mode 100644 drivers/iommu/ioasid.c
delete mode 100644 include/linux/ioasid.h
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 417e602a46b6..dd2a6ed8949b 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -10,7 +10,6 @@
#include <linux/cdev.h>
#include <linux/idr.h>
#include <linux/pci.h>
-#include <linux/ioasid.h>
#include <linux/bitmap.h>
#include <linux/perf_event.h>
#include <linux/iommu.h>
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 79707685d54a..06ddbbfec3bf 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -3,10 +3,6 @@
config IOMMU_IOVA
tristate
-# The IOASID library may also be used by non-IOMMU_API users
-config IOASID
- tristate
-
# IOMMU_API always gets selected by whoever wants it.
config IOMMU_API
bool
@@ -158,7 +154,6 @@ config IOMMU_DMA
# Shared Virtual Addressing
config IOMMU_SVA
bool
- select IOASID
config FSL_PAMU
bool "Freescale IOMMU support"
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index f461d0651385..769e43d780ce 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -9,7 +9,6 @@ obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
obj-$(CONFIG_IOMMU_IO_PGTABLE_DART) += io-pgtable-dart.o
-obj-$(CONFIG_IOASID) += ioasid.o
obj-$(CONFIG_IOMMU_IOVA) += iova.o
obj-$(CONFIG_OF_IOMMU) += of_iommu.o
obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 6bdfbead82c4..80582e497782 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -19,7 +19,6 @@
#include <linux/iommu.h>
#include <linux/io-64-nonatomic-lo-hi.h>
#include <linux/dmar.h>
-#include <linux/ioasid.h>
#include <linux/bitfield.h>
#include <linux/xarray.h>
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index be98af2fce06..b4efc541f6b3 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -17,7 +17,6 @@
#include <linux/interrupt.h>
#include <linux/mm_types.h>
#include <linux/xarray.h>
-#include <linux/ioasid.h>
#include <asm/page.h>
#include <asm/fpu/api.h>
diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
deleted file mode 100644
index a786c034907c..000000000000
--- a/drivers/iommu/ioasid.c
+++ /dev/null
@@ -1,422 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * I/O Address Space ID allocator. There is one global IOASID space, split into
- * subsets. Users create a subset with DECLARE_IOASID_SET, then allocate and
- * free IOASIDs with ioasid_alloc() and ioasid_free().
- */
-#include <linux/ioasid.h>
-#include <linux/module.h>
-#include <linux/slab.h>
-#include <linux/spinlock.h>
-#include <linux/xarray.h>
-
-struct ioasid_data {
- ioasid_t id;
- struct ioasid_set *set;
- 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);
-
-/**
- * ioasid_set_data - Set private data for an allocated ioasid
- * @ioasid: the ID to set data
- * @data: the private data
- *
- * For IOASID that is already allocated, private data can be set
- * via this API. Future lookup can be done via ioasid_find.
- */
-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);
- if (ioasid_data)
- rcu_assign_pointer(ioasid_data->private, data);
- else
- ret = -ENOENT;
- spin_unlock(&ioasid_allocator_lock);
-
- /*
- * Wait for readers to stop accessing the old private data, so the
- * caller can free it.
- */
- if (!ret)
- synchronize_rcu();
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(ioasid_set_data);
-
-/**
- * ioasid_alloc - Allocate an IOASID
- * @set: the IOASID set
- * @min: the minimum ID (inclusive)
- * @max: the maximum ID (inclusive)
- * @private: data private to the caller
- *
- * Allocate an ID between @min and @max. The @private pointer is stored
- * internally and can be retrieved with ioasid_find().
- *
- * Return: the allocated ID on success, or %INVALID_IOASID on failure.
- */
-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);
- if (!data)
- return INVALID_IOASID;
-
- 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);
- 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;
-}
-EXPORT_SYMBOL_GPL(ioasid_alloc);
-
-/**
- * ioasid_free - Free an ioasid
- * @ioasid: the ID to remove
- */
-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);
-}
-EXPORT_SYMBOL_GPL(ioasid_free);
-
-/**
- * ioasid_find - Find IOASID data
- * @set: the IOASID set
- * @ioasid: the IOASID to find
- * @getter: function to call on the found object
- *
- * The optional getter function allows to take a reference to the found object
- * under the rcu lock. The function can also check if the object is still valid:
- * if @getter returns false, then the object is invalid and NULL is returned.
- *
- * If the IOASID exists, return the private pointer passed to ioasid_alloc.
- * Private data can be NULL if not set. Return an error if the IOASID is not
- * found, or if @set is not NULL and the IOASID does not belong to the set.
- */
-void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
- bool (*getter)(void *))
-{
- 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);
- if (!ioasid_data) {
- priv = ERR_PTR(-ENOENT);
- goto unlock;
- }
- if (set && ioasid_data->set != set) {
- /* data found but does not belong to the set */
- priv = ERR_PTR(-EACCES);
- goto unlock;
- }
- /* Now IOASID and its set is verified, we can return the private data */
- priv = rcu_dereference(ioasid_data->private);
- if (getter && !getter(priv))
- priv = NULL;
-unlock:
- rcu_read_unlock();
-
- return priv;
-}
-EXPORT_SYMBOL_GPL(ioasid_find);
-
-MODULE_AUTHOR("Jean-Philippe Brucker <[email protected]>");
-MODULE_AUTHOR("Jacob Pan <[email protected]>");
-MODULE_DESCRIPTION("IO Address Space ID (IOASID) allocator");
-MODULE_LICENSE("GPL");
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
index c22d0174ad61..54946b5a7caf 100644
--- a/drivers/iommu/iommu-sva.h
+++ b/drivers/iommu/iommu-sva.h
@@ -5,7 +5,6 @@
#ifndef _IOMMU_SVA_H
#define _IOMMU_SVA_H
-#include <linux/ioasid.h>
#include <linux/mm_types.h>
/* I/O Page fault */
diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
deleted file mode 100644
index 665a3773a409..000000000000
--- a/include/linux/ioasid.h
+++ /dev/null
@@ -1,80 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __LINUX_IOASID_H
-#define __LINUX_IOASID_H
-
-#include <linux/types.h>
-#include <linux/errno.h>
-#include <linux/iommu-helper.h>
-
-typedef unsigned int ioasid_t;
-#define INVALID_IOASID ((ioasid_t)-1)
-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)
-ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min, ioasid_t max,
- void *private);
-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);
-
-#else /* !CONFIG_IOASID */
-static inline ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min,
- ioasid_t max, void *private)
-{
- return INVALID_IOASID;
-}
-
-static inline void ioasid_free(ioasid_t ioasid) { }
-
-static inline void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
- bool (*getter)(void *))
-{
- 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;
-}
-
-static inline bool pasid_valid(ioasid_t ioasid)
-{
- return false;
-}
-
-#endif /* CONFIG_IOASID */
-#endif /* __LINUX_IOASID_H */
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a694f6256d2d..39a97bd8f04a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -13,7 +13,6 @@
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/of.h>
-#include <linux/ioasid.h>
#include <uapi/linux/iommu.h>
#define IOMMU_READ (1 << 0)
--
2.25.1
On 3/1/23 15:56, Jacob Pan wrote:
> INVALID_IOASID and IOMMU_PASID_INVALID are duplicated. Rename
> INVALID_IOASID and consolidate since we are moving away from IOASID
> infrastructure.
>
> Reviewed-by: Dave Jiang <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
Reviewed-by: Fenghua Yu <[email protected]>
Thanks.
-Fenghua
> From: Jacob Pan <[email protected]>
> Sent: Thursday, March 2, 2023 7:57 AM
>
>
> - if (min == INVALID_IOASID || max == INVALID_IOASID ||
> + if (min == IOMMU_PASID_INVALID || max ==
> IOMMU_PASID_INVALID ||
> min == 0 || max < min)
> return -EINVAL;
>
if (!pasid_valid(min) || !pasid_valid(max) || ...)
with that,
Reviewed-by: Kevin Tian <[email protected]>
> From: Jacob Pan <[email protected]>
> Sent: Thursday, March 2, 2023 7:57 AM
>
> We’re not using spinlock-protected IOASID allocation anymore, there’s
> no need for GFP_ATOMIC.
>
> Signed-off-by: Jacob Pan <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
> From: Jacob Pan <[email protected]>
> Sent: Thursday, March 2, 2023 7:57 AM
>
> @@ -1194,7 +1194,7 @@ static void idxd_device_set_perm_entry(struct
> idxd_device *idxd,
> {
> union msix_perm mperm;
>
> - if (ie->pasid == INVALID_IOASID)
> + if (ie->pasid == IOMMU_PASID_INVALID)
> return;
>
if (!pasid_valid(ie->pasid))
same for other occurrences in this patch.
Reviewed-by: Kevin Tian <[email protected]>
> From: Jacob Pan <[email protected]>
> Sent: Thursday, March 2, 2023 7:57 AM
>
> From: Jason Gunthorpe <[email protected]>
>
> This has no use anymore, delete it all.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
> From: Jacob Pan <[email protected]>
> Sent: Thursday, March 2, 2023 7:57 AM
>
> -static inline void mm_pasid_drop(struct mm_struct *mm)
> -{
> - if (pasid_valid(mm->pasid)) {
> - ioasid_free(mm->pasid);
> - mm->pasid = INVALID_IOASID;
> - }
> -}
> +void mm_pasid_drop(struct mm_struct *mm);
Is it good to have a function declared in a header file of one
subsystem while being implemented in another subsystem?
On 2023/3/2 7:56, Jacob Pan wrote:
> Preparing to remove IOASID infrastructure, PASID management will be
> under SVA code. Decouple mm code from IOASID. Use iommu-help.h instead
> of iommu.h to prevent circular inclusion.
>
> Signed-off-by: Jacob Pan <[email protected]>
> ---
> v4: (Jason's comments)
> - delete and open code mm_set_pasid
> - keep mm_init_pasid() as inline for fork performance
> ---
> drivers/iommu/iommu-sva.c | 10 +++++++++-
> include/linux/ioasid.h | 2 +-
> include/linux/iommu-helper.h | 1 +
> include/linux/sched/mm.h | 18 ++----------------
> 4 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 24bf9b2b58aa..376b2a9e2543 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -44,7 +44,7 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> if (!pasid_valid(pasid))
> ret = -ENOMEM;
> else
> - mm_pasid_set(mm, pasid);
> + mm->pasid = ret;
This seems obviously incorrect.
mm->pasid = pasid;
?
> out:
> mutex_unlock(&iommu_sva_lock);
> return ret;
> @@ -238,3 +238,11 @@ iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
>
> return status;
> }
> +
> +void mm_pasid_drop(struct mm_struct *mm)
> +{
> + if (pasid_valid(mm->pasid)) {
> + ioasid_free(mm->pasid);
> + mm->pasid = INVALID_IOASID;
> + }
> +}
> diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
> index af1c9d62e642..2c502e77ee78 100644
> --- a/include/linux/ioasid.h
> +++ b/include/linux/ioasid.h
> @@ -4,8 +4,8 @@
>
> #include <linux/types.h>
> #include <linux/errno.h>
> +#include <linux/iommu-helper.h>
>
> -#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);
> diff --git a/include/linux/iommu-helper.h b/include/linux/iommu-helper.h
> index 74be34f3a20a..0aa922f6bfad 100644
> --- a/include/linux/iommu-helper.h
> +++ b/include/linux/iommu-helper.h
> @@ -40,5 +40,6 @@ static inline unsigned long iommu_num_pages(unsigned long addr,
>
> return DIV_ROUND_UP(size, io_page_size);
> }
> +#define INVALID_IOASID (-1U)
>
> #endif
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index 2a243616f222..ae5a3f16b321 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -8,7 +8,7 @@
> #include <linux/mm_types.h>
> #include <linux/gfp.h>
> #include <linux/sync_core.h>
> -#include <linux/ioasid.h>
> +#include <linux/iommu-helper.h>
>
> /*
> * Routines for handling mm_structs
> @@ -456,23 +456,9 @@ static inline void mm_pasid_init(struct mm_struct *mm)
> {
> mm->pasid = INVALID_IOASID;
> }
> -
> -/* Associate a PASID with an mm_struct: */
> -static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid)
> -{
> - mm->pasid = pasid;
> -}
> -
> -static inline void mm_pasid_drop(struct mm_struct *mm)
> -{
> - if (pasid_valid(mm->pasid)) {
> - ioasid_free(mm->pasid);
> - mm->pasid = INVALID_IOASID;
> - }
> -}
> +void mm_pasid_drop(struct mm_struct *mm);
> #else
> static inline void mm_pasid_init(struct mm_struct *mm) {}
> -static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid) {}
> static inline void mm_pasid_drop(struct mm_struct *mm) {}
Above mm_pasid_drop() should also be removed.
> #endif
>
Best regards,
baolu
On 2023/3/2 7:56, Jacob Pan wrote:
> From: Jason Gunthorpe <[email protected]>
>
> Instead SVA drivers can use a simple global IDA to allocate PASIDs for
> each mm_struct.
>
> Future work would be to allow drivers using the SVA APIs to reserve global
> PASIDs from this IDA for their internal use, eg with the DMA API PASID
> support.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
> ---
> v4:
> - Keep GFP_ATOMIC flag for PASID allocation, will changed to
> GFP_KERNEL in a separate patch.
> ---
> drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
> drivers/iommu/iommu-sva.h | 3 --
> 2 files changed, 15 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 376b2a9e2543..297852ae5e7c 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -9,26 +9,13 @@
> #include "iommu-sva.h"
>
> static DEFINE_MUTEX(iommu_sva_lock);
> -static DECLARE_IOASID_SET(iommu_sva_pasid);
> +static DEFINE_IDA(iommu_global_pasid_ida);
>
> -/**
> - * iommu_sva_alloc_pasid - Allocate a PASID for the mm
> - * @mm: the mm
> - * @min: minimum PASID value (inclusive)
> - * @max: maximum PASID value (inclusive)
> - *
> - * Try to allocate a PASID for this mm, or take a reference to the existing one
> - * provided it fits within the [@min, @max] range. On success the PASID is
> - * available in mm->pasid and will be available for the lifetime of the mm.
> - *
> - * Returns 0 on success and < 0 on error.
> - */
> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> +static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> {
> - int ret = 0;
> - ioasid_t pasid;
> + int ret;
>
> - if (min == INVALID_IOASID || max == INVALID_IOASID ||
> + if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
> min == 0 || max < min)
It's irrelevant to this patch. Just out of curiosity, why do we need to
exclude PASID 0 here? I just had a quick look at PCI spec section 6.20.
The spec does not state that PASID 0 is invalid.
> return -EINVAL;
>
> @@ -37,39 +24,20 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> if (pasid_valid(mm->pasid)) {
> if (mm->pasid < min || mm->pasid >= max)
> ret = -EOVERFLOW;
> + else
> + ret = 0;
Nit:
If you didn't change "int ret = 0" to "int ret", we don't need above two
lines. Did I miss anything?
> goto out;
> }
>
> - pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
> - if (!pasid_valid(pasid))
> - ret = -ENOMEM;
> - else
> - mm->pasid = ret;
> + ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_ATOMIC);
> + if (ret < min)
Nit:
ret < 0?
ida_alloc_range() returns negative error number on failure.
> + goto out;
> + mm->pasid = ret;
> + ret = 0;
> out:
> mutex_unlock(&iommu_sva_lock);
> return ret;
> }
> -EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
> -
> -/* ioasid_find getter() requires a void * argument */
> -static bool __mmget_not_zero(void *mm)
> -{
> - return mmget_not_zero(mm);
> -}
> -
> -/**
> - * iommu_sva_find() - Find mm associated to the given PASID
> - * @pasid: Process Address Space ID assigned to the mm
> - *
> - * On success a reference to the mm is taken, and must be released with mmput().
> - *
> - * Returns the mm corresponding to this PASID, or an error if not found.
> - */
> -struct mm_struct *iommu_sva_find(ioasid_t pasid)
> -{
> - return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
> -}
> -EXPORT_SYMBOL_GPL(iommu_sva_find);
Removing iommu_sva_find() has nothing to do with the intention of this
patch. Perhaps make it in a separated patch?
>
> /**
> * iommu_sva_bind_device() - Bind a process address space to a device
> @@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
>
> void mm_pasid_drop(struct mm_struct *mm)
> {
> - if (pasid_valid(mm->pasid)) {
> - ioasid_free(mm->pasid);
> - mm->pasid = INVALID_IOASID;
> - }
> + if (likely(!pasid_valid(mm->pasid)))
Why is this a likely?
> + return;
> +
> + ida_free(&iommu_global_pasid_ida, mm->pasid);
> }
> diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
> index 7215a761b962..c22d0174ad61 100644
> --- a/drivers/iommu/iommu-sva.h
> +++ b/drivers/iommu/iommu-sva.h
> @@ -8,9 +8,6 @@
> #include <linux/ioasid.h>
> #include <linux/mm_types.h>
>
> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
> -struct mm_struct *iommu_sva_find(ioasid_t pasid);
> -
> /* I/O Page fault */
> struct device;
> struct iommu_fault;
Best regards,
baolu
On 2023/3/2 7:56, Jacob Pan wrote:
> We’re not using spinlock-protected IOASID allocation anymore, there’s
> no need for GFP_ATOMIC.
>
> Signed-off-by: Jacob Pan<[email protected]>
Reviewed-by: Lu Baolu <[email protected]>
Best regards,
baolu
On 2023/3/2 7:56, Jacob Pan wrote:
> INVALID_IOASID and IOMMU_PASID_INVALID are duplicated. Rename
> INVALID_IOASID and consolidate since we are moving away from IOASID
> infrastructure.
>
> Reviewed-by: Dave Jiang<[email protected]>
> Reviewed-by: Jason Gunthorpe<[email protected]>
> Signed-off-by: Jacob Pan<[email protected]>
Reviewed-by: Lu Baolu <[email protected]>
Best regards,
baolu
On 2023/3/2 7:56, Jacob Pan wrote:
> From: Jason Gunthorpe<[email protected]>
>
> This has no use anymore, delete it all.
>
> Signed-off-by: Jason Gunthorpe<[email protected]>
> Signed-off-by: Jacob Pan<[email protected]>
Reviewed-by: Lu Baolu <[email protected]>
Best regards,
baolu
Hi,
On 3/2/23 07:56, Jacob Pan wrote:
> From: Jason Gunthorpe <[email protected]>
>
> Instead SVA drivers can use a simple global IDA to allocate PASIDs for
> each mm_struct.
>
> Future work would be to allow drivers using the SVA APIs to reserve global
> PASIDs from this IDA for their internal use, eg with the DMA API PASID
> support.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
> ---
> v4:
> - Keep GFP_ATOMIC flag for PASID allocation, will changed to
> GFP_KERNEL in a separate patch.
> ---
> drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
> drivers/iommu/iommu-sva.h | 3 --
> 2 files changed, 15 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 376b2a9e2543..297852ae5e7c 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -9,26 +9,13 @@
> #include "iommu-sva.h"
>
> static DEFINE_MUTEX(iommu_sva_lock);
> -static DECLARE_IOASID_SET(iommu_sva_pasid);
> +static DEFINE_IDA(iommu_global_pasid_ida);
>
> -/**
> - * iommu_sva_alloc_pasid - Allocate a PASID for the mm
> - * @mm: the mm
> - * @min: minimum PASID value (inclusive)
> - * @max: maximum PASID value (inclusive)
> - *
> - * Try to allocate a PASID for this mm, or take a reference to the existing one
> - * provided it fits within the [@min, @max] range. On success the PASID is
> - * available in mm->pasid and will be available for the lifetime of the mm.
> - *
> - * Returns 0 on success and < 0 on error.
> - */
> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> +static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> {
> - int ret = 0;
> - ioasid_t pasid;
> + int ret;
>
> - if (min == INVALID_IOASID || max == INVALID_IOASID ||
> + if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
> min == 0 || max < min)
> return -EINVAL;
>
> @@ -37,39 +24,20 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> if (pasid_valid(mm->pasid)) {
> if (mm->pasid < min || mm->pasid >= max)
Here seems not right, since the valid range is defined [min, max].
Shouldn't the invalid range be:
if (mm->pasid < min || mm->pasid > max)
Regards,
-Tina
> ret = -EOVERFLOW;
> + else
> + ret = 0;
> goto out;
> }
>
> - pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
> - if (!pasid_valid(pasid))
> - ret = -ENOMEM;
> - else
> - mm->pasid = ret;
> + ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_ATOMIC);
> + if (ret < min)
> + goto out;
> + mm->pasid = ret;
> + ret = 0;
> out:
> mutex_unlock(&iommu_sva_lock);
> return ret;
> }
> -EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
> -
> -/* ioasid_find getter() requires a void * argument */
> -static bool __mmget_not_zero(void *mm)
> -{
> - return mmget_not_zero(mm);
> -}
> -
> -/**
> - * iommu_sva_find() - Find mm associated to the given PASID
> - * @pasid: Process Address Space ID assigned to the mm
> - *
> - * On success a reference to the mm is taken, and must be released with mmput().
> - *
> - * Returns the mm corresponding to this PASID, or an error if not found.
> - */
> -struct mm_struct *iommu_sva_find(ioasid_t pasid)
> -{
> - return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
> -}
> -EXPORT_SYMBOL_GPL(iommu_sva_find);
>
> /**
> * iommu_sva_bind_device() - Bind a process address space to a device
> @@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
>
> void mm_pasid_drop(struct mm_struct *mm)
> {
> - if (pasid_valid(mm->pasid)) {
> - ioasid_free(mm->pasid);
> - mm->pasid = INVALID_IOASID;
> - }
> + if (likely(!pasid_valid(mm->pasid)))
> + return;
> +
> + ida_free(&iommu_global_pasid_ida, mm->pasid);
> }
> diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
> index 7215a761b962..c22d0174ad61 100644
> --- a/drivers/iommu/iommu-sva.h
> +++ b/drivers/iommu/iommu-sva.h
> @@ -8,9 +8,6 @@
> #include <linux/ioasid.h>
> #include <linux/mm_types.h>
>
> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
> -struct mm_struct *iommu_sva_find(ioasid_t pasid);
> -
> /* I/O Page fault */
> struct device;
> struct iommu_fault;
Hi Kevin,
On Thu, 2 Mar 2023 09:03:52 +0000, "Tian, Kevin" <[email protected]>
wrote:
> > From: Jacob Pan <[email protected]>
> > Sent: Thursday, March 2, 2023 7:57 AM
> >
> > -static inline void mm_pasid_drop(struct mm_struct *mm)
> > -{
> > - if (pasid_valid(mm->pasid)) {
> > - ioasid_free(mm->pasid);
> > - mm->pasid = INVALID_IOASID;
> > - }
> > -}
> > +void mm_pasid_drop(struct mm_struct *mm);
>
> Is it good to have a function declared in a header file of one
> subsystem while being implemented in another subsystem?
Good point! I will move it to iommu_helper.h
Thanks,
Jacob
Hi Baolu,
On Thu, 2 Mar 2023 20:21:33 +0800, Baolu Lu <[email protected]>
wrote:
> On 2023/3/2 7:56, Jacob Pan wrote:
> > Preparing to remove IOASID infrastructure, PASID management will be
> > under SVA code. Decouple mm code from IOASID. Use iommu-help.h instead
> > of iommu.h to prevent circular inclusion.
> >
> > Signed-off-by: Jacob Pan <[email protected]>
> > ---
> > v4: (Jason's comments)
> > - delete and open code mm_set_pasid
> > - keep mm_init_pasid() as inline for fork performance
> > ---
> > drivers/iommu/iommu-sva.c | 10 +++++++++-
> > include/linux/ioasid.h | 2 +-
> > include/linux/iommu-helper.h | 1 +
> > include/linux/sched/mm.h | 18 ++----------------
> > 4 files changed, 13 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index 24bf9b2b58aa..376b2a9e2543 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -44,7 +44,7 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm,
> > ioasid_t min, ioasid_t max) if (!pasid_valid(pasid))
> > ret = -ENOMEM;
> > else
> > - mm_pasid_set(mm, pasid);
> > + mm->pasid = ret;
>
> This seems obviously incorrect.
>
> mm->pasid = pasid;
>
you are right, it got messed up while partitioning. the final was correct.
>
> > out:
> > mutex_unlock(&iommu_sva_lock);
> > return ret;
> > @@ -238,3 +238,11 @@ iommu_sva_handle_iopf(struct iommu_fault *fault,
> > void *data)
> > return status;
> > }
> > +
> > +void mm_pasid_drop(struct mm_struct *mm)
> > +{
> > + if (pasid_valid(mm->pasid)) {
> > + ioasid_free(mm->pasid);
> > + mm->pasid = INVALID_IOASID;
> > + }
> > +}
> > diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
> > index af1c9d62e642..2c502e77ee78 100644
> > --- a/include/linux/ioasid.h
> > +++ b/include/linux/ioasid.h
> > @@ -4,8 +4,8 @@
> >
> > #include <linux/types.h>
> > #include <linux/errno.h>
> > +#include <linux/iommu-helper.h>
> >
> > -#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); diff --git a/include/linux/iommu-helper.h
> > b/include/linux/iommu-helper.h index 74be34f3a20a..0aa922f6bfad 100644
> > --- a/include/linux/iommu-helper.h
> > +++ b/include/linux/iommu-helper.h
> > @@ -40,5 +40,6 @@ static inline unsigned long iommu_num_pages(unsigned
> > long addr,
> > return DIV_ROUND_UP(size, io_page_size);
> > }
> > +#define INVALID_IOASID (-1U)
> >
> > #endif
> > diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> > index 2a243616f222..ae5a3f16b321 100644
> > --- a/include/linux/sched/mm.h
> > +++ b/include/linux/sched/mm.h
> > @@ -8,7 +8,7 @@
> > #include <linux/mm_types.h>
> > #include <linux/gfp.h>
> > #include <linux/sync_core.h>
> > -#include <linux/ioasid.h>
> > +#include <linux/iommu-helper.h>
> >
> > /*
> > * Routines for handling mm_structs
> > @@ -456,23 +456,9 @@ static inline void mm_pasid_init(struct mm_struct
> > *mm) {
> > mm->pasid = INVALID_IOASID;
> > }
> > -
> > -/* Associate a PASID with an mm_struct: */
> > -static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid)
> > -{
> > - mm->pasid = pasid;
> > -}
> > -
> > -static inline void mm_pasid_drop(struct mm_struct *mm)
> > -{
> > - if (pasid_valid(mm->pasid)) {
> > - ioasid_free(mm->pasid);
> > - mm->pasid = INVALID_IOASID;
> > - }
> > -}
> > +void mm_pasid_drop(struct mm_struct *mm);
> > #else
> > static inline void mm_pasid_init(struct mm_struct *mm) {}
> > -static inline void mm_pasid_set(struct mm_struct *mm, u32 pasid) {}
> > static inline void mm_pasid_drop(struct mm_struct *mm) {}
>
> Above mm_pasid_drop() should also be removed.
will do,
Thanks,
Jacob
Hi Kevin,
On Thu, 2 Mar 2023 08:58:21 +0000, "Tian, Kevin" <[email protected]>
wrote:
> > From: Jacob Pan <[email protected]>
> > Sent: Thursday, March 2, 2023 7:57 AM
> >
> >
> > - if (min == INVALID_IOASID || max == INVALID_IOASID ||
> > + if (min == IOMMU_PASID_INVALID || max ==
> > IOMMU_PASID_INVALID ||
> > min == 0 || max < min)
> > return -EINVAL;
> >
>
> if (!pasid_valid(min) || !pasid_valid(max) || ...)
>
will do
> with that,
>
> Reviewed-by: Kevin Tian <[email protected]>
>
Thanks,
Jacob
Hi Baolu,
On Thu, 2 Mar 2023 21:01:42 +0800, Baolu Lu <[email protected]>
wrote:
> On 2023/3/2 7:56, Jacob Pan wrote:
> > From: Jason Gunthorpe <[email protected]>
> >
> > Instead SVA drivers can use a simple global IDA to allocate PASIDs for
> > each mm_struct.
> >
> > Future work would be to allow drivers using the SVA APIs to reserve
> > global PASIDs from this IDA for their internal use, eg with the DMA API
> > PASID support.
> >
> > Signed-off-by: Jason Gunthorpe <[email protected]>
> > Signed-off-by: Jacob Pan <[email protected]>
> > ---
> > v4:
> > - Keep GFP_ATOMIC flag for PASID allocation, will changed to
> > GFP_KERNEL in a separate patch.
> > ---
> > drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
> > drivers/iommu/iommu-sva.h | 3 --
> > 2 files changed, 15 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index 376b2a9e2543..297852ae5e7c 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -9,26 +9,13 @@
> > #include "iommu-sva.h"
> >
> > static DEFINE_MUTEX(iommu_sva_lock);
> > -static DECLARE_IOASID_SET(iommu_sva_pasid);
> > +static DEFINE_IDA(iommu_global_pasid_ida);
> >
> > -/**
> > - * iommu_sva_alloc_pasid - Allocate a PASID for the mm
> > - * @mm: the mm
> > - * @min: minimum PASID value (inclusive)
> > - * @max: maximum PASID value (inclusive)
> > - *
> > - * Try to allocate a PASID for this mm, or take a reference to the
> > existing one
> > - * provided it fits within the [@min, @max] range. On success the
> > PASID is
> > - * available in mm->pasid and will be available for the lifetime of
> > the mm.
> > - *
> > - * Returns 0 on success and < 0 on error.
> > - */
> > -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t
> > max) +static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t
> > min, ioasid_t max) {
> > - int ret = 0;
> > - ioasid_t pasid;
> > + int ret;
> >
> > - if (min == INVALID_IOASID || max == INVALID_IOASID ||
> > + if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
> > min == 0 || max < min)
>
> It's irrelevant to this patch. Just out of curiosity, why do we need to
> exclude PASID 0 here? I just had a quick look at PCI spec section 6.20.
> The spec does not state that PASID 0 is invalid.
>
my understanding is that ARM reserves PASID0, unlike VT-d where RID_PASID
is programmable.
> > return -EINVAL;
> >
> > @@ -37,39 +24,20 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm,
> > ioasid_t min, ioasid_t max) if (pasid_valid(mm->pasid)) {
> > if (mm->pasid < min || mm->pasid >= max)
> > ret = -EOVERFLOW;
> > + else
> > + ret = 0;
>
> Nit:
>
> If you didn't change "int ret = 0" to "int ret", we don't need above two
> lines. Did I miss anything?
>
you are right
> > goto out;
> > }
> >
> > - pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
> > - if (!pasid_valid(pasid))
> > - ret = -ENOMEM;
> > - else
> > - mm->pasid = ret;
> > + ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,
> > GFP_ATOMIC);
> > + if (ret < min)
>
> Nit:
> ret < 0?
will do
> ida_alloc_range() returns negative error number on failure.
>
> > + goto out;
> > + mm->pasid = ret;
> > + ret = 0;
> > out:
> > mutex_unlock(&iommu_sva_lock);
> > return ret;
> > }
> > -EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
> > -
> > -/* ioasid_find getter() requires a void * argument */
> > -static bool __mmget_not_zero(void *mm)
> > -{
> > - return mmget_not_zero(mm);
> > -}
> > -
> > -/**
> > - * iommu_sva_find() - Find mm associated to the given PASID
> > - * @pasid: Process Address Space ID assigned to the mm
> > - *
> > - * On success a reference to the mm is taken, and must be released
> > with mmput().
> > - *
> > - * Returns the mm corresponding to this PASID, or an error if not
> > found.
> > - */
> > -struct mm_struct *iommu_sva_find(ioasid_t pasid)
> > -{
> > - return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
> > -}
> > -EXPORT_SYMBOL_GPL(iommu_sva_find);
>
> Removing iommu_sva_find() has nothing to do with the intention of this
> patch. Perhaps make it in a separated patch?
will do
> >
> > /**
> > * iommu_sva_bind_device() - Bind a process address space to a device
> > @@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault,
> > void *data)
> > void mm_pasid_drop(struct mm_struct *mm)
> > {
> > - if (pasid_valid(mm->pasid)) {
> > - ioasid_free(mm->pasid);
> > - mm->pasid = INVALID_IOASID;
> > - }
> > + if (likely(!pasid_valid(mm->pasid)))
>
> Why is this a likely?
most mm does not have a PASID, thus initialized with invalid ioasid during
fork. This function is called for every mm.
> > + return;
> > +
> > + ida_free(&iommu_global_pasid_ida, mm->pasid);
> > }
> > diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
> > index 7215a761b962..c22d0174ad61 100644
> > --- a/drivers/iommu/iommu-sva.h
> > +++ b/drivers/iommu/iommu-sva.h
> > @@ -8,9 +8,6 @@
> > #include <linux/ioasid.h>
> > #include <linux/mm_types.h>
> >
> > -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t
> > max); -struct mm_struct *iommu_sva_find(ioasid_t pasid);
> > -
> > /* I/O Page fault */
> > struct device;
> > struct iommu_fault;
>
> Best regards,
> baolu
Thanks,
Jacob
Hi Tina,
On Thu, 2 Mar 2023 21:52:42 +0800, Tina Zhang <[email protected]> wrote:
> > if (mm->pasid < min || mm->pasid >= max)
> Here seems not right, since the valid range is defined [min, max].
> Shouldn't the invalid range be:
> if (mm->pasid < min || mm->pasid > max)
yes it is better to be consistent even if we removed the inclusive
requirements in the previous comments.
Thanks,
Jacob
On 3/3/23 1:17 AM, Jacob Pan wrote:
> Hi Baolu,
>
> On Thu, 2 Mar 2023 21:01:42 +0800, Baolu Lu <[email protected]>
> wrote:
>
>> On 2023/3/2 7:56, Jacob Pan wrote:
>>> From: Jason Gunthorpe <[email protected]>
>>>
>>> Instead SVA drivers can use a simple global IDA to allocate PASIDs for
>>> each mm_struct.
>>>
>>> Future work would be to allow drivers using the SVA APIs to reserve
>>> global PASIDs from this IDA for their internal use, eg with the DMA API
>>> PASID support.
>>>
>>> Signed-off-by: Jason Gunthorpe <[email protected]>
>>> Signed-off-by: Jacob Pan <[email protected]>
>>> ---
>>> v4:
>>> - Keep GFP_ATOMIC flag for PASID allocation, will changed to
>>> GFP_KERNEL in a separate patch.
>>> ---
>>> drivers/iommu/iommu-sva.c | 62 ++++++++++-----------------------------
>>> drivers/iommu/iommu-sva.h | 3 --
>>> 2 files changed, 15 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
>>> index 376b2a9e2543..297852ae5e7c 100644
>>> --- a/drivers/iommu/iommu-sva.c
>>> +++ b/drivers/iommu/iommu-sva.c
>>> @@ -9,26 +9,13 @@
>>> #include "iommu-sva.h"
>>>
>>> static DEFINE_MUTEX(iommu_sva_lock);
>>> -static DECLARE_IOASID_SET(iommu_sva_pasid);
>>> +static DEFINE_IDA(iommu_global_pasid_ida);
>>>
>>> -/**
>>> - * iommu_sva_alloc_pasid - Allocate a PASID for the mm
>>> - * @mm: the mm
>>> - * @min: minimum PASID value (inclusive)
>>> - * @max: maximum PASID value (inclusive)
>>> - *
>>> - * Try to allocate a PASID for this mm, or take a reference to the
>>> existing one
>>> - * provided it fits within the [@min, @max] range. On success the
>>> PASID is
>>> - * available in mm->pasid and will be available for the lifetime of
>>> the mm.
>>> - *
>>> - * Returns 0 on success and < 0 on error.
>>> - */
>>> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t
>>> max) +static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t
>>> min, ioasid_t max) {
>>> - int ret = 0;
>>> - ioasid_t pasid;
>>> + int ret;
>>>
>>> - if (min == INVALID_IOASID || max == INVALID_IOASID ||
>>> + if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
>>> min == 0 || max < min)
>>
>> It's irrelevant to this patch. Just out of curiosity, why do we need to
>> exclude PASID 0 here? I just had a quick look at PCI spec section 6.20.
>> The spec does not state that PASID 0 is invalid.
>>
> my understanding is that ARM reserves PASID0, unlike VT-d where RID_PASID
> is programmable.
I suppose the common thing is reserving some kind of special PASIDs.
>
>>> return -EINVAL;
>>>
>>> @@ -37,39 +24,20 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm,
>>> ioasid_t min, ioasid_t max) if (pasid_valid(mm->pasid)) {
>>> if (mm->pasid < min || mm->pasid >= max)
>>> ret = -EOVERFLOW;
>>> + else
>>> + ret = 0;
>>
>> Nit:
>>
>> If you didn't change "int ret = 0" to "int ret", we don't need above two
>> lines. Did I miss anything?
>>
> you are right
>
>>> goto out;
>>> }
>>>
>>> - pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
>>> - if (!pasid_valid(pasid))
>>> - ret = -ENOMEM;
>>> - else
>>> - mm->pasid = ret;
>>> + ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,
>>> GFP_ATOMIC);
>>> + if (ret < min)
>>
>> Nit:
>> ret < 0?
> will do
>
>> ida_alloc_range() returns negative error number on failure.
>>
>>> + goto out;
>>> + mm->pasid = ret;
>>> + ret = 0;
>>> out:
>>> mutex_unlock(&iommu_sva_lock);
>>> return ret;
>>> }
>>> -EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
>>> -
>>> -/* ioasid_find getter() requires a void * argument */
>>> -static bool __mmget_not_zero(void *mm)
>>> -{
>>> - return mmget_not_zero(mm);
>>> -}
>>> -
>>> -/**
>>> - * iommu_sva_find() - Find mm associated to the given PASID
>>> - * @pasid: Process Address Space ID assigned to the mm
>>> - *
>>> - * On success a reference to the mm is taken, and must be released
>>> with mmput().
>>> - *
>>> - * Returns the mm corresponding to this PASID, or an error if not
>>> found.
>>> - */
>>> -struct mm_struct *iommu_sva_find(ioasid_t pasid)
>>> -{
>>> - return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
>>> -}
>>> -EXPORT_SYMBOL_GPL(iommu_sva_find);
>>
>> Removing iommu_sva_find() has nothing to do with the intention of this
>> patch. Perhaps make it in a separated patch?
> will do
>
>>>
>>> /**
>>> * iommu_sva_bind_device() - Bind a process address space to a device
>>> @@ -241,8 +209,8 @@ iommu_sva_handle_iopf(struct iommu_fault *fault,
>>> void *data)
>>> void mm_pasid_drop(struct mm_struct *mm)
>>> {
>>> - if (pasid_valid(mm->pasid)) {
>>> - ioasid_free(mm->pasid);
>>> - mm->pasid = INVALID_IOASID;
>>> - }
>>> + if (likely(!pasid_valid(mm->pasid)))
>>
>> Why is this a likely?
> most mm does not have a PASID, thus initialized with invalid ioasid during
> fork. This function is called for every mm.
Make sense.
>
>>> + return;
>>> +
>>> + ida_free(&iommu_global_pasid_ida, mm->pasid);
>>> }
>>> diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
>>> index 7215a761b962..c22d0174ad61 100644
>>> --- a/drivers/iommu/iommu-sva.h
>>> +++ b/drivers/iommu/iommu-sva.h
>>> @@ -8,9 +8,6 @@
>>> #include <linux/ioasid.h>
>>> #include <linux/mm_types.h>
>>>
>>> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t
>>> max); -struct mm_struct *iommu_sva_find(ioasid_t pasid);
>>> -
>>> /* I/O Page fault */
>>> struct device;
>>> struct iommu_fault;
>>
>> Best regards,
>> baolu
>
>
> Thanks,
>
> Jacob
Best regards,
baolu
On Fri, Mar 03, 2023 at 10:24:42AM +0800, Baolu Lu wrote:
> > > > -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t
> > > > max) +static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t
> > > > min, ioasid_t max) {
> > > > - int ret = 0;
> > > > - ioasid_t pasid;
> > > > + int ret;
> > > > - if (min == INVALID_IOASID || max == INVALID_IOASID ||
> > > > + if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
> > > > min == 0 || max < min)
> > >
> > > It's irrelevant to this patch. Just out of curiosity, why do we need to
> > > exclude PASID 0 here? I just had a quick look at PCI spec section 6.20.
> > > The spec does not state that PASID 0 is invalid.
> > >
> > my understanding is that ARM reserves PASID0, unlike VT-d where RID_PASID
> > is programmable.
It does, but that's specific to the IOMMU driver so we shouldn't check it
here.
> I suppose the common thing is reserving some kind of special PASIDs.
Are you planning to use RID_PASID != 0 in VT-d? Otherwise we could just
communicate min_pasid from the IOMMU driver the same way we do max_pasid.
Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
driver calls to reserve PASID0/RID_PASID.
Thanks,
Jean
On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
>> I suppose the common thing is reserving some kind of special PASIDs.
> Are you planning to use RID_PASID != 0 in VT-d? Otherwise we could just
> communicate min_pasid from the IOMMU driver the same way we do max_pasid.
>
> Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> driver calls to reserve PASID0/RID_PASID.
Yes. We probably will use a non-zero RID_PASID in the future. An
interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
should work then.
Best regards,
baolu
On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
> > > I suppose the common thing is reserving some kind of special PASIDs.
> > Are you planning to use RID_PASID != 0 in VT-d? Otherwise we could just
> > communicate min_pasid from the IOMMU driver the same way we do max_pasid.
> >
> > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > driver calls to reserve PASID0/RID_PASID.
>
> Yes. We probably will use a non-zero RID_PASID in the future. An
> interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> should work then.
Just allowing the driver to store XA_ZERO_ENTRY would be fine
Jason
Hi Jason,
On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <[email protected]> wrote:
> On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> > On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
> > > > I suppose the common thing is reserving some kind of special
> > > > PASIDs.
> > > Are you planning to use RID_PASID != 0 in VT-d? Otherwise we could
> > > just communicate min_pasid from the IOMMU driver the same way we do
> > > max_pasid.
> > >
> > > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > > driver calls to reserve PASID0/RID_PASID.
> >
> > Yes. We probably will use a non-zero RID_PASID in the future. An
> > interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> > should work then.
>
> Just allowing the driver to store XA_ZERO_ENTRY would be fine
>
So we provide APIs for both?
1. alloc a global PASID, returned by this API
2. try to reserve a global PASID given by the driver, i.e.
xa_cmpxchg(&iommu_global_pasid_ida.xa, 2, NULL, XA_ZERO_ENTRY,
GFP_KERNEL);
seems #1 is sufficient.
Thanks,
Jacob
Hi Jason,
On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <[email protected]> wrote:
> On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> > On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
> > > > I suppose the common thing is reserving some kind of special
> > > > PASIDs.
> > > Are you planning to use RID_PASID != 0 in VT-d? Otherwise we could
> > > just communicate min_pasid from the IOMMU driver the same way we do
> > > max_pasid.
> > >
> > > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > > driver calls to reserve PASID0/RID_PASID.
> >
> > Yes. We probably will use a non-zero RID_PASID in the future. An
> > interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> > should work then.
>
> Just allowing the driver to store XA_ZERO_ENTRY would be fine
>
It looks like there are incoming users of iommu_sva_find()
https://lore.kernel.org/lkml/[email protected]/T/#m1fc97725a0e56ea269c8bdabacee447070d51846
Should we keep the xa here instead of the global ida?
Thanks,
Jacob
> From: Jacob Pan <[email protected]>
> Sent: Wednesday, March 8, 2023 1:42 AM
>
> Hi Jason,
>
> On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <[email protected]>
> wrote:
>
> > On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> > > On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
> > > > > I suppose the common thing is reserving some kind of special
> > > > > PASIDs.
> > > > Are you planning to use RID_PASID != 0 in VT-d? Otherwise we could
> > > > just communicate min_pasid from the IOMMU driver the same way we
> do
> > > > max_pasid.
> > > >
> > > > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > > > driver calls to reserve PASID0/RID_PASID.
> > >
> > > Yes. We probably will use a non-zero RID_PASID in the future. An
> > > interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> > > should work then.
> >
> > Just allowing the driver to store XA_ZERO_ENTRY would be fine
> >
> So we provide APIs for both?
> 1. alloc a global PASID, returned by this API
> 2. try to reserve a global PASID given by the driver, i.e.
> xa_cmpxchg(&iommu_global_pasid_ida.xa, 2, NULL,
> XA_ZERO_ENTRY,
> GFP_KERNEL);
> seems #1 is sufficient.
>
No need for both. There will be on-demand allocation on this global
space so a reservation interface doesn't make sense.
On Tue, Mar 07, 2023 at 02:32:09PM -0800, Jacob Pan wrote:
> Hi Jason,
>
> On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <[email protected]> wrote:
>
> > On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
> > > On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
> > > > > I suppose the common thing is reserving some kind of special
> > > > > PASIDs.
> > > > Are you planning to use RID_PASID != 0 in VT-d? Otherwise we could
> > > > just communicate min_pasid from the IOMMU driver the same way we do
> > > > max_pasid.
> > > >
> > > > Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
> > > > driver calls to reserve PASID0/RID_PASID.
> > >
> > > Yes. We probably will use a non-zero RID_PASID in the future. An
> > > interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
> > > should work then.
> >
> > Just allowing the driver to store XA_ZERO_ENTRY would be fine
> >
> It looks like there are incoming users of iommu_sva_find()
> https://lore.kernel.org/lkml/[email protected]/T/#m1fc97725a0e56ea269c8bdabacee447070d51846
> Should we keep the xa here instead of the global ida?
I'm not sure this should be in the iommu core, it is really gross.
I would expect IDXD to keep track of the PASID's and mms it is using
and do this kind of stuff itself.
And why is this using access_remote_vm anyhow? If you know you are in
a kthread then kthread_use_mm() is probably better anyhow.
In any event we don't need a iommu_sva_find() function to wrapper
xa_load for another function inside the same .c file.
Jason
Hi, Jason and Jacob,
On 3/8/23 10:23, Jason Gunthorpe wrote:
> On Tue, Mar 07, 2023 at 02:32:09PM -0800, Jacob Pan wrote:
>> Hi Jason,
>>
>> On Fri, 3 Mar 2023 09:15:45 -0400, Jason Gunthorpe <[email protected]> wrote:
>>
>>> On Fri, Mar 03, 2023 at 05:57:41PM +0800, Baolu Lu wrote:
>>>> On 2023/3/3 17:32, Jean-Philippe Brucker wrote:
>>>>>> I suppose the common thing is reserving some kind of special
>>>>>> PASIDs.
>>>>> Are you planning to use RID_PASID != 0 in VT-d? Otherwise we could
>>>>> just communicate min_pasid from the IOMMU driver the same way we do
>>>>> max_pasid.
>>>>>
>>>>> Otherwise I guess re-introduce a lighter ioasid_alloc() that the IOMMU
>>>>> driver calls to reserve PASID0/RID_PASID.
>>>>
>>>> Yes. We probably will use a non-zero RID_PASID in the future. An
>>>> interface to reserve (or allocate) a PASID from iommu_global_pasid_ida
>>>> should work then.
>>>
>>> Just allowing the driver to store XA_ZERO_ENTRY would be fine
>>>
>> It looks like there are incoming users of iommu_sva_find()
>> https://lore.kernel.org/lkml/[email protected]/T/#m1fc97725a0e56ea269c8bdabacee447070d51846
>> Should we keep the xa here instead of the global ida?
>
> I'm not sure this should be in the iommu core, it is really gross.
>
> I would expect IDXD to keep track of the PASID's and mms it is using
> and do this kind of stuff itself.
>
> And why is this using access_remote_vm anyhow? If you know you are in
> a kthread then kthread_use_mm() is probably better anyhow.
>
> In any event we don't need a iommu_sva_find() function to wrapper
> xa_load for another function inside the same .c file.
Ok. I will maintain mm and find mm from PASID inside IDXD driver. And
will implement accessing the remote mm inside IDXD driver although the
implementation will have duplicate code as access_remote_vm().
Thanks.
-Fenghua
On Sat, Mar 11, 2023 at 09:18:30AM -0800, Fenghua Yu wrote:
> Ok. I will maintain mm and find mm from PASID inside IDXD driver. And will
> implement accessing the remote mm inside IDXD driver although the
> implementation will have duplicate code as access_remote_vm().
If you really need it and it really makes sense, then export it
Jason