Message-ID: <4773594.btHp9vXayV@vostro.rjw.lan>
Date:	Wed, 29 Oct 2014 22:33:06 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Hanjun Guo <hanjun.guo@...aro.org>
Cc:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Mark Rutland <Mark.Rutland@....com>,
	Olof Johansson <olof@...om.net>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	Will Deacon <Will.Deacon@....com>,
	"graeme.gregory@...aro.org" <graeme.gregory@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	Sudeep Holla <Sudeep.Holla@....com>,
	"jcm@...hat.com" <jcm@...hat.com>,
	Jason Cooper <jason@...edaemon.net>,
	Marc Zyngier <Marc.Zyngier@....com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>,
	Robert Richter <rric@...nel.org>,
	Lv Zheng <lv.zheng@...el.com>,
	Robert Moore <robert.moore@...el.com>,
	Liviu Dudau <Liviu.Dudau@....com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Charles Garcia-Tobin <Charles.Garcia-Tobin@....com>,
	"Kangkang.Shen@...wei.com" <Kangkang.Shen@...wei.com>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>
Subject: Re: [PATCH v5 12/18] ACPI / processor: Make it possible to get CPU hardware ID via GICC

On Monday, October 27, 2014 05:58:10 PM Hanjun Guo wrote:
> On 2014-10-25 1:39, Lorenzo Pieralisi wrote:
> > On Fri, Oct 17, 2014 at 02:37:08PM +0100, Hanjun Guo wrote:
> >> Introduce a new function map_gicc_mpidr() to allow MPIDRs to be obtained
> >> from the GICC Structure introduced by ACPI 5.1.
> >>
> >> MPIDR is the CPU hardware ID as local APIC ID on x86 platform, so we use
> >> MPIDR not the GIC CPU interface ID to identify CPUs.
> >>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
> >> ---
> >>  arch/arm64/include/asm/acpi.h |   29 +++++++++++++++++++++++++++++
> >>  arch/arm64/kernel/acpi.c      |    1 -
> >>  drivers/acpi/processor_core.c |   37 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 66 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >> index c82d4a1..483ff45 100644
> >> --- a/arch/arm64/include/asm/acpi.h
> >> +++ b/arch/arm64/include/asm/acpi.h
> >> @@ -12,6 +12,8 @@
> >>  #ifndef _ASM_ACPI_H
> >>  #define _ASM_ACPI_H
> >>  
> >> +#include <asm/smp_plat.h>
> >> +
> >>  /* Basic configuration for ACPI */
> >>  #ifdef	CONFIG_ACPI
> >>  #define acpi_strict 1	/* No out-of-spec workarounds on ARM64 */
> >> @@ -45,6 +47,33 @@ static inline void enable_acpi(void)
> >>  	acpi_noirq = 0;
> >>  }
> >>  
> >> +/* MPIDR value provided in GICC structure is 64 bits, but the
> >> + * existing apic_id (CPU hardware ID) using in acpi processor
> >> + * driver is 32-bit, to conform to the same datatype we need
> >> + * to repack the GICC structure MPIDR.
> >> + *
> >> + * Only 32 bits of MPIDR are used:
> >> + * 
> >> + * Bits [0:7] Aff0;
> >> + * Bits [8:15] Aff1;
> >> + * Bits [16:23] Aff2;
> >> + * Bits [32:39] Aff3;
> >> + */
> >> +static inline u32 pack_mpidr(u64 mpidr)
> >> +{
> >> +	return (u32) ((mpidr & 0xff00000000) >> 8) | mpidr;
> >> +}
> >> +
> >> +/*
> >> + * The ACPI processor driver for ACPI core code needs this macro
> >> + * to find out this cpu was already mapped (mapping from CPU hardware
> >> + * ID to CPU logical ID) or not.
> >> + *
> >> + * cpu_logical_map(cpu) is the mapping of MPIDR and the logical cpu,
> >> + * and MPIDR is the cpu hardware ID we needed to pack.
> >> + */
> >> +#define cpu_physical_id(cpu) pack_mpidr(cpu_logical_map(cpu))
> >> +
> >>  /*
> >>   * It's used from ACPI core in kdump to boot UP system with SMP kernel,
> >>   * with this check the ACPI core will not override the CPU index
> >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> >> index 34837e0..e91ec76 100644
> >> --- a/arch/arm64/kernel/acpi.c
> >> +++ b/arch/arm64/kernel/acpi.c
> >> @@ -24,7 +24,6 @@
> >>  #include <linux/bootmem.h>
> >>  #include <linux/smp.h>
> >>  
> >> -#include <asm/smp_plat.h>
> >>  #include <asm/cputype.h>
> >>  #include <asm/cpu_ops.h>
> >>  
> >> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> >> index e32321c..4007313 100644
> >> --- a/drivers/acpi/processor_core.c
> >> +++ b/drivers/acpi/processor_core.c
> >> @@ -64,6 +64,38 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
> >>  	return 0;
> >>  }
> >>  
> >> +/*
> >> + * On ARM platform, MPIDR value is the hardware ID as apic ID
> >> + * on Intel platforms
> >> + */
> >> +static int map_gicc_mpidr(struct acpi_subtable_header *entry,
> >> +		int device_declaration, u32 acpi_id, int *mpidr)
> >> +{
> >> +	struct acpi_madt_generic_interrupt *gicc =
> >> +	    container_of(entry, struct acpi_madt_generic_interrupt, header);
> >> +
> >> +	if (!(gicc->flags & ACPI_MADT_ENABLED))
> >> +		return -ENODEV;
> >> +
> >> +	/* In the GIC interrupt model, logical processors are
> >> +	 * required to have a Processor Device object in the DSDT,
> >> +	 * so we should check device_declaration here
> >> +	 */
> >> +	if (device_declaration && (gicc->uid == acpi_id)) {
> >> +		/*
> >> +		 * Only bits [0:7] Aff0, bits [8:15] Aff1, bits [16:23] Aff2
> >> +		 * and bits [32:39] Aff3 are meaningful, so pack the Affx
> >> +		 * fields into a single 32 bit identifier to accommodate the
> >> +		 * acpi processor drivers.
> >> +		 */
> >> +		*mpidr = ((gicc->arm_mpidr & 0xff00000000) >> 8)
> >> +			 | gicc->arm_mpidr;
> > 
> > The simple fact that you define a function to pack the mpidr value and
> > you can't use it here because this is *generic* code is telling, and
> > a very bad omen. At the cost of sounding like a broken record, I do not
> > like this mpidr->apic->logical_cpu song and dance at all.
> > ACPI is peppered with code (eg hotplug is another example, CPUidle driver
> > even worse) that is supposed to be generic but contains x86 code to carry
> > out this cpuid conversion, I really think that in order to start an ARM64
> > ACPI port properly we should at least try to factor out this physical to
> > logical cpu id conversion, and it is not the first time that I mention this
> > on the lists.
> 
> I know, thanks for pointing this out. As I replied in previous version
> of this patch set, apic_id is x86/ia64 specific, but the meaning behind
> it is not. It means the CPU hardware id to identify itself in the system,
> it just like MPIDR on ARM.
> 
> I will send out a patch for RFC to convert apic_id to physid which
> is generic for all platforms.
> 
> > 
> > I will also talk to Rafael about this at the earliest opportunity, I
> > guess that x86 code relies on apic-id because some ACPI versions could
> > not rely on the acpi-id or some other reasons I have to investigate.
> 
> As ACPI spec (section 8.4, Declaring Processors) said, Each processor
> in the system must be declared in the ACPI namespace, so each cpu will
> have acpi_id in all ACPI versions, and in theory we can map acpi_id to
> logical cpu id if we want to.
> 
> But things are complicated, apic_id is connected to many
> tables, MADT for smp init, DSDT for device driver, and SRAT for NUMA (there
> is no acpi_id in it, ONLY has apic_id in the table for x86/ia64), so if we
> want to factor the code to map acpi_id to logical cpu id, we need to modify:
> 
>  - ACPI drivers;
>  - SMP init for x86 and ia64
>  - the mappings for NUMA init for x86 and ia64
> that will be lots of work I think.

And error prone for that matter.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists