Message-ID: <86lgfoc9lx.wl-marc.zyngier@arm.com>
Date:   Tue, 20 Feb 2018 09:07:38 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     Jayachandran C <jnair@...iumnetworks.com>
Cc:     Ganapatrao Kulkarni <ganapatrao.kulkarni@...ium.com>,
        <linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <tglx@...utronix.de>,
        <jason@...edaemon.net>, <catalin.marinas@....com>,
        <will.deacon@....com>, <corbet@....net>,
        <Robert.Richter@...ium.com>, <Jan.Glauber@...ium.com>,
        <Vadim.Lomovtsev@...ium.com>, <gklkml16@...il.com>
Subject: Re: [PATCH v2] irqchip/gic-v3-its: Add workaround for ThunderX2 erratum #174

On Mon, 19 Feb 2018 21:12:10 +0000,
Jayachandran C wrote:
> 
> On Sun, Jan 21, 2018 at 11:35:34AM +0000, Marc Zyngier wrote:
> > On Sun, 21 Jan 2018 07:00:48 +0000,
> > Jayachandran C wrote:
> > > 
> > > On Thu, Jan 18, 2018 at 10:58:20AM +0530, Ganapatrao Kulkarni wrote:
> > > > This erratum is observed on the ThunderX2 GICv3 ITS. When a
> > > > MOVI command is used to change affinity of a LPI to a collection/cpu
> > > > on another node, the LPI is not delivered to the cpu.
> > > > An additional INV command is required after the MOVI to deliver
> > > > the LPI to the new destination.
> > > > 
> > > > If we add INV after MOVI, there is a chance that we lose LPIs which
> > > > are raised when the affinity is changed. So for now, adding workaround fix
> > > > to disable inter node affinity change.
> > > > 
> > > > Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@...ium.com>
> > > > ---
> > > > 
> > > > v2: Added workaround to avoid inter node affinity change.
> > > > 
> > > > v1: Initial patch
> > > > 
> > > >  Documentation/arm64/silicon-errata.txt |  1 +
> > > >  arch/arm64/Kconfig                     | 10 ++++++++++
> > > >  drivers/irqchip/irq-gic-v3-its.c       | 21 ++++++++++++++++++++-
> > > >  3 files changed, 31 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> > > > index fc1c884..fb27cb5 100644
> > > > --- a/Documentation/arm64/silicon-errata.txt
> > > > +++ b/Documentation/arm64/silicon-errata.txt
> > > > @@ -63,6 +63,7 @@ stable kernels.
> > > >  | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456        |
> > > >  | Cavium         | ThunderX Core   | #30115          | CAVIUM_ERRATUM_30115        |
> > > >  | Cavium         | ThunderX SMMUv2 | #27704          | N/A                         |
> > > > +| Cavium         | ThunderX2 ITS   | #174            | CAVIUM_ERRATUM_174          |
> > > >  | Cavium         | ThunderX2 SMMUv3| #74             | N/A                         |
> > > >  | Cavium         | ThunderX2 SMMUv3| #126            | N/A                         |
> > > >  |                |                 |                 |                             |
> > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > > index c9a7e9e..0dbf3bd 100644
> > > > --- a/arch/arm64/Kconfig
> > > > +++ b/arch/arm64/Kconfig
> > > > @@ -461,6 +461,16 @@ config ARM64_ERRATUM_843419
> > > >  
> > > >  	  If unsure, say Y.
> > > >  
> > > > +config CAVIUM_ERRATUM_174
> > > > +	bool "Cavium ThunderX2 erratum 174"
> > > > +	default y
> > > > +	help
> > > > +	  Cavium ThunderX2 dual socket systems may loose interrupts
> > > > +	  on affinity change to a cpu on other node.
> > > > +	  This workaround fix avoids inter node affinity change.
> > > 
> > > This has to be fixed up to match the commit message (and for spelling).
> > > I have seen some questions offlist about how important this fix is,
> > > and how it can affect users - so that would be useful to have in the
> > > description as well.
> > > 
> > > To clarify, this errata comes into play only when the irq affinity is
> > > forced from the node given by the device (and ITS) affinity to another
> > > node.  This should not happen in normal, useful configurations.
> > 
> > Define normal. That's all under control of userspace, and the kernel
> > doesn't really have a say. irqbalance will happily move interrupts
> > around. Disable all CPUs from node at runtime (again, from userspace),
> > and you'll get the exact same thing. I can't see what's so "abnormal"
> > about any of that.
> > 
> > > Also, we will hold further posting of this errata until we do another
> > > round of investigation with the hardware team for a better solution.
> > > If we can handle the pending interrupts for the small window of MOVI/INV
> > > in first workaround, we will not need this restriction at all.
> > 
> > What do you mean by "If we can handle the pending interrupts for the
> > small window of MOVI/INV"? Taking the interrupt on the source CPU?
> > Sure, that would be fine. But that's assuming that the souce CPU is in
> > a position to actually handle this, and is not simply going down.
> > 
> > If there is only a slight possibility that you may loose an interrupt
> > in the MOVI/INV window (which is not that small, since that's a 4
> > command sequence), your only other solution is to inject a spurious
> > interrupt to replace the one you may have lost in that window.
> > 
> > In the meantime, and until I see a patch fixing this (or a decent
> > explanation of why this isn't a problem), I'll consider it broken.
> 
> After reviewing the issue with our hardware team, we decided to
> tweak the redistributor cache configuration from firmware rather
> than go with this errata workaournd in Linux (and other OSes).
> 
> So, with the new firmware MOVI will work across nodes as expected,
> and this patch is no longer neeeded.

That's good news! One question though: how do we detect the old (or
new) firmware? We'd still need mitigation for the "old firmware"
case, unless you have a way to guarantee that all existing boxes are
magically updated to a fixed version?

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Powered by blists - more mailing lists