Message ID | 20170227003655.18459-1-bsingharora@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Balbir Singh <bsingharora@gmail.com> writes: > With XICS emulation, setting the CPPR to DEFAULT_PRIORITY ^ (Current Processor Priority Register) > masks all interrupts including IPI's which map to a single > underlying priority. It took me a while to parse that. So because of the way the OPAL XICS emulation is implemented, setting the CPPR to DEFAULT_PRIORITY has the effect of masking all interrupts. That is because the OPAL code internally maps all priorities that are > 0 and < 0xff to a single priority. It happens to use 7, but I don't think that matters does it? It's just that there's no differentiation between DEFAULT and IPI. I realise we need to work around it anyway, but are we calling this a bug in the XICS emulation? Or just an alternate feature? :) Have we thought about doing the fix in icp_opal_set_cpu_priority() instead? > The fix does two things > > 1. It moves the setting of CPPR to after all IRQ migration > is complete > 2. It sets the CPPR to LOWEST_PRIORITY, so that interrupts > and IPI's are effectively enabled. The expectation is that > we'll see just IPI's after migration > > The fix is quite generic in that it will work when we move > DEFAULT and IPI priorities to the same value in the kernel > later. I didn't know we were doing that :) What I'm most interested in is some soothing words to tell me that this definitely won't break on existing machines, using either a real native XICS or the HV backend. > Cc: mikey@neuling.org > Cc: benh@kernel.crashing.org I really dislike those Cc lines in the change log, they record nothing, other than the fact that Ben & Mikey get lots of email that they don't have time to read. In fact I'm not sure you even Cc'ed them? > Fixes: d74361881f0d ("powerpc/xics: Add ICP OPAL backend") > Cc: stable@vger.kernel.org # v4.8+ > > Reported-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> > Tested-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> > Signed-off-by: Balbir Singh <bsingharora@gmail.com> > --- > arch/powerpc/sysdev/xics/xics-common.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c > index c674a9d..31774e0 100644 > --- a/arch/powerpc/sysdev/xics/xics-common.c > +++ b/arch/powerpc/sysdev/xics/xics-common.c > @@ -20,6 +20,7 @@ > #include <linux/of.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > +#include <linux/delay.h> > > #include <asm/prom.h> > #include <asm/io.h> > @@ -198,9 +199,6 @@ void xics_migrate_irqs_away(void) > /* Remove ourselves from the global interrupt queue */ > xics_set_cpu_giq(xics_default_distrib_server, 0); > > - /* Allow IPIs again... */ > - icp_ops->set_priority(LOWEST_PRIORITY); > - That's DEFAULT_PRIORITY in my tree? > @@ -255,6 +253,19 @@ void xics_migrate_irqs_away(void) > unlock: > raw_spin_unlock_irqrestore(&desc->lock, flags); > } > + > + /* > + * Allow sufficient time to drop any inflight IRQ's > + */ > + mdelay(1); That looks suspiciously like the code in migrate_irqs(), except it lacks the irq enable/disable. So the comment should make it clear that we're waiting for the hardware to drop any inflight IRQs, not that we're waiting for them to be handled by Linux. And the dropping is caused by the set_priority(0) we did previously (not visible in the diff). Do we have *any* basis for 1ms, other than it's "a while"? > + > + /* > + * Allow IPIs again. This is done at the very end, after ^ and all other interrupts > + * migrating all interrupts, the expectation is that we'll > + * only get woken up by an IPI interrupt beyond this point **because we have migrated all other irqs away** ? > + */ > + icp_ops->set_priority(LOWEST_PRIORITY); > + > } > #endif /* CONFIG_HOTPLUG_CPU */ cheers
On Mon, 2017-02-27 at 23:03 +1100, Michael Ellerman wrote: > It took me a while to parse that. > > So because of the way the OPAL XICS emulation is implemented, setting > the CPPR to DEFAULT_PRIORITY has the effect of masking all > interrupts. > > That is because the OPAL code internally maps all priorities that are > > > 0 and < 0xff to a single priority. It happens to use 7, but I don't > think that matters does it? It's just that there's no differentiation > between DEFAULT and IPI. > > I realise we need to work around it anyway, but are we calling this a > bug in the XICS emulation? Or just an alternate feature? :) > > Have we thought about doing the fix in icp_opal_set_cpu_priority() > instead? The fix in OPAL is doable but requires churn. I would have to do IPIs differently (the way I do them in KVM XICS emulation actually which is noticeably better). I will eventually do it I think but for now, I favor this patch instead because the existing code is also broken for Balbir WIP patch that removes the separate priority for IPIs to do lazy masking in the XICS. Cheers, Ben.
diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c index c674a9d..31774e0 100644 --- a/arch/powerpc/sysdev/xics/xics-common.c +++ b/arch/powerpc/sysdev/xics/xics-common.c @@ -20,6 +20,7 @@ #include <linux/of.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/delay.h> #include <asm/prom.h> #include <asm/io.h> @@ -198,9 +199,6 @@ void xics_migrate_irqs_away(void) /* Remove ourselves from the global interrupt queue */ xics_set_cpu_giq(xics_default_distrib_server, 0); - /* Allow IPIs again... */ - icp_ops->set_priority(LOWEST_PRIORITY); - for_each_irq_desc(virq, desc) { struct irq_chip *chip; long server; @@ -255,6 +253,19 @@ void xics_migrate_irqs_away(void) unlock: raw_spin_unlock_irqrestore(&desc->lock, flags); } + + /* + * Allow sufficient time to drop any inflight IRQ's + */ + mdelay(1); + + /* + * Allow IPIs again. This is done at the very end, after + * migrating all interrupts, the expectation is that we'll + * only get woken up by an IPI interrupt beyond this point + */ + icp_ops->set_priority(LOWEST_PRIORITY); + } #endif /* CONFIG_HOTPLUG_CPU */