[6.6] membarrier: riscv: Add full memory barrier in switch_mm()

Commit Message

WangYuli Sept. 9, 2024, 2:57 a.m. UTC

From: Andrea Parri <[e-mail]>

[ Upstream commit d6cfd1770f20392d7009ae1fdb04733794514fa9 ]

The membarrier system call requires a full memory barrier after storing
to rq->curr, before going back to user-space.  The barrier is only
needed when switching between processes: the barrier is implied by
mmdrop() when switching from kernel to userspace, and it's not needed
when switching from userspace to kernel.

Rely on the feature/mechanism ARCH_HAS_MEMBARRIER_CALLBACKS and on the
primitive membarrier_arch_switch_mm(), already adopted by the PowerPC
architecture, to insert the required barrier.

Fixes: fab957c11efe2f ("RISC-V: Atomic and Locking Code")
Signed-off-by: Andrea Parri <[e-mail]>
Reviewed-by: Mathieu Desnoyers <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]
Signed-off-by: Palmer Dabbelt <[e-mail]>
Signed-off-by: WangYuli <[e-mail]>
---
 MAINTAINERS                         |  2 +-
 arch/riscv/Kconfig                  |  1 +
 arch/riscv/include/asm/membarrier.h | 31 +++++++++++++++++++++++++++++
 arch/riscv/mm/context.c             |  2 ++
 kernel/sched/core.c                 |  5 +++--
 5 files changed, 38 insertions(+), 3 deletions(-)
 create mode 100644 arch/riscv/include/asm/membarrier.h

Comments

Greg KH Sept. 10, 2024, 7:32 a.m. UTC | #1

On Mon, Sep 09, 2024 at 10:57:01AM +0800, WangYuli wrote:
> From: Andrea Parri <[e-mail]>
> 
> [ Upstream commit d6cfd1770f20392d7009ae1fdb04733794514fa9 ]
> 
> The membarrier system call requires a full memory barrier after storing
> to rq->curr, before going back to user-space.  The barrier is only
> needed when switching between processes: the barrier is implied by
> mmdrop() when switching from kernel to userspace, and it's not needed
> when switching from userspace to kernel.
> 
> Rely on the feature/mechanism ARCH_HAS_MEMBARRIER_CALLBACKS and on the
> primitive membarrier_arch_switch_mm(), already adopted by the PowerPC
> architecture, to insert the required barrier.
> 
> Fixes: fab957c11efe2f ("RISC-V: Atomic and Locking Code")
> Signed-off-by: Andrea Parri <[e-mail]>
> Reviewed-by: Mathieu Desnoyers <[e-mail]>
> Link: https://lore.kernel.org/r/[e-mail]
> Signed-off-by: Palmer Dabbelt <[e-mail]>
> Signed-off-by: WangYuli <[e-mail]>
> ---
>  MAINTAINERS                         |  2 +-
>  arch/riscv/Kconfig                  |  1 +
>  arch/riscv/include/asm/membarrier.h | 31 +++++++++++++++++++++++++++++
>  arch/riscv/mm/context.c             |  2 ++
>  kernel/sched/core.c                 |  5 +++--
>  5 files changed, 38 insertions(+), 3 deletions(-)
>  create mode 100644 arch/riscv/include/asm/membarrier.h

Now queued up, thanks.

greg k-h

Alexandre Ghiti Sept. 10, 2024, 11:31 a.m. UTC | #2

Hi Greg,

