Re: [powerpc/nmi: RFC 2/2] Keep interrupts enabled even on soft disable

[Nicholas Piggin <npiggin@gmail.com>]

On Mon, 12 Dec 2016 20:50:03 +1100
Balbir Singh <bsingharora@gmail.com> wrote:

> This patch removes the disabling of interrupts
> in soft-disable mode, when interrupts are received
> (in lazy mode). The new scheme keeps the interrupts
> enabled when we receive an interrupt and does the
> following
> 
> a. On decrementer interrupt, instead of setting
> dec to maximum and returning, we do the following
>   i. Call a function handle_nmi_dec, which in
>      turn calls handle_soft_nmi
>   ii. handle_soft_nmi sets the decrementer value
>       to 1 second and checks if more than 30
>       seconds have passed since starting it. If
>       so it calls BUG_ON(1), we can do an NMI
>       panic as well.
> b. When an external interrupt is received, we
>    store the interrupt in local_paca via
>    ppc_md.get_irq(). Later when interrupts are
>    enabled and replayed, we reuse the stored
>    interrupt and process it via generic_handle_irq

This seems pretty good. My NMI handler should plug in just
the same to the masked decrementer, so that wouldn't be a
problem.

> 
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> 
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/include/asm/paca.h      |  1 +
>  arch/powerpc/kernel/exceptions-64s.S | 17 ++++++++++-------
>  arch/powerpc/kernel/irq.c            | 21 ++++++++++++++++++++-
>  arch/powerpc/kernel/time.c           | 27 ++++++++++++++++++++++++++-
>  4 files changed, 57 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index 6a6792b..091af5c 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -158,6 +158,7 @@ struct paca_struct {
>  	u8 irq_happened;		/* irq happened while soft-disabled */
>  	u8 io_sync;			/* writel() needs spin_unlock sync */
>  	u8 irq_work_pending;		/* IRQ_WORK interrupt while soft-disable */
> +	u32 irq;			/* IRQ pending */
>  	u8 nap_state_lost;		/* NV GPR values lost in power7_idle */
>  	u64 sprg_vdso;			/* Saved user-visible sprg */

Can you avoid some padding if you move it to below irq_happened?

>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index d39d611..2620a90 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1287,23 +1287,23 @@ EXC_VIRT_NONE(0x5800, 0x5900)
>  #define MASKED_INTERRUPT(_H)				\
>  masked_##_H##interrupt:					\
>  	std	r11,PACA_EXGEN+EX_R11(r13);		\
> +	std	r12,PACA_EXGEN+EX_R12(r13);		\
>  	lbz	r11,PACAIRQHAPPENED(r13);		\
>  	or	r11,r11,r10;				\
>  	stb	r11,PACAIRQHAPPENED(r13);		\
>  	cmpwi	r10,PACA_IRQ_DEC;			\
>  	bne	1f;					\
> -	lis	r10,0x7fff;				\
> -	ori	r10,r10,0xffff;				\
> -	mtspr	SPRN_DEC,r10;				\
> +	GET_SCRATCH0(r10);				\
> +	std	r13,PACA_EXGEN+EX_R13(r13);		\
> +	EXCEPTION_PROLOG_PSERIES_1(handle_nmi_dec, _H);	\
>  	b	2f;					\
>  1:	cmpwi	r10,PACA_IRQ_DBELL;			\
>  	beq	2f;					\
>  	cmpwi	r10,PACA_IRQ_HMI;			\
>  	beq	2f;					\
> -	mfspr	r10,SPRN_##_H##SRR1;			\
> -	rldicl	r10,r10,48,1; /* clear MSR_EE */	\
> -	rotldi	r10,r10,16;				\
> -	mtspr	SPRN_##_H##SRR1,r10;			\
> +	GET_SCRATCH0(r10);				\
> +	std	r13,PACA_EXGEN+EX_R13(r13);		\
> +	EXCEPTION_PROLOG_PSERIES_1(elevate_save_irq, _H);\
>  2:	mtcrf	0x80,r9;				\
>  	ld	r9,PACA_EXGEN+EX_R9(r13);		\
>  	ld	r10,PACA_EXGEN+EX_R10(r13);		\
> @@ -1321,6 +1321,9 @@ USE_FIXED_SECTION(virt_trampolines)
>  	MASKED_INTERRUPT()
>  	MASKED_INTERRUPT(H)
>  
> +EXC_COMMON(handle_nmi_dec, 0x900, handle_soft_nmi)
> +EXC_COMMON(elevate_save_irq, 0x500, handle_elevated_irq)

I wonder if the name should match the type of interrupt rather than
implementation detail (elevated?), and match the existing handlers
e.g, hardware_interrupt_masked common handler could call do_IRQ_masked.

As for the NMI, I would prefer just to keep it out of the timer path
completely and schedule a Linux timer for it as I had.

Otherwise, this looks nice if it does the right thing with the interrupt
controller. It hasn't taken a lot of lines to implement which is very
cool.

Thanks,
Nick