[powerpc/nmi: RFC 0/2] Support Soft NMI

[powerpc/nmi: RFC 0/2] Support Soft NMI

[Balbir Singh]

This patch is based on suggestions from paulus and benh.
The bugs are all mine. The idea was to implement soft
NMI(s) by keeping interrupts enabled in the soft-disabled
state, but to use the interrupt controller to gate posting
of new interrupts to the processor. This is still work in
progress and a preliminary RFC that needs testing.

Nick posted a more comprehensive version for soft NMI at
https://patchwork.ozlabs.org/patch/704605/, but it does
not work when interrupts are disabled

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>

Balbir Singh (2):
  Merge IPI and DEFAULT priorities
  Keep interrupts enabled even on soft disable

 arch/powerpc/include/asm/paca.h      |  1 +
 arch/powerpc/include/asm/xics.h      |  8 ++------
 arch/powerpc/kernel/exceptions-64s.S | 17 ++++++++++-------
 arch/powerpc/kernel/irq.c            | 21 ++++++++++++++++++++-
 arch/powerpc/kernel/time.c           | 27 ++++++++++++++++++++++++++-
 5 files changed, 59 insertions(+), 15 deletions(-)

-- 
2.9.3

[powerpc/nmi: RFC 1/2] Merge IPI and DEFAULT priorities

[Balbir Singh]

We merge IPI and DEFAULT priorities to the same
value. The idea is to keep interrupts enabled
even in lazy soft-disabled mode. Instead of storing
the IPI and irq separately, we keep the levels
same so that we deal with only one of them

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/xics.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/xics.h b/arch/powerpc/include/asm/xics.h
index f0b2385..d6cb9a0 100644
--- a/arch/powerpc/include/asm/xics.h
+++ b/arch/powerpc/include/asm/xics.h
@@ -14,17 +14,13 @@
 /* Want a priority other than 0.  Various HW issues require this. */
 #define	DEFAULT_PRIORITY	5
 
-/*
- * Mark IPIs as higher priority so we can take them inside interrupts
- * FIXME: still true now?
- */
-#define IPI_PRIORITY		4
+#define IPI_PRIORITY		5
 
 /* The least favored priority */
 #define LOWEST_PRIORITY		0xFF
 
 /* The number of priorities defined above */
-#define MAX_NUM_PRIORITIES	3
+#define MAX_NUM_PRIORITIES	2
 
 /* Native ICP */
 #ifdef CONFIG_PPC_ICP_NATIVE
-- 
2.9.3

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

[Balbir Singh]

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

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 */
 #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)
+
 #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
 TRAMP_REAL_BEGIN(kvmppc_skip_interrupt)
 	/*
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 3c05c31..c5b42f7 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -498,6 +498,21 @@ static inline void check_stack_overflow(void)
 #endif
 }
 
+void handle_elevated_irq(struct pt_regs *regs)
+{
+	unsigned int irq;
+	/*
+	 * NOTE, we don't use irq_enter/exit, otherwise
+	 * our accounting and tracing might be incorrect.
+	 */
+	irq = ppc_md.get_irq();
+
+	/*
+	 * Store away irq in PACA for replay later
+	 */
+	local_paca->irq = irq;
+}
+
 void __do_irq(struct pt_regs *regs)
 {
 	unsigned int irq;
@@ -513,7 +528,11 @@ void __do_irq(struct pt_regs *regs)
 	 *
 	 * This will typically lower the interrupt line to the CPU
 	 */
-	irq = ppc_md.get_irq();
+	if (local_paca->irq) {
+		irq = local_paca->irq;
+		local_paca->irq = 0;
+	} else
+		irq = ppc_md.get_irq();
 
 	/* We can hard enable interrupts now to allow perf interrupts */
 	may_hard_irq_enable();
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index be9751f..1f3b3cd 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -117,6 +117,7 @@ struct clock_event_device decrementer_clockevent = {
 };
 EXPORT_SYMBOL(decrementer_clockevent);
 
+DEFINE_PER_CPU(unsigned long long, nmi_started);
 DEFINE_PER_CPU(u64, decrementers_next_tb);
 static DEFINE_PER_CPU(struct clock_event_device, decrementers);
 
@@ -520,6 +521,7 @@ static void __timer_interrupt(void)
 	u64 now;
 
 	trace_timer_interrupt_entry(regs);
+	__this_cpu_write(nmi_started, 0);
 
 	if (test_irq_work_pending()) {
 		clear_irq_work_pending();
@@ -549,7 +551,6 @@ static void __timer_interrupt(void)
 		cu->current_tb = mfspr(SPRN_PURR);
 	}
 #endif
-
 	trace_timer_interrupt_exit(regs);
 }
 
@@ -566,6 +567,7 @@ void timer_interrupt(struct pt_regs * regs)
 	 * some CPUs will continue to take decrementer exceptions.
 	 */
 	set_dec(decrementer_max);
