Re: [PATCH v3 04/11] kernel/fork: Initialize mm's PASID

[Fenghua Yu]

Hi, Thomas,

On Sat, Feb 05, 2022 at 12:22:12AM +0100, Thomas Gleixner wrote:
> On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote:
> > A new mm doesn't have a PASID yet when it's created. Initialize
> > the mm's PASID on fork() or for init_mm to INVALID_IOASID (-1).
> 
> I must be missing something here.
> 
> > diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> > index aa5f09ca5bcf..c74d1edbac2f 100644
> > --- a/include/linux/sched/mm.h
> > +++ b/include/linux/sched/mm.h
> > @@ -8,6 +8,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  /*
> >   * Routines for handling mm_structs
> > @@ -433,4 +434,13 @@ static inline void membarrier_update_current_mm(struct 
> > mm_struct *next_mm)
> >  }
> >  #endif
> >  
> > +#ifdef CONFIG_IOMMU_SVA
> > +static inline void mm_pasid_init(struct mm_struct *mm)
> > +{
> > +   mm->pasid = INVALID_IOASID;
> > +}
> > +#else
> > +static inline void mm_pasid_init(struct mm_struct *mm) {}
> > +#endif
> > +
> >  #endif /* _LINUX_SCHED_MM_H */
> 
> So this adds mm_pasid_init() to linux/sched/mm.h which replaces:
> 
> > -static void mm_init_pasid(struct mm_struct *mm)
> > -{
> > -#ifdef CONFIG_IOMMU_SVA
> > -   mm->pasid = INIT_PASID;
> > -#endif
> > -}
> > -
> 
> I.e. already existing code which is initializing mm->pasid with
> INIT_PASID (0) while the replacement initializes it with INVALID_IOASID
> (-1).
> 
> The change log does not have any information about why INIT_PASID is the
> wrong initialization value and why this change is not having any side
> effects.

I should add the following info in the commit message to explain why
change INIT_PASID (0) to INVALID_IOASID (-1):

INIT_PASID (0) is reserved for kernel legacy DMA PASID. It cannot be
allocated to a user process. Initialize the process's PASID to 0 may
cause confusion that why the process uses reserved kernel legacy DMA
PASID. Initializing the PASID to INVALID_IOASID (-1) explicitly
tells the process doesn't have a valid PASID yet initially.

Is it OK for you?

> 
> It neither mentions why having this in a global available header makes
> sense when the only call site is in the C file from which the already
> existing function is removed.

This series defines three helpers mm_pasid_init(), mm_pasid_set(), and
mm_pasid_drop() in mm because they handle the pasid member in mm_struct
and should be part of mm operations. I explained why mm_pasid_set() and
mm_pasid_drop() are defined in mm, but I didn't explain why mm_pasid_init()
is define in mm.

Is it OK to add the following explanation on why mm_pasid_init() is defined?

mm_pasid_init() is defined in mm and replaces mm_init_pasid() because
the PASID init operation initializes the pasid member in mm_struct and
should be part of mm operations.

Thanks,

-Fenghua
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 04/11] kernel/fork: Initialize mm's PASID

[Thomas Gleixner]

On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote:
> A new mm doesn't have a PASID yet when it's created. Initialize
> the mm's PASID on fork() or for init_mm to INVALID_IOASID (-1).

I must be missing something here.

> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index aa5f09ca5bcf..c74d1edbac2f 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -8,6 +8,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /*
>   * Routines for handling mm_structs
> @@ -433,4 +434,13 @@ static inline void membarrier_update_current_mm(struct 
> mm_struct *next_mm)
>  }
>  #endif
>  
> +#ifdef CONFIG_IOMMU_SVA
> +static inline void mm_pasid_init(struct mm_struct *mm)
> +{
> + mm->pasid = INVALID_IOASID;
> +}
> +#else
> +static inline void mm_pasid_init(struct mm_struct *mm) {}
> +#endif
> +
>  #endif /* _LINUX_SCHED_MM_H */

So this adds mm_pasid_init() to linux/sched/mm.h which replaces:

> -static void mm_init_pasid(struct mm_struct *mm)
> -{
> -#ifdef CONFIG_IOMMU_SVA
> - mm->pasid = INIT_PASID;
> -#endif
> -}
> -

I.e. already existing code which is initializing mm->pasid with
INIT_PASID (0) while the replacement initializes it with INVALID_IOASID
(-1).

The change log does not have any information about why INIT_PASID is the
wrong initialization value and why this change is not having any side
effects.

It neither mentions why having this in a global available header makes
sense when the only call site is in the C file from which the already
existing function is removed.

Thanks,

tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v3 04/11] kernel/fork: Initialize mm's PASID

[Fenghua Yu]

A new mm doesn't have a PASID yet when it's created. Initialize
the mm's PASID on fork() or for init_mm to INVALID_IOASID (-1).

Suggested-by: Dave Hansen 
Signed-off-by: Fenghua Yu 
Reviewed-by: Tony Luck 
---
v2:
- Change condition to more accurate CONFIG_IOMMU_SVA (Jacob)

 include/linux/sched/mm.h | 10 ++
 kernel/fork.c| 10 ++
 mm/init-mm.c |  4 
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index aa5f09ca5bcf..c74d1edbac2f 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -8,6 +8,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * Routines for handling mm_structs
@@ -433,4 +434,13 @@ static inline void membarrier_update_current_mm(struct 
mm_struct *next_mm)
 }
 #endif
 
+#ifdef CONFIG_IOMMU_SVA
+static inline void mm_pasid_init(struct mm_struct *mm)
+{
+   mm->pasid = INVALID_IOASID;
+}
+#else
+static inline void mm_pasid_init(struct mm_struct *mm) {}
+#endif
+
 #endif /* _LINUX_SCHED_MM_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 6ee7551d3bd2..deacd2c17a7f 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -97,6 +97,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1019,13 +1020,6 @@ static void mm_init_owner(struct mm_struct *mm, struct 
task_struct *p)
 #endif
 }
 
-static void mm_init_pasid(struct mm_struct *mm)
-{
-#ifdef CONFIG_IOMMU_SVA
-   mm->pasid = INIT_PASID;
-#endif
-}
-
 static void mm_init_uprobes_state(struct mm_struct *mm)
 {
 #ifdef CONFIG_UPROBES
@@ -1054,7 +1048,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, 
struct task_struct *p,
mm_init_cpumask(mm);
mm_init_aio(mm);
mm_init_owner(mm, p);
-   mm_init_pasid(mm);
+   mm_pasid_init(mm);
RCU_INIT_POINTER(mm->exe_file, NULL);
mmu_notifier_subscriptions_init(mm);
init_tlb_flush_pending(mm);
diff --git a/mm/init-mm.c b/mm/init-mm.c
index b4a6f38fb51d..fbe7844d0912 100644
--- a/mm/init-mm.c
+++ b/mm/init-mm.c
@@ -10,6 +10,7 @@
 
 #include 
 #include 
+#include 
 #include 
 
 #ifndef INIT_MM_CONTEXT
@@ -38,6 +39,9 @@ struct mm_struct init_mm = {
.mmlist = LIST_HEAD_INIT(init_mm.mmlist),
.user_ns= _user_ns,
.cpu_bitmap = CPU_BITS_NONE,
+#ifdef CONFIG_IOMMU_SVA
+   .pasid  = INVALID_IOASID,
+#endif
INIT_MM_CONTEXT(init_mm)
 };
 
-- 
2.35.0

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu