* Re: [PATCH v6 08/12] fork: Clear PASID for new mm
[not found] ` <1594684087-61184-9-git-send-email-fenghua.yu@intel.com>
@ 2021-02-24 10:19 ` Jean-Philippe Brucker
2021-02-25 22:17 ` Fenghua Yu
0 siblings, 1 reply; 3+ messages in thread
From: Jean-Philippe Brucker @ 2021-02-24 10:19 UTC (permalink / raw)
To: Fenghua Yu
Cc: Joerg Roedel, Lu Baolu, Ashok Raj, Jacob Jun Pan, linux-kernel,
iommu, zhangfei.gao, linux-mm
Hi Fenghua,
[Trimmed the Cc list]
On Mon, Jul 13, 2020 at 04:48:03PM -0700, Fenghua Yu wrote:
> When a new mm is created, its PASID should be cleared, i.e. the PASID is
> initialized to its init state 0 on both ARM and X86.
I just noticed this patch was dropped in v7, and am wondering whether we
could still upstream it. Does x86 need a child with a new address space
(!CLONE_VM) to inherit the PASID of the parent? That doesn't make much
sense with regard to IOMMU structures - same PASID indexing multiple PGDs?
Currently iommu_sva_alloc_pasid() assumes mm->pasid is always initialized
to 0 and fails on forked tasks. I'm trying to figure out how to fix this.
Could we clear the pasid on fork or does it break the x86 model?
Thanks,
Jean
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
> v2:
> - Add this patch to initialize PASID value for a new mm.
>
> include/linux/mm_types.h | 2 ++
> kernel/fork.c | 8 ++++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index d61285cfe027..d60d2ec10881 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -22,6 +22,8 @@
> #endif
> #define AT_VECTOR_SIZE (2*(AT_VECTOR_SIZE_ARCH + AT_VECTOR_SIZE_BASE + 1))
>
> +/* Initial PASID value is 0. */
> +#define INIT_PASID 0
>
> struct address_space;
> struct mem_cgroup;
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 142b23645d82..43b5f112604d 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1007,6 +1007,13 @@ 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_SUPPORT
> + mm->pasid = INIT_PASID;
> +#endif
> +}
> +
> static void mm_init_uprobes_state(struct mm_struct *mm)
> {
> #ifdef CONFIG_UPROBES
> @@ -1035,6 +1042,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);
> RCU_INIT_POINTER(mm->exe_file, NULL);
> mmu_notifier_subscriptions_init(mm);
> init_tlb_flush_pending(mm);
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v6 08/12] fork: Clear PASID for new mm
2021-02-24 10:19 ` [PATCH v6 08/12] fork: Clear PASID for new mm Jean-Philippe Brucker
@ 2021-02-25 22:17 ` Fenghua Yu
2021-03-01 23:00 ` Jacob Pan
0 siblings, 1 reply; 3+ messages in thread
From: Fenghua Yu @ 2021-02-25 22:17 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Joerg Roedel, Lu Baolu, Ashok Raj, Jacob Jun Pan, linux-kernel,
iommu, zhangfei.gao, linux-mm
Hi, Jean,
On Wed, Feb 24, 2021 at 11:19:27AM +0100, Jean-Philippe Brucker wrote:
> Hi Fenghua,
>
> [Trimmed the Cc list]
>
> On Mon, Jul 13, 2020 at 04:48:03PM -0700, Fenghua Yu wrote:
> > When a new mm is created, its PASID should be cleared, i.e. the PASID is
> > initialized to its init state 0 on both ARM and X86.
>
> I just noticed this patch was dropped in v7, and am wondering whether we
> could still upstream it. Does x86 need a child with a new address space
> (!CLONE_VM) to inherit the PASID of the parent? That doesn't make much
> sense with regard to IOMMU structures - same PASID indexing multiple PGDs?
You are right: x86 should clear mm->pasid when a new mm is created.
This patch somehow is losted:(
>
> Currently iommu_sva_alloc_pasid() assumes mm->pasid is always initialized
> to 0 and fails on forked tasks. I'm trying to figure out how to fix this.
> Could we clear the pasid on fork or does it break the x86 model?
x86 calls ioasid_alloc() instead of iommu_sva_alloc_pasid(). So
functionality is not a problem without this patch on x86. But I think
we do need to have this patch in the kernel because PASID is per addr
space and two addr spaces shouldn't have the same PASID.
Who will accept this patch?
Thanks.
-Fenghua
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v6 08/12] fork: Clear PASID for new mm
2021-02-25 22:17 ` Fenghua Yu
@ 2021-03-01 23:00 ` Jacob Pan
0 siblings, 0 replies; 3+ messages in thread
From: Jacob Pan @ 2021-03-01 23:00 UTC (permalink / raw)
To: Fenghua Yu
Cc: Jean-Philippe Brucker, Joerg Roedel, Lu Baolu, Ashok Raj,
linux-kernel, iommu, zhangfei.gao, linux-mm, jacob.jun.pan
Hi Fenghua,
On Thu, 25 Feb 2021 22:17:11 +0000, Fenghua Yu <fenghua.yu@intel.com> wrote:
> Hi, Jean,
>
> On Wed, Feb 24, 2021 at 11:19:27AM +0100, Jean-Philippe Brucker wrote:
> > Hi Fenghua,
> >
> > [Trimmed the Cc list]
> >
> > On Mon, Jul 13, 2020 at 04:48:03PM -0700, Fenghua Yu wrote:
> > > When a new mm is created, its PASID should be cleared, i.e. the PASID
> > > is initialized to its init state 0 on both ARM and X86.
> >
> > I just noticed this patch was dropped in v7, and am wondering whether we
> > could still upstream it. Does x86 need a child with a new address space
> > (!CLONE_VM) to inherit the PASID of the parent? That doesn't make much
> > sense with regard to IOMMU structures - same PASID indexing multiple
> > PGDs?
>
> You are right: x86 should clear mm->pasid when a new mm is created.
> This patch somehow is losted:(
>
> >
> > Currently iommu_sva_alloc_pasid() assumes mm->pasid is always
> > initialized to 0 and fails on forked tasks. I'm trying to figure out
> > how to fix this. Could we clear the pasid on fork or does it break the
> > x86 model?
>
> x86 calls ioasid_alloc() instead of iommu_sva_alloc_pasid(). So
We should consolidate at some point, there is no need to store pasid in two
places.
> functionality is not a problem without this patch on x86. But I think
I feel the reason that x86 doesn't care is that mm->pasid is not used
unless bind_mm is called. For the fork children even mm->pasid is non-zero,
it has no effect since it is not loaded onto MSRs.
Perhaps you could also add a check or WARN_ON(!mm->pasid) in load_pasid()?
> we do need to have this patch in the kernel because PASID is per addr
> space and two addr spaces shouldn't have the same PASID.
>
Agreed.
> Who will accept this patch?
>
> Thanks.
>
> -Fenghua
Thanks,
Jacob
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-01 22:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1594684087-61184-1-git-send-email-fenghua.yu@intel.com>
[not found] ` <1594684087-61184-9-git-send-email-fenghua.yu@intel.com>
2021-02-24 10:19 ` [PATCH v6 08/12] fork: Clear PASID for new mm Jean-Philippe Brucker
2021-02-25 22:17 ` Fenghua Yu
2021-03-01 23:00 ` Jacob Pan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).