On 10/09/2024 09:32, Greg KH wrote:
> On Mon, Sep 09, 2024 at 10:57:01AM +0800, WangYuli wrote:
>> From: Andrea Parri <[e-mail]>
>>
>> [ Upstream commit d6cfd1770f20392d7009ae1fdb04733794514fa9 ]
>>
>> The membarrier system call requires a full memory barrier after storing
>> to rq->curr, before going back to user-space.  The barrier is only
>> needed when switching between processes: the barrier is implied by
>> mmdrop() when switching from kernel to userspace, and it's not needed
>> when switching from userspace to kernel.
>>
>> Rely on the feature/mechanism ARCH_HAS_MEMBARRIER_CALLBACKS and on the
>> primitive membarrier_arch_switch_mm(), already adopted by the PowerPC
>> architecture, to insert the required barrier.
>>
>> Fixes: fab957c11efe2f ("RISC-V: Atomic and Locking Code")
>> Signed-off-by: Andrea Parri <[e-mail]>
>> Reviewed-by: Mathieu Desnoyers <[e-mail]>
>> Link: https://lore.kernel.org/r/[e-mail]
>> Signed-off-by: Palmer Dabbelt <[e-mail]>
>> Signed-off-by: WangYuli <[e-mail]>
>> ---
>>   MAINTAINERS                         |  2 +-
>>   arch/riscv/Kconfig                  |  1 +
>>   arch/riscv/include/asm/membarrier.h | 31 +++++++++++++++++++++++++++++
>>   arch/riscv/mm/context.c             |  2 ++
>>   kernel/sched/core.c                 |  5 +++--
>>   5 files changed, 38 insertions(+), 3 deletions(-)
>>   create mode 100644 arch/riscv/include/asm/membarrier.h
> Now queued up, thanks.


The original patch was merged in 6.9 and the Fixes tag points to a 
commit introduced in v4.15. So IIUC, this patch should have been 
backported "automatically" to the releases < 6.9 right? As stated in the 
documentation (process/stable-kernel-rules.html):

"Note, such tagging is unnecessary if the stable team can derive the 
appropriate versions from Fixes: tags."

Or did we miss something?

Thanks,

Alex


>
> greg k-h
>
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Greg KH Sept. 10, 2024, 11:58 a.m. UTC | #3

On Tue, Sep 10, 2024 at 01:31:04PM +0200, Alexandre Ghiti wrote:
> Hi Greg,
> 
> On 10/09/2024 09:32, Greg KH wrote:
> > On Mon, Sep 09, 2024 at 10:57:01AM +0800, WangYuli wrote:
> > > From: Andrea Parri <[e-mail]>
> > > 
> > > [ Upstream commit d6cfd1770f20392d7009ae1fdb04733794514fa9 ]
> > > 
> > > The membarrier system call requires a full memory barrier after storing
> > > to rq->curr, before going back to user-space.  The barrier is only
> > > needed when switching between processes: the barrier is implied by
> > > mmdrop() when switching from kernel to userspace, and it's not needed
> > > when switching from userspace to kernel.
> > > 
> > > Rely on the feature/mechanism ARCH_HAS_MEMBARRIER_CALLBACKS and on the
> > > primitive membarrier_arch_switch_mm(), already adopted by the PowerPC
> > > architecture, to insert the required barrier.
> > > 
> > > Fixes: fab957c11efe2f ("RISC-V: Atomic and Locking Code")
> > > Signed-off-by: Andrea Parri <[e-mail]>
> > > Reviewed-by: Mathieu Desnoyers <[e-mail]>
> > > Link: https://lore.kernel.org/r/[e-mail]
> > > Signed-off-by: Palmer Dabbelt <[e-mail]>
> > > Signed-off-by: WangYuli <[e-mail]>
> > > ---
> > >   MAINTAINERS                         |  2 +-
> > >   arch/riscv/Kconfig                  |  1 +
> > >   arch/riscv/include/asm/membarrier.h | 31 +++++++++++++++++++++++++++++
> > >   arch/riscv/mm/context.c             |  2 ++
> > >   kernel/sched/core.c                 |  5 +++--
> > >   5 files changed, 38 insertions(+), 3 deletions(-)
> > >   create mode 100644 arch/riscv/include/asm/membarrier.h
> > Now queued up, thanks.
> 
> 
> The original patch was merged in 6.9 and the Fixes tag points to a commit
> introduced in v4.15. So IIUC, this patch should have been backported
> "automatically" to the releases < 6.9 right? As stated in the documentation
> (process/stable-kernel-rules.html):
> 
> "Note, such tagging is unnecessary if the stable team can derive the
> appropriate versions from Fixes: tags."
> 
> Or did we miss something?

Yes, you didn't tag cc: stable at all in this commit, which is why we
did not see it.  The documentation says that :)

