| Message ID | df36c6205ab64326fb1b991993c82057e92ace2f.1685955214.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0eb089a72fda3f7969e6277804bde75dc1474a14 |
| Headers | show |
| Series | powerpc/interrupt: Don't read MSR from interrupt_exit_kernel_prepare() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
On Mon Jun 5, 2023 at 6:55 PM AEST, Christophe Leroy wrote: > A disassembly of interrupt_exit_kernel_prepare() shows a useless read > of MSR register. This is shown by r9 being re-used immediately without > doing anything with the value read. > > c000e0e0: 60 00 00 00 nop > c000e0e4: 7d 3a c2 a6 mfmd_ap r9 > c000e0e8: 7d 20 00 a6 mfmsr r9 > c000e0ec: 7c 51 13 a6 mtspr 81,r2 > c000e0f0: 81 3f 00 84 lwz r9,132(r31) > c000e0f4: 71 29 80 00 andi. r9,r9,32768 > > This is due to the use of local_irq_save(). The flags read by > local_irq_save() are never used, use local_irq_disable() instead. I did have a patch that warns if you do a local_irq_disable() when irqs are disabled which is why I did this, but it is kind of silly. You could do 'if (!irqs_disabled()) local_irq_disable()' Unfortunately that adds another branch but if it is not taken frequently then maybe avoiding the mtMSR/EID would make it a win? If you don't change that I might end up doing it if I can get that warning patch merged (needs a few core kernel changes). I wonder how much that would help local_irq_save too, if interrupts are already disabled then avoid the mt? Maybe those things are not very costly on smaller in-order cores. Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > Fixes: 13799748b957 ("powerpc/64: use interrupt restart table to speed up return from interrupt") > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/kernel/interrupt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c > index e34c72285b4e..f3fc5fe919d9 100644 > --- a/arch/powerpc/kernel/interrupt.c > +++ b/arch/powerpc/kernel/interrupt.c > @@ -368,7 +368,6 @@ void preempt_schedule_irq(void); > > notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs) > { > - unsigned long flags; > unsigned long ret = 0; > unsigned long kuap; > bool stack_store = read_thread_flags() & _TIF_EMULATE_STACK_STORE; > @@ -392,7 +391,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs) > > kuap = kuap_get_and_assert_locked(); > > - local_irq_save(flags); > + local_irq_disable(); > > if (!arch_irq_disabled_regs(regs)) { > /* Returning to a kernel context with local irqs enabled. */ > -- > 2.40.1
On Mon, 05 Jun 2023 10:55:26 +0200, Christophe Leroy wrote: > A disassembly of interrupt_exit_kernel_prepare() shows a useless read > of MSR register. This is shown by r9 being re-used immediately without > doing anything with the value read. > > c000e0e0: 60 00 00 00 nop > c000e0e4: 7d 3a c2 a6 mfmd_ap r9 > c000e0e8: 7d 20 00 a6 mfmsr r9 > c000e0ec: 7c 51 13 a6 mtspr 81,r2 > c000e0f0: 81 3f 00 84 lwz r9,132(r31) > c000e0f4: 71 29 80 00 andi. r9,r9,32768 > > [...] Applied to powerpc/next. [1/1] powerpc/interrupt: Don't read MSR from interrupt_exit_kernel_prepare() https://git.kernel.org/powerpc/c/0eb089a72fda3f7969e6277804bde75dc1474a14 cheers
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c index e34c72285b4e..f3fc5fe919d9 100644 --- a/arch/powerpc/kernel/interrupt.c +++ b/arch/powerpc/kernel/interrupt.c @@ -368,7 +368,6 @@ void preempt_schedule_irq(void); notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs) { - unsigned long flags; unsigned long ret = 0; unsigned long kuap; bool stack_store = read_thread_flags() & _TIF_EMULATE_STACK_STORE; @@ -392,7 +391,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs) kuap = kuap_get_and_assert_locked(); - local_irq_save(flags); + local_irq_disable(); if (!arch_irq_disabled_regs(regs)) { /* Returning to a kernel context with local irqs enabled. */
A disassembly of interrupt_exit_kernel_prepare() shows a useless read of MSR register. This is shown by r9 being re-used immediately without doing anything with the value read. c000e0e0: 60 00 00 00 nop c000e0e4: 7d 3a c2 a6 mfmd_ap r9 c000e0e8: 7d 20 00 a6 mfmsr r9 c000e0ec: 7c 51 13 a6 mtspr 81,r2 c000e0f0: 81 3f 00 84 lwz r9,132(r31) c000e0f4: 71 29 80 00 andi. r9,r9,32768 This is due to the use of local_irq_save(). The flags read by local_irq_save() are never used, use local_irq_disable() instead. Fixes: 13799748b957 ("powerpc/64: use interrupt restart table to speed up return from interrupt") Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/kernel/interrupt.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)