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 <redacted>
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
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 <redacted>
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(-)
@@ -14,17 +14,13 @@/* Want a priority other than 0. Various HW issues require this. */#define DEFAULT_PRIORITY 5-/*-*MarkIPIsashigherprioritysowecantaketheminsideinterrupts-*FIXME:stilltruenow?-*/-#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
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 <redacted>
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(-)
@@ -513,7 +528,11 @@ void __do_irq(struct pt_regs *regs)**ThiswilltypicallylowertheinterruptlinetotheCPU*/-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();
@@ -566,6 +567,7 @@ void timer_interrupt(struct pt_regs * regs)*someCPUswillcontinuetotakedecrementerexceptions.*/set_dec(decrementer_max);+__this_cpu_write(nmi_started,0);/* Some implementations of hotplug will get timer interrupts while*offline,justignoretheseandwealsoneedtoset
From: Nicholas Piggin <npiggin@gmail.com> Date: 2016-12-12 13:31:39
On Mon, 12 Dec 2016 20:50:03 +1100
Balbir Singh [off-list ref] 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.
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2016-12-12 15:24:33
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.
On Mon, 2016-12-12 at 09:24 -0600, Benjamin Herrenschmidt wrote:
On Mon, 2016-12-12 at 23:31 +1000, Nicholas Piggin wrote:
quoted
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
On Mon, 2016-12-12 at 23:31 +1000, Nicholas Piggin wrote:
On Mon, 12 Dec 2016 20:50:03 +1100
Balbir Singh [off-list ref] wrote:
quoted
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>
quoted
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?
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
From: Nicholas Piggin <npiggin@gmail.com> Date: 2016-12-13 06:06:55
On Tue, 13 Dec 2016 16:36:11 +1100
Balbir Singh [off-list ref] wrote:
On Mon, 2016-12-12 at 23:31 +1000, Nicholas Piggin wrote:
quoted
On Mon, 12 Dec 2016 20:50:03 +1100
Balbir Singh [off-list ref] wrote:
quoted
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>
quoted
quoted
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?
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
quoted
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2016-12-13 15:22:41
On Tue, 2016-12-13 at 14:28 +1100, Balbir Singh wrote:
quoted
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.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2016-12-13 15:28:24
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.
On Tue, 2016-12-13 at 16:36 +1100, Balbir Singh wrote:
quoted
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2016-12-15 15:15:38
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.
quoted
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.