Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

Hi Linus,

On Mon, 8 May 2023 10:17:53 -0700, Linus Torvalds
<torvalds@linux-foundation.org> wrote:

> On Mon, May 8, 2023 at 9:55 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > CONFIG_IOMMU_MM_PASID perhaps? Says what it does without a clue about
> > why it does it. x86 arch code could select it ideally?  
> 
> Maybe we don't even need the "IOMMU" part. It's a core x86
> architecture feature. Maybe it usually (always?) gets used within the
> framework of some IOMMU work, but I guess ENCQCMD could be used in
> some hardwired way even without that (ie is it possible to have just
> some "basic PASID set up by VMM environment" thing?)
> 
> I don't actually know who uses it and how, so I'm probably not the
> right person to name it, but just looking at the x86 code that sets
> it, the PASID code technically has no connection to any iommu code,
> it's literally a core CPU feature with an MSR and some magic faulting
> thing, and seems to be possibly usable as-is.
> 
> That existing
> 
>     #ifdef CONFIG_IOMMU_SVA
> 
> in the x86 traps.c code just looks odd, and making it be
> CONFIG_IOMMU_MM_PASID sounds better to me. I'm just not sure about the
> "IOMMU" part either. Just "MM_PASID"?
> 
> That said, the arm-smmu-v3-sva.c code clearly *also* uses pasid,
> except it seems to really want to call it "ssid".
> 
> So "having a per-mm ASID for IO" is clearly a common feature. But
> naming seems hard, with x86 calling it PASID, arm64 seemingly calling
> it "SSID".
> 
> Right now the only user *does* seem to be through the IOMMU SVA code,
> but that may or may not be fundamental.
> 
> Now, "SSID" is a completely horrible name, as I immediately realized
> when I tried to google for it. So arm64 is just wrong, and we're most
> definitely continuing to call it PASID.
> 
> I'd lean towards just "CONFIG_MM_PASID" or something, but at some
> point this is bikeshedding, and I don't know about any possible
> non-iommu direct uses?
+Jean who has mentioned potential use of PASID without IOMMU. But I don't
think there is anything in the current kernel.
Leave the name MM_PASID aside, I am trying to capture the discussion with a
patch below. I am struggling to get a clean solution since SVA code is
common as you said "having a per-mm ASID for IO". Having x86 Kconfig select
MM_PASID would be redundant if it is already selected by IOMMU_SVA. Perhaps
I am totally missing the point.

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 58b1f208eff5..d69acc69bbbb 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -652,7 +652,7 @@ static bool fixup_iopl_exception(struct pt_regs *regs)
  */
 static bool try_fixup_enqcmd_gp(void)
 {
-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
 	u32 pasid;
 
 	/*
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index db98c3f86e8c..7106f3af74ee 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -153,9 +153,13 @@ config IOMMU_DMA
 	select IRQ_MSI_IOMMU
 	select NEED_SG_DMA_LENGTH
 
+config IOMMU_MM_PASID
+	bool
+
 # Shared Virtual Addressing
 config IOMMU_SVA
 	bool
+	select IOMMU_MM_PASID
 
 config FSL_PAMU
 	bool "Freescale IOMMU support"
diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
index 2e56bd79f589..b4d7bd68a911 100644
--- a/drivers/iommu/intel/Kconfig
+++ b/drivers/iommu/intel/Kconfig
@@ -50,6 +50,7 @@ config INTEL_IOMMU_SVM
 	depends on X86_64
 	select MMU_NOTIFIER
 	select IOMMU_SVA
+	select IOMMU_MM_PASID
 	help
 	  Shared Virtual Memory (SVM) provides a facility for devices
 	  to access DMA resources through process address space by
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e8c9a7da1060..bdd7f4c1b9ad 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1166,22 +1166,12 @@ static inline bool tegra_dev_iommu_get_stream_id(struct device *dev, u32 *stream
 }
 
 #ifdef CONFIG_IOMMU_SVA
-static inline void mm_pasid_init(struct mm_struct *mm)
-{
-	mm->pasid = IOMMU_PASID_INVALID;
-}
-static inline bool mm_valid_pasid(struct mm_struct *mm)
-{
-	return mm->pasid != IOMMU_PASID_INVALID;
-}
-void mm_pasid_drop(struct mm_struct *mm);
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm);
 void iommu_sva_unbind_device(struct iommu_sva *handle);
 u32 iommu_sva_get_pasid(struct iommu_sva *handle);
 #else
-static inline struct iommu_sva *
-iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
+static inline struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
 {
 	return NULL;
 }
@@ -1194,9 +1184,22 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
 {
 	return IOMMU_PASID_INVALID;
 }
+#endif /* CONFIG_IOMMU_SVA */
+
+#ifdef CONFIG_IOMMU_MM_PASID
+static inline void mm_pasid_init(struct mm_struct *mm)
+{
+	mm->pasid = IOMMU_PASID_INVALID;
+}
+static inline bool mm_valid_pasid(struct mm_struct *mm)
+{
+	return mm->pasid != IOMMU_PASID_INVALID;
+}
+void mm_pasid_drop(struct mm_struct *mm);
+#else
 static inline void mm_pasid_init(struct mm_struct *mm) {}
 static inline bool mm_valid_pasid(struct mm_struct *mm) { return false; }
 static inline void mm_pasid_drop(struct mm_struct *mm) {}
-#endif /* CONFIG_IOMMU_SVA */
+#endif /* CONFIG_IOMMU_MM_PASID */
 
 #endif /* __LINUX_IOMMU_H */
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 306a3d1a0fa6..70740a4ab58a 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -771,7 +771,7 @@ struct mm_struct {
 #endif
 		struct work_struct async_put_work;
 
-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
 		u32 pasid;
 #endif
 #ifdef CONFIG_KSM
diff --git a/include/linux/sched.h b/include/linux/sched.h
index eed5d65b8d1f..0b6498eafb0c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -945,7 +945,7 @@ struct task_struct {
 	/* Recursion prevention for eventfd_signal() */
 	unsigned			in_eventfd:1;
 #endif
-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
 	unsigned			pasid_activated:1;
 #endif
 #ifdef	CONFIG_CPU_SUP_INTEL
diff --git a/kernel/fork.c b/kernel/fork.c
index ed4e01daccaa..cb02ddadd6fb 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1177,7 +1177,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 	tsk->use_memdelay = 0;
 #endif
 
-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
 	tsk->pasid_activated = 0;
 #endif
 
diff --git a/mm/init-mm.c b/mm/init-mm.c
index efa97b57acfd..b97414c2b2f7 100644
--- a/mm/init-mm.c
+++ b/mm/init-mm.c
@@ -42,7 +42,7 @@ struct mm_struct init_mm = {
 #endif
 	.user_ns	= &init_user_ns,
 	.cpu_bitmap	= CPU_BITS_NONE,
-#ifdef CONFIG_IOMMU_SVA
+#ifdef CONFIG_IOMMU_MM_PASID
 	.pasid		= IOMMU_PASID_INVALID,
 #endif
 	INIT_MM_CONTEXT(init_mm)



Thanks,

Jacob