Re: [PATCH v5 09/22] powerpc/syscall: Make interrupt.c buildable on PPC32

From: Nicholas Piggin <npiggin@gmail.com>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Christophe Leroy <christophe.leroy@csgroup.eu>,
	Michael Ellerman <mpe@ellerman.id.au>,
	msuchanek@suse.de, Paul Mackerras <paulus@samba.org>
Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v5 09/22] powerpc/syscall: Make interrupt.c buildable on PPC32
Date: Tue, 09 Feb 2021 11:27:13 +1000	[thread overview]
Message-ID: <1612833796.dl9doe6njg.astroid@bobo.none> (raw)
In-Reply-To: <ba073ad67bd971a88ce331b65d6655523b54c794.1612796617.git.christophe.leroy@csgroup.eu>

Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am:
> To allow building interrupt.c on PPC32, ifdef out specific PPC64
> code or use helpers which are available on both PP32 and PPC64
> 
> Modify Makefile to always build interrupt.o
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v5:
> - Also for interrupt exit preparation
> - Opted out kuap related code, ppc32 keeps it in ASM for the time being
> ---
>  arch/powerpc/kernel/Makefile    |  4 ++--
>  arch/powerpc/kernel/interrupt.c | 31 ++++++++++++++++++++++++-------
>  2 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 26ff8c6e06b7..163755b1cef4 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -57,10 +57,10 @@ obj-y				:= cputable.o syscalls.o \
>  				   prom.o traps.o setup-common.o \
>  				   udbg.o misc.o io.o misc_$(BITS).o \
>  				   of_platform.o prom_parse.o firmware.o \
> -				   hw_breakpoint_constraints.o
> +				   hw_breakpoint_constraints.o interrupt.o
>  obj-y				+= ptrace/
>  obj-$(CONFIG_PPC64)		+= setup_64.o \
> -				   paca.o nvram_64.o note.o interrupt.o
> +				   paca.o nvram_64.o note.o
>  obj-$(CONFIG_COMPAT)		+= sys_ppc32.o signal_32.o
>  obj-$(CONFIG_VDSO32)		+= vdso32_wrapper.o
>  obj-$(CONFIG_PPC_WATCHDOG)	+= watchdog.o
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index d6be4f9a67e5..2dac4d2bb1cf 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -39,7 +39,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
>  		BUG_ON(!(regs->msr & MSR_RI));
>  	BUG_ON(!(regs->msr & MSR_PR));
>  	BUG_ON(!FULL_REGS(regs));
> -	BUG_ON(regs->softe != IRQS_ENABLED);
> +	BUG_ON(arch_irq_disabled_regs(regs));
>  
>  #ifdef CONFIG_PPC_PKEY
>  	if (mmu_has_feature(MMU_FTR_PKEY)) {
> @@ -65,7 +65,9 @@ notrace long system_call_exception(long r3, long r4, long r5,
>  			isync();
>  	} else
>  #endif
> +#ifdef CONFIG_PPC64
>  		kuap_check_amr();
> +#endif

Wouldn't mind trying to get rid of these ifdefs at some point, but 
there's some kuap / keys changes going on recently so I'm happy enough 
to let this settle then look at whether we can refactor.

>  
>  	account_cpu_user_entry();
>  
> @@ -77,7 +79,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
>  	 * frame, or if the unwinder was taught the first stack frame always
>  	 * returns to user with IRQS_ENABLED, this store could be avoided!
>  	 */
> -	regs->softe = IRQS_ENABLED;
> +	irq_soft_mask_regs_set_state(regs, IRQS_ENABLED);
>  
>  	local_irq_enable();
>  
> @@ -151,6 +153,7 @@ static notrace inline bool __prep_irq_for_enabled_exit(bool clear_ri)
>  		__hard_EE_RI_disable();
>  	else
>  		__hard_irq_disable();
> +#ifdef CONFIG_PPC64
>  	if (unlikely(lazy_irq_pending_nocheck())) {
>  		/* Took an interrupt, may have more exit work to do. */
>  		if (clear_ri)
> @@ -162,7 +165,7 @@ static notrace inline bool __prep_irq_for_enabled_exit(bool clear_ri)
>  	}
>  	local_paca->irq_happened = 0;
>  	irq_soft_mask_set(IRQS_ENABLED);
> -
> +#endif
>  	return true;
>  }
>  

Do we prefer space before return except in trivial cases?

> @@ -216,7 +219,9 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>  
>  	CT_WARN_ON(ct_state() == CONTEXT_USER);
>  
> +#ifdef CONFIG_PPC64
>  	kuap_check_amr();
> +#endif
>  
>  	regs->result = r3;
>  
> @@ -309,7 +314,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>  
>  	account_cpu_user_exit();
>  
> -#ifdef CONFIG_PPC_BOOK3S /* BOOK3E not yet using this */
> +#ifdef CONFIG_PPC_BOOK3S_64 /* BOOK3E and ppc32 not using this */
>  	/*
>  	 * We do this at the end so that we do context switch with KERNEL AMR
>  	 */
> @@ -318,7 +323,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>  	return ret;
>  }
>  
> -#ifdef CONFIG_PPC_BOOK3S /* BOOK3E not yet using this */
> +#ifndef CONFIG_PPC_BOOK3E_64 /* BOOK3E not yet using this */
>  notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>  {
>  #ifdef CONFIG_PPC_BOOK3E

Why are you building this for 32? I don't mind if it's just to keep 
things similar and make it build for now, but you're not using it yet,
right?

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> @@ -333,14 +338,16 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>  		BUG_ON(!(regs->msr & MSR_RI));
>  	BUG_ON(!(regs->msr & MSR_PR));
>  	BUG_ON(!FULL_REGS(regs));
> -	BUG_ON(regs->softe != IRQS_ENABLED);
> +	BUG_ON(arch_irq_disabled_regs(regs));
>  	CT_WARN_ON(ct_state() == CONTEXT_USER);
>  
>  	/*
>  	 * We don't need to restore AMR on the way back to userspace for KUAP.
>  	 * AMR can only have been unlocked if we interrupted the kernel.
>  	 */
> +#ifdef CONFIG_PPC64
>  	kuap_check_amr();
> +#endif
>  
>  	local_irq_save(flags);
>  
> @@ -407,7 +414,9 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>  	/*
>  	 * We do this at the end so that we do context switch with KERNEL AMR
>  	 */
> +#ifdef CONFIG_PPC64
>  	kuap_user_restore(regs);
> +#endif
>  	return ret;
>  }
>  
> @@ -419,7 +428,9 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
>  	unsigned long *ti_flagsp = &current_thread_info()->flags;
>  	unsigned long flags;
>  	unsigned long ret = 0;
> +#ifdef CONFIG_PPC64
>  	unsigned long amr;
> +#endif
>  
>  	if (IS_ENABLED(CONFIG_PPC_BOOK3S) && unlikely(!(regs->msr & MSR_RI)))
>  		unrecoverable_exception(regs);
> @@ -432,7 +443,9 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
>  	if (TRAP(regs) != 0x700)
>  		CT_WARN_ON(ct_state() == CONTEXT_USER);
>  
> +#ifdef CONFIG_PPC64
>  	amr = kuap_get_and_check_amr();
> +#endif
>  
>  	if (unlikely(*ti_flagsp & _TIF_EMULATE_STACK_STORE)) {
>  		clear_bits(_TIF_EMULATE_STACK_STORE, ti_flagsp);
> @@ -441,7 +454,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
>  
>  	local_irq_save(flags);
>  
> -	if (regs->softe == IRQS_ENABLED) {
> +	if (!arch_irq_disabled_regs(regs)) {
>  		/* Returning to a kernel context with local irqs enabled. */
>  		WARN_ON_ONCE(!(regs->msr & MSR_EE));
>  again:
> @@ -458,8 +471,10 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
>  	} else {
>  		/* Returning to a kernel context with local irqs disabled. */
>  		__hard_EE_RI_disable();
> +#ifdef CONFIG_PPC64
>  		if (regs->msr & MSR_EE)
>  			local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
> +#endif
>  	}
>  
>  
> @@ -472,7 +487,9 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
>  	 * which would cause Read-After-Write stalls. Hence, we take the AMR
>  	 * value from the check above.
>  	 */
> +#ifdef CONFIG_PPC64
>  	kuap_kernel_restore(regs, amr);
> +#endif
>  
>  	return ret;
>  }
> -- 
> 2.25.0
> 
> 