thanks,

greg k-h

Alexandre Ghiti Sept. 10, 2024, 12:35 p.m. UTC | #4

On 10/09/2024 13:58, Greg KH wrote:
> On Tue, Sep 10, 2024 at 01:31:04PM +0200, Alexandre Ghiti wrote:
>> Hi Greg,
>>
>> On 10/09/2024 09:32, Greg KH wrote:
>>> On Mon, Sep 09, 2024 at 10:57:01AM +0800, WangYuli wrote:
>>>> From: Andrea Parri <[e-mail]>
>>>>
>>>> [ Upstream commit d6cfd1770f20392d7009ae1fdb04733794514fa9 ]
>>>>
>>>> The membarrier system call requires a full memory barrier after storing
>>>> to rq->curr, before going back to user-space.  The barrier is only
>>>> needed when switching between processes: the barrier is implied by
>>>> mmdrop() when switching from kernel to userspace, and it's not needed
>>>> when switching from userspace to kernel.
>>>>
>>>> Rely on the feature/mechanism ARCH_HAS_MEMBARRIER_CALLBACKS and on the
>>>> primitive membarrier_arch_switch_mm(), already adopted by the PowerPC
>>>> architecture, to insert the required barrier.
>>>>
>>>> Fixes: fab957c11efe2f ("RISC-V: Atomic and Locking Code")
>>>> Signed-off-by: Andrea Parri <[e-mail]>
>>>> Reviewed-by: Mathieu Desnoyers <[e-mail]>
>>>> Link: https://lore.kernel.org/r/[e-mail]
>>>> Signed-off-by: Palmer Dabbelt <[e-mail]>
>>>> Signed-off-by: WangYuli <[e-mail]>
>>>> ---
>>>>    MAINTAINERS                         |  2 +-
>>>>    arch/riscv/Kconfig                  |  1 +
>>>>    arch/riscv/include/asm/membarrier.h | 31 +++++++++++++++++++++++++++++
>>>>    arch/riscv/mm/context.c             |  2 ++
>>>>    kernel/sched/core.c                 |  5 +++--
>>>>    5 files changed, 38 insertions(+), 3 deletions(-)
>>>>    create mode 100644 arch/riscv/include/asm/membarrier.h
>>> Now queued up, thanks.
>>
>> The original patch was merged in 6.9 and the Fixes tag points to a commit
>> introduced in v4.15. So IIUC, this patch should have been backported
>> "automatically" to the releases < 6.9 right? As stated in the documentation
>> (process/stable-kernel-rules.html):
>>
>> "Note, such tagging is unnecessary if the stable team can derive the
>> appropriate versions from Fixes: tags."
>>
>> Or did we miss something?
> Yes, you didn't tag cc: stable at all in this commit, which is why we
> did not see it.  The documentation says that :)


Ok, some patches seem to make it to stable without the cc: stable tag 
(like the one below for example), so I thought it was not necessary.

From: Alexandre Ghiti <[e-mail]>

[ Upstream commit 1ff95eb2bebda50c4c5406caaf201e0fcb24cc8f ]

RISCV_ALTERNATIVE_EARLY will issue sbi_ecall() very early in the boot
process, before the first memory mapping is setup so we can't have any
instrumentation happening here.

In addition, when the kernel is relocatable, we must also not issue any
relocation this early since they would have been patched virtually only.

So, instead of disabling instrumentation for the whole kernel/sbi.c file
and compiling it with -fno-pie, simply move __sbi_ecall() and
__sbi_base_ecall() into their own file where this is fixed.

