linux-kernel - Re: [PATCH] irqchip, gicv3-its, numa: Enable Cavium ThunderX #23144 workaround for ACPI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160511162715.GI24726@rric.localdomain>
Date:	Wed, 11 May 2016 18:27:15 +0200
From:	Robert Richter <robert.richter@...iumnetworks.com>
To:	Marc Zyngier <marc.zyngier@....com>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	Will Deacon <will.deacon@....com>,
	Tomasz Nowicki <tn@...ihalf.com>,
	"David Daney" <david.daney@...iumnetworks.com>,
	Ashok Kumar <ashoks@...adcom.com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] irqchip, gicv3-its, numa: Enable Cavium ThunderX #23144
 workaround for ACPI

Marc,

thanks for review and sorry for the delay of my answer.

On 03.05.16 08:40:47, Marc Zyngier wrote:
> On 02/05/16 17:38, Robert Richter wrote:
> > From: Robert Richter <rrichter@...ium.com>
> > 
> > In case of acpi the firmware does not provide node ids for cpus and
> > its devices. Determine it from system topology special to Cavium
> > ThunderX systems. This enables #23144 workaround for ACPI.
> > 
> > Signed-off-by: Robert Richter <rrichter@...ium.com>
> > ---
> >  drivers/irqchip/irq-gic-v3-its.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index 494395274cf7..6eac0f3c1e56 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -1107,11 +1107,13 @@ static void its_cpu_init_collection(void)
> >  
> >  		/* avoid cross node collections and its mapping */
> >  		if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
> > -			struct device_node *cpu_node;
> > -
> > -			cpu_node = of_get_cpu_node(cpu, NULL);
> > +			int nid = of_node_to_nid(of_get_cpu_node(cpu, NULL));
> > +			if (nid == NUMA_NO_NODE) {
> > +				pr_warn_once("ITS: Updating cpu numa node ids\n");
> 
> I don't really understand the meaning of that message. What are you
> updating here?

Maybe the message is misleading here. The intention is to get the node
id of the cpu which must map with that one of the its device. DT
provides that information, but for ACPI there is no way atm to
describe the topology. In this case, the nid is not set
(NUMA_NO_NODE). But for ThunderX we can determine the nid using mpidr.
This works since this (errata workaround) code is special to ThunderX
only and executed only there. Thus, even if acpi does not provide the
nid, we can determine the nid anyway. The message should just tell
that this mechanism was used.

> > +				nid = MPIDR_AFFINITY_LEVEL(read_cpuid_mpidr(), 2);
> > +			}
> >  			if (its->numa_node != NUMA_NO_NODE &&
> > -				its->numa_node != of_node_to_nid(cpu_node))
> 
> So you're going from something that was relatively generic
> (of_node_to_nid) to something that is now completely hardcoding the
> Cavium view of CPU topology. Doesn't ACPI have similar abstractions?

No, I would have used that then instead. So once a generic solution is
available we can update the code to use acpi and that code wont be
executed then anymore.

> 
> > +				its->numa_node != nid)
> >  				continue;
> >  		}
> >  
> > @@ -1443,6 +1445,15 @@ static void __maybe_unused its_enable_quirk_cavium_23144(void *data)
> >  {
> >  	struct its_node *its = data;
> >  
> > +	if (!IS_ENABLED(CONFIG_NUMA))
> > +		return;
> > +
> > +	if (its->numa_node == NUMA_NO_NODE) {
> > +		/* make ACPI work */
> > +		its->numa_node = (its->phys_base >> 44) & 0x3;
> 
> How is that ACPI specific?

Same here, the acpi specific is that the numa_node can not be detected
with acpi. Thus, we use a cpu specific approach to get the nid.

I also would prefer a generic way to read that from (acpi) firmware,
but right now this is the only way to get the workaround enabled in
acpi systems. Since this is isolated to ThunderX only, I think this is
ok.

-Robert

> > +		pr_warn_once("ITS: Updating ITS numa node ids\n");
> > +	}
> > +
> >  	its->flags |= ITS_FLAGS_WORKAROUND_CAVIUM_23144;
> >  }
> >  
> >