Re: [PATCH v2 1/4] powerpc/64: handle MSR EE and RI in interrupt entry wrapper
Excerpts from Daniel Axtens's message of August 27, 2021 5:31 pm:
> Hi,
>
>> Similarly to the system call change in the previous patch, the mtmsrd to
>
> I don't actually know what patch this was - I assume it's from a series
> that has since been applied?
Good catch yes that used to be in the same series. Now merged, it's
dd152f, I'll update the changelog.
>> enable RI can be combined with the mtmsrd to enable EE for interrupts
>> which enable the latter, which tends to be the important synchronous
>> interrupts (i.e., page faults).
>>
>> Do this by enabling EE and RI together at the beginning of the entry
>> wrapper if PACA_IRQ_HARD_DIS is clear, and just enabling RI if it is set
>> (which means something wanted EE=0).
>
>
>> diff --git a/arch/powerpc/include/asm/interrupt.h
>> b/arch/powerpc/include/asm/interrupt.h
>> index 6b800d3e2681..e3228a911b35 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -148,9 +148,21 @@ static inline void interrupt_enter_prepare(struct
>> pt_regs *regs, struct interrup
>> #endif
>>
>> #ifdef CONFIG_PPC64
>> -if (irq_soft_mask_set_return(IRQS_ALL_DISABLED) == IRQS_ENABLED)
>> +bool trace_enable = false;
>> +
>> +if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS)) {
>> +if (irq_soft_mask_set_return(IRQS_DISABLED) == IRQS_ENABLED)
>
> In the previous code we had IRQS_ALL_DISABLED, now we just have
> IRQS_DISABLED. Is that intentional?
Hmm, no it should be IRQS_ALL_DISABLED. It doesn't matter much in
practice I think because MSR[EE] is disabled, but obviously shouldn't
be changed by this patch (and shouldn't be changed at all IMO having
ALL_DISABLED).
>
>> +trace_enable = true;
>> +} else {
>> +irq_soft_mask_set(IRQS_DISABLED);
>> +}
>> +/* If the interrupt was taken with HARD_DIS set, don't enable MSR[EE] */
>> +if (local_paca->irq_happened & PACA_IRQ_HARD_DIS)
>> +__hard_RI_enable();
>> +else
>> +__hard_irq_enable();
>> +if (trace_enable)
>> trace_hardirqs_off();
>> -local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
>>
>> if (user_mode(regs)) {
>> CT_WARN_ON(ct_state() != CONTEXT_USER);
>
>
>> @@ -901,11 +892,6 @@ INT_DEFINE_BEGIN(system_reset)
>> IVEC=0x100
>> IAREA=PACA_EXNMI
>> IVIRT=0 /* no virt entry point */
>> -/*
>> - * MSR_RI is not enabled, because PACA_EXNMI and nmi stack is
>> - * being used, so a nested NMI exception would corrupt it.
>> - */
>> -ISET_RI=0
>> ISTACK=0
>> IKVM_REAL=1
>> INT_DEFINE_END(system_reset)
>
>> @@ -986,8 +972,6 @@ EXC_COMMON_BEGIN(system_reset_common)
>
> Right before this change, there's a comment that reads:
>
> /*
>* Increment paca->in_nmi then enable MSR_RI. SLB or MCE will be able
>* to recover, but nested NMI will notice in_nmi and not recover
> ...
>
> You've taken out the bit that enables MSR_RI, which means the comment is
> no longer accurate.
Ah yep, that should be okay because we moved the RI enable to the NMI
entry wrapper. Comment just needs a tweak.
>
> Beyond that, I'm still trying to follow all the various changes in code
> flows. It seems to make at least vague sense: we move the setting of
> MSR_RI from the early asm to interrupt*enter_prepare. I'm just
> struggling to convince myself that when we hit the underlying handler
> that the RI states all line up.
Thanks,
Nick
Re: [PATCH v2 1/4] powerpc/64: handle MSR EE and RI in interrupt entry wrapper
Hi,
> Similarly to the system call change in the previous patch, the mtmsrd to
I don't actually know what patch this was - I assume it's from a series
that has since been applied?
> enable RI can be combined with the mtmsrd to enable EE for interrupts
> which enable the latter, which tends to be the important synchronous
> interrupts (i.e., page faults).
>
> Do this by enabling EE and RI together at the beginning of the entry
> wrapper if PACA_IRQ_HARD_DIS is clear, and just enabling RI if it is set
> (which means something wanted EE=0).
> diff --git a/arch/powerpc/include/asm/interrupt.h
> b/arch/powerpc/include/asm/interrupt.h
> index 6b800d3e2681..e3228a911b35 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -148,9 +148,21 @@ static inline void interrupt_enter_prepare(struct
> pt_regs *regs, struct interrup
> #endif
>
> #ifdef CONFIG_PPC64
> - if (irq_soft_mask_set_return(IRQS_ALL_DISABLED) == IRQS_ENABLED)
> + bool trace_enable = false;
> +
> + if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS)) {
> + if (irq_soft_mask_set_return(IRQS_DISABLED) == IRQS_ENABLED)
In the previous code we had IRQS_ALL_DISABLED, now we just have
IRQS_DISABLED. Is that intentional?
> + trace_enable = true;
> + } else {
> + irq_soft_mask_set(IRQS_DISABLED);
> + }
> + /* If the interrupt was taken with HARD_DIS set, don't enable MSR[EE] */
> + if (local_paca->irq_happened & PACA_IRQ_HARD_DIS)
> + __hard_RI_enable();
> + else
> + __hard_irq_enable();
> + if (trace_enable)
> trace_hardirqs_off();
> - local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
>
> if (user_mode(regs)) {
> CT_WARN_ON(ct_state() != CONTEXT_USER);
> @@ -901,11 +892,6 @@ INT_DEFINE_BEGIN(system_reset)
> IVEC=0x100
> IAREA=PACA_EXNMI
> IVIRT=0 /* no virt entry point */
> - /*
> - * MSR_RI is not enabled, because PACA_EXNMI and nmi stack is
> - * being used, so a nested NMI exception would corrupt it.
> - */
> - ISET_RI=0
> ISTACK=0
> IKVM_REAL=1
> INT_DEFINE_END(system_reset)
> @@ -986,8 +972,6 @@ EXC_COMMON_BEGIN(system_reset_common)
Right before this change, there's a comment that reads:
/*
* Increment paca->in_nmi then enable MSR_RI. SLB or MCE will be able
* to recover, but nested NMI will notice in_nmi and not recover
...
You've taken out the bit that enables MSR_RI, which means the comment is
no longer accurate.
Beyond that, I'm still trying to follow all the various changes in code
flows. It seems to make at least vague sense: we move the setting of
MSR_RI from the early asm to interrupt*enter_prepare. I'm just
struggling to convince myself that when we hit the underlying handler
that the RI states all line up.
Kind regards,
Daniel
[PATCH v2 1/4] powerpc/64: handle MSR EE and RI in interrupt entry wrapper
Similarly to the system call change in the previous patch, the mtmsrd to
enable RI can be combined with the mtmsrd to enable EE for interrupts
which enable the latter, which tends to be the important synchronous
interrupts (i.e., page faults).
Do this by enabling EE and RI together at the beginning of the entry
wrapper if PACA_IRQ_HARD_DIS is clear, and just enabling RI if it is set
(which means something wanted EE=0).
Asynchronous interrupts set PACA_IRQ_HARD_DIS, but synchronous ones
leave it unchanged, so by default they always get EE=1 unless they
interrupt a caller that has hard disabled. When the sync interrupt
later calls interrupt_cond_local_irq_enable(), that will not require
another mtmsrd because we already enabled here.
64e is conceptually unchanged, but it also sets MSR[EE]=1 now in the
interrupt wrapper for synchronous interrupts with the same code.
On 64s, saves one mtmsrd L=1 for synchronous interrupts on 64s, which
saves about 20 cycles. For kernel-mode interrupts, both synchronous and
asynchronous, this saves an additional ~40 cycles due to the mtmsrd
being moved ahead of mfspr SPRN_AMR, which prevents a SPR scoreboard
stall (on POWER9).
Signed-off-by: Nicholas Piggin
---
arch/powerpc/include/asm/interrupt.h | 31
arch/powerpc/kernel/exceptions-64s.S | 30 ---
arch/powerpc/kernel/fpu.S| 5 +
arch/powerpc/kernel/vector.S | 8 +++
4 files changed, 40 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/include/asm/interrupt.h
b/arch/powerpc/include/asm/interrupt.h
index 6b800d3e2681..e3228a911b35 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -148,9 +148,21 @@ static inline void interrupt_enter_prepare(struct pt_regs
*regs, struct interrup
#endif
#ifdef CONFIG_PPC64
- if (irq_soft_mask_set_return(IRQS_ALL_DISABLED) == IRQS_ENABLED)
+ bool trace_enable = false;
+
+ if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS)) {
+ if (irq_soft_mask_set_return(IRQS_DISABLED) == IRQS_ENABLED)
+ trace_enable = true;
+ } else {
+ irq_soft_mask_set(IRQS_DISABLED);
+ }
+ /* If the interrupt was taken with HARD_DIS set, don't enable MSR[EE] */
+ if (local_paca->irq_happened & PACA_IRQ_HARD_DIS)
+ __hard_RI_enable();
+ else
+ __hard_irq_enable();
+ if (trace_enable)
trace_hardirqs_off();
- local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
if (user_mode(regs)) {
CT_WARN_ON(ct_state() != CONTEXT_USER);
@@ -200,13 +212,20 @@ static inline void interrupt_exit_prepare(struct pt_regs
*regs, struct interrupt
static inline void interrupt_async_enter_prepare(struct pt_regs *regs, struct
interrupt_state *state)
{
+#ifdef CONFIG_PPC64
+ /* Ensure interrupt_enter_prepare does not enable MSR[EE] */
+ local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
+#endif
+ interrupt_enter_prepare(regs, state);
#ifdef CONFIG_PPC_BOOK3S_64
+ /*
+* MSR[RI] is only enabled after interrupt_enter_prepare, so this
+* thread flags access has to come afterward.
+*/
if (cpu_has_feature(CPU_FTR_CTRL) &&
!test_thread_local_flags(_TLF_RUNLATCH))
__ppc64_runlatch_on();
#endif
-
- interrupt_enter_prepare(regs, state);
irq_enter();
}
@@ -273,6 +292,8 @@ static inline void interrupt_nmi_enter_prepare(struct
pt_regs *regs, struct inte
if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
BUG_ON(!arch_irq_disabled_regs(regs) && !(regs->msr & MSR_EE));
+ __hard_RI_enable();
+
/* Don't do any per-CPU operations until interrupt state is fixed */
if (nmi_disables_ftrace(regs)) {
@@ -370,6 +391,8 @@ interrupt_handler long func(struct pt_regs *regs)
\
{ \
long ret; \
\
+ __hard_RI_enable(); \
+ \
ret = ##func (regs);\
\
return ret; \
diff --git a/arch/powerpc/kernel/exceptions-64s.S
b/arch/powerpc/kernel/exceptions-64s.S
index 4aec59a77d4c..69a472c38f62 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -113,7 +113,6 @@ name:
#define IISIDE .L_IISIDE_\name\() /* Uses SRR0/1 not DAR/DSISR */
#define IDAR .L_IDAR_\name\()/* Uses DAR (or SRR0) */
#define IDSISR .L_IDSISR_\name\() /* Uses DSISR (or SR