Reported-by: Conor Dooley <[e-mail]>
Closes:https://lore.kernel.org/linux-riscv/20240813-pony-truck-3e7a83e9759e@spud/
Reported-by 
<https://lore.kernel.org/linux-riscv/20240813-pony-truck-3e7a83e9759e@spud/Reported-by>:[e-mail] 
<mailto:[e-mail]>
Closes:https://lore.kernel.org/linux-riscv/[e-mail]/ 
<https://lore.kernel.org/linux-riscv/[e-mail]/>
Fixes: 1745cfafebdf ("riscv: don't use global static vars to store 
alternative data")
Signed-off-by: Alexandre Ghiti <[e-mail]>
Link:https://lore.kernel.org/r/[e-mail] 
<https://lore.kernel.org/r/[e-mail]>
Signed-off-by: Palmer Dabbelt <[e-mail]>
Signed-off-by: Sasha Levin <[e-mail]>
---
  arch/riscv/include/asm/sbi.h | 20 ++++++++++-
  arch/riscv/kernel/Makefile   |  6 +++-
  arch/riscv/kernel/sbi.c      | 63 -----------------------------------
  arch/riscv/kernel/sbi_ecall.c | 48 ++++++++++++++++++++++++++
  4 files changed, 72 insertions(+), 65 deletions(-)
  create mode 100644 arch/riscv/kernel/sbi_ecall.c

I'll pay attention to that from now on.

Thanks,

Alex


>
> thanks,
>
> greg k-h
>
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Greg KH Sept. 10, 2024, 1:25 p.m. UTC | #5

On Tue, Sep 10, 2024 at 02:35:06PM +0200, Alexandre Ghiti wrote:
> 
> On 10/09/2024 13:58, Greg KH wrote:
> > On Tue, Sep 10, 2024 at 01:31:04PM +0200, Alexandre Ghiti wrote:
> > > Hi Greg,
> > > 
> > > On 10/09/2024 09:32, Greg KH wrote:
> > > > On Mon, Sep 09, 2024 at 10:57:01AM +0800, WangYuli wrote:
> > > > > From: Andrea Parri <[e-mail]>
> > > > > 
> > > > > [ Upstream commit d6cfd1770f20392d7009ae1fdb04733794514fa9 ]
> > > > > 
> > > > > The membarrier system call requires a full memory barrier after storing
> > > > > to rq->curr, before going back to user-space.  The barrier is only
> > > > > needed when switching between processes: the barrier is implied by
> > > > > mmdrop() when switching from kernel to userspace, and it's not needed
> > > > > when switching from userspace to kernel.
> > > > > 
> > > > > Rely on the feature/mechanism ARCH_HAS_MEMBARRIER_CALLBACKS and on the
> > > > > primitive membarrier_arch_switch_mm(), already adopted by the PowerPC
> > > > > architecture, to insert the required barrier.
> > > > > 
> > > > > Fixes: fab957c11efe2f ("RISC-V: Atomic and Locking Code")
> > > > > Signed-off-by: Andrea Parri <[e-mail]>
> > > > > Reviewed-by: Mathieu Desnoyers <[e-mail]>
> > > > > Link: https://lore.kernel.org/r/[e-mail]
> > > > > Signed-off-by: Palmer Dabbelt <[e-mail]>
> > > > > Signed-off-by: WangYuli <[e-mail]>
> > > > > ---
> > > > >    MAINTAINERS                         |  2 +-
> > > > >    arch/riscv/Kconfig                  |  1 +
> > > > >    arch/riscv/include/asm/membarrier.h | 31 +++++++++++++++++++++++++++++
> > > > >    arch/riscv/mm/context.c             |  2 ++
> > > > >    kernel/sched/core.c                 |  5 +++--
> > > > >    5 files changed, 38 insertions(+), 3 deletions(-)
> > > > >    create mode 100644 arch/riscv/include/asm/membarrier.h
> > > > Now queued up, thanks.
> > > 
> > > The original patch was merged in 6.9 and the Fixes tag points to a commit
> > > introduced in v4.15. So IIUC, this patch should have been backported
> > > "automatically" to the releases < 6.9 right? As stated in the documentation
> > > (process/stable-kernel-rules.html):
> > > 
> > > "Note, such tagging is unnecessary if the stable team can derive the
> > > appropriate versions from Fixes: tags."
> > > 
> > > Or did we miss something?
> > Yes, you didn't tag cc: stable at all in this commit, which is why we
> > did not see it.  The documentation says that :)
> 
> 
> Ok, some patches seem to make it to stable without the cc: stable tag (like
> the one below for example), so I thought it was not necessary.

