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

[Robert Richter]

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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > ---
> >  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;
> >  }
> >  
> >