* [PATCH 1/2] powerpc/tm: Move tm_enable definition
@ 2018-10-01 19:47 Breno Leitao
2018-10-01 19:47 ` [PATCH 2/2] powerpc/tm: Avoid SPR flush if TM is disabled Breno Leitao
2021-06-11 7:32 ` [PATCH 1/2] powerpc/tm: Move tm_enable definition Christophe Leroy
0 siblings, 2 replies; 5+ messages in thread
From: Breno Leitao @ 2018-10-01 19:47 UTC (permalink / raw
To: linuxppc-dev; +Cc: Breno Leitao, mikey, gromero
The goal of this patch is to move function tm_enabled() to tm.h in order
to allow this function to be used by other files as an inline function.
This patch also removes the double inclusion of tm.h in the traps.c
source code. One inclusion is inside a CONFIG_PPC64 ifdef block, and
another one is in the generic part. This double inclusion causes a
redefinition of tm_enable(), that is why it is being fixed here.
There is generic code (non CONFIG_PPC64, thus, non
CONFIG_PPC_TRANSACTIONAL_MEM also) using some TM definitions, which
explains why tm.h is being imported in the generic code. This is
not correct, and this code is now surrounded by a
CONFIG_PPC_TRANSACTIONAL_MEM ifdef block.
These ifdef inclusion will avoid calling tm_abort_check() completely,
but it is not a problem since this function is just returning 'false' if
CONFIG_PPC_TRANSACTIONAL_MEM is not defined.
Signed-off-by: Breno Leitao <leitao@debian.org>
---
arch/powerpc/include/asm/tm.h | 5 +++++
arch/powerpc/kernel/process.c | 5 -----
arch/powerpc/kernel/traps.c | 5 ++++-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/tm.h b/arch/powerpc/include/asm/tm.h
index e94f6db5e367..646d45a2aaae 100644
--- a/arch/powerpc/include/asm/tm.h
+++ b/arch/powerpc/include/asm/tm.h
@@ -19,4 +19,9 @@ extern void tm_restore_sprs(struct thread_struct *thread);
extern bool tm_suspend_disabled;
+static inline bool tm_enabled(struct task_struct *tsk)
+{
+ return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
+}
+
#endif /* __ASSEMBLY__ */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 913c5725cdb2..c1ca2451fa3b 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -862,11 +862,6 @@ static inline bool hw_brk_match(struct arch_hw_breakpoint *a,
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-static inline bool tm_enabled(struct task_struct *tsk)
-{
- return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
-}
-
static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
{
/*
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index c85adb858271..a3d6298b8074 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -64,7 +64,6 @@
#include <asm/rio.h>
#include <asm/fadump.h>
#include <asm/switch_to.h>
-#include <asm/tm.h>
#include <asm/debug.h>
#include <asm/asm-prototypes.h>
#include <asm/hmi.h>
@@ -1276,9 +1275,11 @@ static int emulate_instruction(struct pt_regs *regs)
/* Emulate load/store string insn. */
if ((instword & PPC_INST_STRING_GEN_MASK) == PPC_INST_STRING) {
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
if (tm_abort_check(regs,
TM_CAUSE_EMULATE | TM_CAUSE_PERSISTENT))
return -EINVAL;
+#endif
PPC_WARN_EMULATED(string, regs);
return emulate_string_inst(regs, instword);
}
@@ -1508,8 +1509,10 @@ void alignment_exception(struct pt_regs *regs)
if (!arch_irq_disabled_regs(regs))
local_irq_enable();
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
if (tm_abort_check(regs, TM_CAUSE_ALIGNMENT | TM_CAUSE_PERSISTENT))
goto bail;
+#endif
/* we don't implement logging of alignment exceptions */
if (!(current->thread.align_ctl & PR_UNALIGN_SIGBUS))
--
2.19.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] powerpc/tm: Avoid SPR flush if TM is disabled
2018-10-01 19:47 [PATCH 1/2] powerpc/tm: Move tm_enable definition Breno Leitao
@ 2018-10-01 19:47 ` Breno Leitao
2018-10-02 0:05 ` Michael Neuling
2021-06-11 7:35 ` Christophe Leroy
2021-06-11 7:32 ` [PATCH 1/2] powerpc/tm: Move tm_enable definition Christophe Leroy
1 sibling, 2 replies; 5+ messages in thread
From: Breno Leitao @ 2018-10-01 19:47 UTC (permalink / raw
To: linuxppc-dev; +Cc: Breno Leitao, mikey, gromero
There is a bug in the flush_tmregs_to_thread() function, where it forces
TM SPRs to be saved to the thread even if the TM facility is disabled.
This bug could be reproduced using a simple test case:
mtspr(SPRN_TEXASR, XX);
sleep until load_tm == 0
cause a coredump
read SPRN_TEXASR in the coredump
In this case, the coredump may contain an invalid SPR, because the
current code is flushing live SPRs (Used by the last thread with TM
active) into the current thread, overwriting the latest SPRs (which were
valid).
This patch checks if TM is enabled for current task before
saving the SPRs, otherwise, the TM is lazily disabled and the thread
value is already up-to-date and could be used directly, and saving is
not required.
Fixes: cd63f3cf1d5 ("powerpc/tm: Fix saving of TM SPRs in core dump")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
arch/powerpc/kernel/ptrace.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 9667666eb18e..e0a2ee865032 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -138,7 +138,12 @@ static void flush_tmregs_to_thread(struct task_struct *tsk)
if (MSR_TM_SUSPENDED(mfmsr())) {
tm_reclaim_current(TM_CAUSE_SIGNAL);
- } else {
+ } else if (tm_enabled(tsk)) {
+ /*
+ * Only flush TM SPRs to the thread if TM was enabled,
+ * otherwise (TM lazily disabled), the thread already
+ * contains the latest SPR value
+ */
tm_enable();
tm_save_sprs(&(tsk->thread));
}
--
2.19.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] powerpc/tm: Avoid SPR flush if TM is disabled
2018-10-01 19:47 ` [PATCH 2/2] powerpc/tm: Avoid SPR flush if TM is disabled Breno Leitao
@ 2018-10-02 0:05 ` Michael Neuling
2021-06-11 7:35 ` Christophe Leroy
1 sibling, 0 replies; 5+ messages in thread
From: Michael Neuling @ 2018-10-02 0:05 UTC (permalink / raw
To: Breno Leitao, linuxppc-dev; +Cc: gromero
On Mon, 2018-10-01 at 16:47 -0300, Breno Leitao wrote:
> There is a bug in the flush_tmregs_to_thread() function, where it forces
> TM SPRs to be saved to the thread even if the TM facility is disabled.
>
> This bug could be reproduced using a simple test case:
>
> mtspr(SPRN_TEXASR, XX);
> sleep until load_tm == 0
> cause a coredump
> read SPRN_TEXASR in the coredump
>
> In this case, the coredump may contain an invalid SPR, because the
> current code is flushing live SPRs (Used by the last thread with TM
> active) into the current thread, overwriting the latest SPRs (which were
> valid).
>
> This patch checks if TM is enabled for current task before
> saving the SPRs, otherwise, the TM is lazily disabled and the thread
> value is already up-to-date and could be used directly, and saving is
> not required.
Acked-by: Michael Neuling <mikey@neuling.org>
Breno, can you send your selftest upstream that also?
Mikey
>
> Fixes: cd63f3cf1d5 ("powerpc/tm: Fix saving of TM SPRs in core dump")
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> arch/powerpc/kernel/ptrace.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 9667666eb18e..e0a2ee865032 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -138,7 +138,12 @@ static void flush_tmregs_to_thread(struct task_struct
> *tsk)
>
> if (MSR_TM_SUSPENDED(mfmsr())) {
> tm_reclaim_current(TM_CAUSE_SIGNAL);
> - } else {
> + } else if (tm_enabled(tsk)) {
> + /*
> + * Only flush TM SPRs to the thread if TM was enabled,
> + * otherwise (TM lazily disabled), the thread already
> + * contains the latest SPR value
> + */
> tm_enable();
> tm_save_sprs(&(tsk->thread));
> }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] powerpc/tm: Move tm_enable definition
2018-10-01 19:47 [PATCH 1/2] powerpc/tm: Move tm_enable definition Breno Leitao
2018-10-01 19:47 ` [PATCH 2/2] powerpc/tm: Avoid SPR flush if TM is disabled Breno Leitao
@ 2021-06-11 7:32 ` Christophe Leroy
1 sibling, 0 replies; 5+ messages in thread
From: Christophe Leroy @ 2021-06-11 7:32 UTC (permalink / raw
To: Breno Leitao, linuxppc-dev; +Cc: mikey, gromero
Le 01/10/2018 à 21:47, Breno Leitao a écrit :
> The goal of this patch is to move function tm_enabled() to tm.h in order
> to allow this function to be used by other files as an inline function.
>
> This patch also removes the double inclusion of tm.h in the traps.c
> source code. One inclusion is inside a CONFIG_PPC64 ifdef block, and
> another one is in the generic part. This double inclusion causes a
> redefinition of tm_enable(), that is why it is being fixed here.
>
> There is generic code (non CONFIG_PPC64, thus, non
> CONFIG_PPC_TRANSACTIONAL_MEM also) using some TM definitions, which
> explains why tm.h is being imported in the generic code. This is
> not correct, and this code is now surrounded by a
> CONFIG_PPC_TRANSACTIONAL_MEM ifdef block.
You should leave the generic inclusion and remove the second one.
Don't use #ifdef blocks when not necessary, see
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation
>
> These ifdef inclusion will avoid calling tm_abort_check() completely,
> but it is not a problem since this function is just returning 'false' if
> CONFIG_PPC_TRANSACTIONAL_MEM is not defined.
As tm_abort_check() always returns false, gcc will see it and will optimise-out the check by itself,
no worry.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> arch/powerpc/include/asm/tm.h | 5 +++++
> arch/powerpc/kernel/process.c | 5 -----
> arch/powerpc/kernel/traps.c | 5 ++++-
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/tm.h b/arch/powerpc/include/asm/tm.h
> index e94f6db5e367..646d45a2aaae 100644
> --- a/arch/powerpc/include/asm/tm.h
> +++ b/arch/powerpc/include/asm/tm.h
> @@ -19,4 +19,9 @@ extern void tm_restore_sprs(struct thread_struct *thread);
>
> extern bool tm_suspend_disabled;
>
> +static inline bool tm_enabled(struct task_struct *tsk)
> +{
> + return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
> +}
> +
> #endif /* __ASSEMBLY__ */
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 913c5725cdb2..c1ca2451fa3b 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -862,11 +862,6 @@ static inline bool hw_brk_match(struct arch_hw_breakpoint *a,
>
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>
> -static inline bool tm_enabled(struct task_struct *tsk)
> -{
> - return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
> -}
> -
> static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
> {
> /*
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index c85adb858271..a3d6298b8074 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -64,7 +64,6 @@
> #include <asm/rio.h>
> #include <asm/fadump.h>
> #include <asm/switch_to.h>
> -#include <asm/tm.h>
> #include <asm/debug.h>
> #include <asm/asm-prototypes.h>
> #include <asm/hmi.h>
> @@ -1276,9 +1275,11 @@ static int emulate_instruction(struct pt_regs *regs)
>
> /* Emulate load/store string insn. */
> if ((instword & PPC_INST_STRING_GEN_MASK) == PPC_INST_STRING) {
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
This ifdef is not needed, tm_abort_check() returns false when CONFIG_PPC_TRANSACTIONAL_MEM is not
defined.
> if (tm_abort_check(regs,
> TM_CAUSE_EMULATE | TM_CAUSE_PERSISTENT))
> return -EINVAL;
> +#endif
> PPC_WARN_EMULATED(string, regs);
> return emulate_string_inst(regs, instword);
> }
> @@ -1508,8 +1509,10 @@ void alignment_exception(struct pt_regs *regs)
> if (!arch_irq_disabled_regs(regs))
> local_irq_enable();
>
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
Same here.
> if (tm_abort_check(regs, TM_CAUSE_ALIGNMENT | TM_CAUSE_PERSISTENT))
> goto bail;
> +#endif
>
> /* we don't implement logging of alignment exceptions */
> if (!(current->thread.align_ctl & PR_UNALIGN_SIGBUS))
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] powerpc/tm: Avoid SPR flush if TM is disabled
2018-10-01 19:47 ` [PATCH 2/2] powerpc/tm: Avoid SPR flush if TM is disabled Breno Leitao
2018-10-02 0:05 ` Michael Neuling
@ 2021-06-11 7:35 ` Christophe Leroy
1 sibling, 0 replies; 5+ messages in thread
From: Christophe Leroy @ 2021-06-11 7:35 UTC (permalink / raw
To: Breno Leitao, linuxppc-dev; +Cc: mikey, gromero
Le 01/10/2018 à 21:47, Breno Leitao a écrit :
> There is a bug in the flush_tmregs_to_thread() function, where it forces
> TM SPRs to be saved to the thread even if the TM facility is disabled.
>
> This bug could be reproduced using a simple test case:
>
> mtspr(SPRN_TEXASR, XX);
> sleep until load_tm == 0
> cause a coredump
> read SPRN_TEXASR in the coredump
>
> In this case, the coredump may contain an invalid SPR, because the
> current code is flushing live SPRs (Used by the last thread with TM
> active) into the current thread, overwriting the latest SPRs (which were
> valid).
>
> This patch checks if TM is enabled for current task before
> saving the SPRs, otherwise, the TM is lazily disabled and the thread
> value is already up-to-date and could be used directly, and saving is
> not required.
If this patch is still applicable, it has to be rebased.
>
> Fixes: cd63f3cf1d5 ("powerpc/tm: Fix saving of TM SPRs in core dump")
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> arch/powerpc/kernel/ptrace.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 9667666eb18e..e0a2ee865032 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -138,7 +138,12 @@ static void flush_tmregs_to_thread(struct task_struct *tsk)
>
> if (MSR_TM_SUSPENDED(mfmsr())) {
> tm_reclaim_current(TM_CAUSE_SIGNAL);
> - } else {
> + } else if (tm_enabled(tsk)) {
> + /*
> + * Only flush TM SPRs to the thread if TM was enabled,
> + * otherwise (TM lazily disabled), the thread already
> + * contains the latest SPR value
> + */
> tm_enable();
> tm_save_sprs(&(tsk->thread));
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-11 7:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-01 19:47 [PATCH 1/2] powerpc/tm: Move tm_enable definition Breno Leitao
2018-10-01 19:47 ` [PATCH 2/2] powerpc/tm: Avoid SPR flush if TM is disabled Breno Leitao
2018-10-02 0:05 ` Michael Neuling
2021-06-11 7:35 ` Christophe Leroy
2021-06-11 7:32 ` [PATCH 1/2] powerpc/tm: Move tm_enable definition Christophe Leroy
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 read-only IMAP folder(s) and NNTP newsgroup(s).