Yes, they do, because many maintainers and developers don't follow the
normal rules, so we sweep the tree when we have a chance and do a "best
effort" backport at times.

But again, it is NOT guaranteed that this will happen, and even if it
does, you will NOT get emails saying the attempt-at-backporting fails,
if it fails, as obviously the developers involved weren't explicitly
asking for it to be backported.

So in short, just properly tag things for stable, and all will be fine.

thanks,

greg k-h

Alexandre Ghiti Sept. 10, 2024, 6:59 p.m. UTC | #6

On 10/09/2024 15:25, Greg KH wrote:
> On Tue, Sep 10, 2024 at 02:35:06PM +0200, Alexandre Ghiti wrote:
>> On 10/09/2024 13:58, Greg KH wrote:
>>> On Tue, Sep 10, 2024 at 01:31:04PM +0200, Alexandre Ghiti wrote:
>>>> Hi Greg,
>>>>
>>>> On 10/09/2024 09:32, Greg KH wrote:
>>>>> On Mon, Sep 09, 2024 at 10:57:01AM +0800, WangYuli wrote:
>>>>>> From: Andrea Parri <[e-mail]>
>>>>>>
>>>>>> [ Upstream commit d6cfd1770f20392d7009ae1fdb04733794514fa9 ]
>>>>>>
>>>>>> The membarrier system call requires a full memory barrier after storing
>>>>>> to rq->curr, before going back to user-space.  The barrier is only
>>>>>> needed when switching between processes: the barrier is implied by
>>>>>> mmdrop() when switching from kernel to userspace, and it's not needed
>>>>>> when switching from userspace to kernel.
>>>>>>
>>>>>> Rely on the feature/mechanism ARCH_HAS_MEMBARRIER_CALLBACKS and on the
>>>>>> primitive membarrier_arch_switch_mm(), already adopted by the PowerPC
>>>>>> architecture, to insert the required barrier.
>>>>>>
>>>>>> Fixes: fab957c11efe2f ("RISC-V: Atomic and Locking Code")
>>>>>> Signed-off-by: Andrea Parri <[e-mail]>
>>>>>> Reviewed-by: Mathieu Desnoyers <[e-mail]>
>>>>>> Link: https://lore.kernel.org/r/[e-mail]
>>>>>> Signed-off-by: Palmer Dabbelt <[e-mail]>
>>>>>> Signed-off-by: WangYuli <[e-mail]>
>>>>>> ---
>>>>>>     MAINTAINERS                         |  2 +-
>>>>>>     arch/riscv/Kconfig                  |  1 +
>>>>>>     arch/riscv/include/asm/membarrier.h | 31 +++++++++++++++++++++++++++++
>>>>>>     arch/riscv/mm/context.c             |  2 ++
>>>>>>     kernel/sched/core.c                 |  5 +++--
>>>>>>     5 files changed, 38 insertions(+), 3 deletions(-)
>>>>>>     create mode 100644 arch/riscv/include/asm/membarrier.h
>>>>> Now queued up, thanks.
>>>> The original patch was merged in 6.9 and the Fixes tag points to a commit
>>>> introduced in v4.15. So IIUC, this patch should have been backported
>>>> "automatically" to the releases < 6.9 right? As stated in the documentation
>>>> (process/stable-kernel-rules.html):
>>>>
>>>> "Note, such tagging is unnecessary if the stable team can derive the
>>>> appropriate versions from Fixes: tags."
>>>>
>>>> Or did we miss something?
>>> Yes, you didn't tag cc: stable at all in this commit, which is why we
>>> did not see it.  The documentation says that :)
>>
>> Ok, some patches seem to make it to stable without the cc: stable tag (like
>> the one below for example), so I thought it was not necessary.
> Yes, they do, because many maintainers and developers don't follow the
> normal rules, so we sweep the tree when we have a chance and do a "best
> effort" backport at times.
>
> But again, it is NOT guaranteed that this will happen, and even if it
> does, you will NOT get emails saying the attempt-at-backporting fails,
> if it fails, as obviously the developers involved weren't explicitly
> asking for it to be backported.
>
> So in short, just properly tag things for stable, and all will be fine.


