Excerpts from Christophe Leroy's message of June 30, 2021 5:56 pm:
>
>
> Le 30/06/2021 à 09:46, Nicholas Piggin a écrit :
>> The implicit soft-masking to speed up interrupt return was going to be
>> used by 64e as well, but it has not been extensively tested on that
>> platform and is not considered ready. It was intended to be disabled
>> before merge. Disable it for now.
>>
>> Most of the restart code is common with 64s, so with more correctness
>> and performance testing this could be re-enabled again by adding the
>> extra soft-mask checks to interrupt handlers and flipping
>> exit_must_hard_disable().
>>
>> Fixes: 9d1988ca87dd ("powerpc/64: treat low kernel text as irqs soft-masked")
>> Signed-off-by: Nicholas Piggin <npig...@gmail.com>
>> ---
>> arch/powerpc/include/asm/interrupt.h | 33 ++++++++++++++++++++--------
>> arch/powerpc/kernel/exceptions-64e.S | 12 +---------
>> arch/powerpc/kernel/interrupt.c | 2 +-
>> arch/powerpc/kernel/interrupt_64.S | 16 ++++++++++++--
>> 4 files changed, 40 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/interrupt.h
>> b/arch/powerpc/include/asm/interrupt.h
>> index 8b4b1e84e110..f13c93b033c7 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -73,20 +73,34 @@
>> #include <asm/kprobes.h>
>> #include <asm/runlatch.h>
>>
>> -#ifdef CONFIG_PPC64
>> +#ifdef CONFIG_PPC_BOOK3S_64
>
> Can we avoid that ifdef and use IS_ENABLED(CONFIG_PPC_BOOK3S_64) below ?
Hey Christophe,
Thanks for the review, sorry it was a bit rushed to get these fixes in
before the pull. I agree with this and there's a few other cleanups we
might do as well. Something to look at next.
>
>> extern char __end_soft_masked[];
>> unsigned long search_kernel_restart_table(unsigned long addr);
>> -#endif
>>
>> -#ifdef CONFIG_PPC_BOOK3S_64
>> DECLARE_STATIC_KEY_FALSE(interrupt_exit_not_reentrant);
>>
>> +static inline bool is_implicit_soft_masked(struct pt_regs *regs)
>> +{
>> + if (regs->msr & MSR_PR)
>> + return false;
>> +
>> + if (regs->nip >= (unsigned long)__end_soft_masked)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> static inline void srr_regs_clobbered(void)
>> {
>> local_paca->srr_valid = 0;
>> local_paca->hsrr_valid = 0;
>> }
>> #else
>> +static inline bool is_implicit_soft_masked(struct pt_regs *regs)
>> +{
>> + return false;
>> +}
>> +
>> static inline void srr_regs_clobbered(void)
>> {
>> }
>> @@ -150,11 +164,13 @@ static inline void interrupt_enter_prepare(struct
>> pt_regs *regs, struct interrup
>> */
>> if (TRAP(regs) != INTERRUPT_PROGRAM) {
>> CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>> - BUG_ON(regs->nip < (unsigned long)__end_soft_masked);
>> + BUG_ON(is_implicit_soft_masked(regs));
>> }
>> +#ifdef CONFIG_PPC_BOOK3S
>
> Allthough we are already in a PPC64 section, wouldn't it be better to use
> CONFIG_PPC_BOOK3S_64 ?
>
> Can we use IS_ENABLED(CONFIG_PPC_BOOK3S_64) instead ?
Good question, it's a matter of preference. I have used PPC_BOOK3S in
other places, but maybe in files shared by 32-bit it would be better to
have the _64?
On the other hand, in cases where you have an else or #else, then you
still need the PPC64 context to understand that.
I don't really have a preference, I would go with either. Making some
convention and using it everywhere is probably a good idea though.
Thanks,
Nick
