Re: [PATCH] iommu: Add Kconfig help text for IOMMU_SVA
From: Jacob Pan
Date: Mon May 08 2023 - 16:17:12 EST
Hi Linus,
On Mon, 8 May 2023 10:17:53 -0700, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, May 8, 2023 at 9:55 AM Jason Gunthorpe <jgg@xxxxxxxxxx> 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