Great, thank you, I'll make sure we tag the fixes to backport properly!

Thanks again,

Alex


>
> thanks,
>
> greg k-h

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index f09415b2b3c5..ae4c0cec5073 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13702,7 +13702,7 @@  M:	Mathieu Desnoyers <[e-mail]>
 M:	"Paul E. McKenney" <[e-mail]>
 L:	[e-mail]
 S:	Supported
-F:	arch/powerpc/include/asm/membarrier.h
+F:	arch/*/include/asm/membarrier.h
 F:	include/uapi/linux/membarrier.h
 F:	kernel/sched/membarrier.c
 
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index c785a0200573..1df4afccc381 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -27,6 +27,7 @@  config RISCV
 	select ARCH_HAS_GCOV_PROFILE_ALL
 	select ARCH_HAS_GIGANTIC_PAGE
 	select ARCH_HAS_KCOV
+	select ARCH_HAS_MEMBARRIER_CALLBACKS
 	select ARCH_HAS_MMIOWB
 	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	select ARCH_HAS_PMEM_API
diff --git a/arch/riscv/include/asm/membarrier.h b/arch/riscv/include/asm/membarrier.h
new file mode 100644
index 000000000000..6c016ebb5020
--- /dev/null
+++ b/arch/riscv/include/asm/membarrier.h
@@ -0,0 +1,31 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _ASM_RISCV_MEMBARRIER_H
+#define _ASM_RISCV_MEMBARRIER_H
+
+static inline void membarrier_arch_switch_mm(struct mm_struct *prev,
+					     struct mm_struct *next,
+					     struct task_struct *tsk)
+{
+	/*
+	 * Only need the full barrier when switching between processes.
+	 * Barrier when switching from kernel to userspace is not
+	 * required here, given that it is implied by mmdrop(). Barrier
+	 * when switching from userspace to kernel is not needed after
+	 * store to rq->curr.
+	 */
+	if (IS_ENABLED(CONFIG_SMP) &&
+	    likely(!(atomic_read(&next->membarrier_state) &
+		     (MEMBARRIER_STATE_PRIVATE_EXPEDITED |
+		      MEMBARRIER_STATE_GLOBAL_EXPEDITED)) || !prev))
+		return;
+
+	/*
+	 * The membarrier system call requires a full memory barrier
+	 * after storing to rq->curr, before going back to user-space.
+	 * Matches a full barrier in the proximity of the membarrier
+	 * system call entry.
+	 */
+	smp_mb();
+}
+
+#endif /* _ASM_RISCV_MEMBARRIER_H */
diff --git a/arch/riscv/mm/context.c b/arch/riscv/mm/context.c
index 217fd4de6134..ba8eb3944687 100644
--- a/arch/riscv/mm/context.c
+++ b/arch/riscv/mm/context.c
@@ -323,6 +323,8 @@  void switch_mm(struct mm_struct *prev, struct mm_struct *next,
 	if (unlikely(prev == next))
 		return;
 
+	membarrier_arch_switch_mm(prev, next, task);
+
 	/*
 	 * Mark the current MM context as inactive, and the next as
 	 * active.  This is at least used by the icache flushing
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 97571d390f18..9b406d988654 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6679,8 +6679,9 @@  static void __sched notrace __schedule(unsigned int sched_mode)
 		 *
 		 * Here are the schemes providing that barrier on the
 		 * various architectures:
-		 * - mm ? switch_mm() : mmdrop() for x86, s390, sparc, PowerPC.
-		 *   switch_mm() rely on membarrier_arch_switch_mm() on PowerPC.
+		 * - mm ? switch_mm() : mmdrop() for x86, s390, sparc, PowerPC,
+		 *   RISC-V.  switch_mm() relies on membarrier_arch_switch_mm()
+		 *   on PowerPC and on RISC-V.
 		 * - finish_lock_switch() for weakly-ordered
 		 *   architectures where spin_unlock is a full barrier,
 		 * - switch_to() for arm64 (weakly-ordered, spin_unlock