+	__this_cpu_write(nmi_started, 0);
 
 	/* Some implementations of hotplug will get timer interrupts while
 	 * offline, just ignore these and we also need to set
@@ -598,6 +600,29 @@ void timer_interrupt(struct pt_regs * regs)
 }
 EXPORT_SYMBOL(timer_interrupt);
 
+
+/*
+ * If we have watchdog enabled, we do expect to hit this
+ * at-least once per sample_frequence (20 seconds).
+ * We set the decrement to 20 seconds and if we hit it
+ * again.. it's an NMI panic
+ */
+void handle_soft_nmi(struct pt_regs *regs)
+{
+	unsigned long long tb = mftb();
+	unsigned long long nmi_started_tb = __this_cpu_read(nmi_started);
+
+	if (!nmi_started_tb) {
+		set_dec(ppc_tb_freq);
+		__this_cpu_write(nmi_started, tb);
+	} else {
+		if ((tb - nmi_started_tb) >= (30 * ppc_tb_freq)) {
+			BUG_ON(1);
+		} else
+			set_dec(ppc_tb_freq);
+	}
+}
+
 /*
  * Hypervisor decrementer interrupts shouldn't occur but are sometimes
  * left pending on exit from a KVM guest.  We don't need to do anything
-- 
2.9.3

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

[Nicholas Piggin]

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

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

[Benjamin Herrenschmidt]

On Mon, 2016-12-12 at 23:31 +1000, Nicholas Piggin wrote:
> 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.

We might want to be a bit careful. It will work with XICS fine, but it
might be trickier with a controller that needs explicit masking
of the just received interrupts like some of the old mac ones. Or MPIC
that you haven't modified to flatten the priorities etc....

Also lazy masking is ppc64 only but irc.c and time.c are shared.

I think we need to make this an "opt-in" based on some bit set by the
platform or the PIC, possibly in ppc_md.

Also note that there's already a PACA field to "recover" an interrupt
snatched by KVM, though it's XICS specific, while your approach is more
generic, you may want to merge the two. Talk to Paulus.

Cheers,
Ben.

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

[Balbir Singh]

On Mon, 2016-12-12 at 09:24 -0600, Benjamin Herrenschmidt wrote:
> On Mon, 2016-12-12 at 23:31 +1000, Nicholas Piggin wrote:
> > 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.
> 
> We might want to be a bit careful. It will work with XICS fine, but
> it
> might be trickier with a controller that needs explicit masking
> of the just received interrupts like some of the old mac ones. Or
> MPIC
> that you haven't modified to flatten the priorities etc....
> 
> Also lazy masking is ppc64 only but irc.c and time.c are shared.
> 
> I think we need to make this an "opt-in" based on some bit set by the
> platform or the PIC, possibly in ppc_md.
> 

Even though I'm using ppc_md.get_irq() the routine is called
only from exception64s.S, I can make the code conditional on
PPC_XICS. When we exploit the xive bits, we can add support for
that as well.

> Also note that there's already a PACA field to "recover" an interrupt
> snatched by KVM, though it's XICS specific, while your approach is
> more
> generic, you may want to merge the two. Talk to Paulus.
>

That is specific to KVM for kvm_interrupt_hv and kvm has a referecne to
the xics as well and within it, saved_xirr is tracked

Thanks for the reviews,
Balbir 

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

[Balbir Singh]

On Mon, 2016-12-12 at 23:31 +1000, Nicholas Piggin wrote:
> 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.

Thats good to know, I believe so as well.

<snip>

> > 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?
>

Will do
 
> > +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.
>

Sure, will rename them
 
> 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.
> 

Yep, although the code works for PPC_XICS only which is good for now.
When we do XIVE, we can add more bits

Balbir

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

[Nicholas Piggin]

On Tue, 13 Dec 2016 16:36:11 +1100
Balbir Singh <bsingharora@gmail.com> wrote:

> On Mon, 2016-12-12 at 23:31 +1000, Nicholas Piggin wrote:
> > 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.  
> 
> Thats good to know, I believe so as well.
> 
> <snip>
> 
> > > 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?
> >  
> 
> Will do
>  
> > > +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.
> >  
> 
> Sure, will rename them
>  
> > 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.
> >   
> 
> Yep, although the code works for PPC_XICS only which is good for now.
> When we do XIVE, we can add more bits

Other thing is, I would do the masked external interrupt as its own
patch. NMI is basically independent as far as I can see.

Thanks,
Nick

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

[Benjamin Herrenschmidt]

On Tue, 2016-12-13 at 14:28 +1100, Balbir Singh wrote:
> > Also note that there's already a PACA field to "recover" an
> > interrupt
> > snatched by KVM, though it's XICS specific, while your approach is
> > more
> > generic, you may want to merge the two. Talk to Paulus.
> > 
> 
> That is specific to KVM for kvm_interrupt_hv and kvm has a referecne
> to the xics as well and within it, saved_xirr is tracked

yes but it does exactly the same thing as what you are adding some one
of them should probably go.

Cheers,
Ben.

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

[Benjamin Herrenschmidt]

On Tue, 2016-12-13 at 16:36 +1100, Balbir Singh wrote:
> Yep, although the code works for PPC_XICS only which is good for now.
> When we do XIVE, we can add more bits

We may want to do XIVE differently, dunno. On XIVE we can just poke the
processor priority with a single MMIO store, so we don't actually need
to "fetch" the interrupt and we can continue doing separate priorities.

Note that raising the priority would work on XICS in *theory* as well
but HW bugs get in the way if we do that.

We also need to make sure you either adjust MPIC and all other PICs
potentially used on ppc64 to do this "only one priority" thing or you
disable that new mechanism on all those PICs.

That's why I mentioned opt-in. Maybe make it conditional on a global
boolean that gets enabled by the PIC itself, or make it an enum

enum lazy_irq_masking_mode {
	lazy_irq_mask_ee,	/* Use CPU EE bit (default) */
	lazy_irq_mask_fetch,	/* Fetch the interrupt and stash it away */
	lazy_irq_mask_prio	/* Change processor priority */
};

For the latter we'd need a ppc_md. hook to do the priority change
which xive (and potentially others like MPIC) could use.

Cheers,
Ben.

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

[Balbir Singh]

On 14/12/16 02:27, Benjamin Herrenschmidt wrote:
> On Tue, 2016-12-13 at 16:36 +1100, Balbir Singh wrote:
>> Yep, although the code works for PPC_XICS only which is good for now.
>> When we do XIVE, we can add more bits
> 
> We may want to do XIVE differently, dunno. On XIVE we can just poke the
> processor priority with a single MMIO store, so we don't actually need
> to "fetch" the interrupt and we can continue doing separate priorities.
> 

It would be good to have it be uniform, where the CPPR can be set to
the level of the current IRQ being processed (as seen from the controller)
but not yet finished via EOI. I've not looked at the CPPR/Context/Queue/Ring
details of the XIVE. But I'll let you provide the expertise on IRQ
handling

> Note that raising the priority would work on XICS in *theory* as well
> but HW bugs get in the way if we do that.
> 

Yep, I am not using the MFRR. Right now, I notice the CPPR is set to the
priority of the acked interrupt when we do a read of XIRR. Which works
well when we have just one priority at the moment.

> We also need to make sure you either adjust MPIC and all other PICs
> potentially used on ppc64 to do this "only one priority" thing or you
> disable that new mechanism on all those PICs.
> 

I was planning to skipping other IRQ chips for now and support just
XICS/XIVE with BOOK3S and PPC64. But we can discuss this.

> That's why I mentioned opt-in. Maybe make it conditional on a global
> boolean that gets enabled by the PIC itself, or make it an enum
> 
> enum lazy_irq_masking_mode {
> 	lazy_irq_mask_ee,	/* Use CPU EE bit (default) */
> 	lazy_irq_mask_fetch,	/* Fetch the interrupt and stash it away */
> 	lazy_irq_mask_prio	/* Change processor priority */
> };
> 
> For the latter we'd need a ppc_md. hook to do the priority change
> which xive (and potentially others like MPIC) could use.

We have set_cpu_priority for XICS, which sets the base_priority
only for the CPPR at the moment. It can be extended

Thanks for the comments,
Balbir

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

[Benjamin Herrenschmidt]

On Wed, 2016-12-14 at 11:41 +1100, Balbir Singh wrote:
> I was planning to skipping other IRQ chips for now and support just
> XICS/XIVE with BOOK3S and PPC64. But we can discuss this.

Well you still need to make sure you don't do your lazy stuff on
them and actually mask EE.

> > That's why I mentioned opt-in. Maybe make it conditional on a
> > global
> > boolean that gets enabled by the PIC itself, or make it an enum
> > 
> > enum lazy_irq_masking_mode {
> >        lazy_irq_mask_ee,       /* Use CPU EE bit (default) */
> >        lazy_irq_mask_fetch,    /* Fetch the interrupt and stash it
> > away */
> >        lazy_irq_mask_prio      /* Change processor priority */
> > };
> > 
> > For the latter we'd need a ppc_md. hook to do the priority change
> > which xive (and potentially others like MPIC) could use.
> 
> We have set_cpu_priority for XICS, which sets the base_priority
> only for the CPPR at the moment. It can be extended

Well, that's what I said earlier. XICS can do that in *theory* but it's
broken in HW. There's a race condition or two, if you whack the CPPR in
a way that causes a pending interrupt to be rejected, there's a timing
window where the ICP can wedge itself or the interrupt be lost, I don't
remember.

The only safe way on XICS is to fetch the interrupt (which implicitly
raises the CPPR) and lower it using EOI.

Cheers,
Ben.