Message-ID: <54177DD8.5070004@linaro.org>
Date:	Tue, 16 Sep 2014 08:01:28 +0800
From:	Hanjun Guo <hanjun.guo@...aro.org>
To:	Olof Johansson <olof@...om.net>
CC:	Catalin Marinas <catalin.marinas@....com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Mark Rutland <mark.rutland@....com>,
	Grant Likely <grant.likely@...aro.org>,
	Will Deacon <will.deacon@....com>,
	Graeme Gregory <graeme.gregory@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	Sudeep Holla <Sudeep.Holla@....com>,
	Jon Masters <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>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	Liviu Dudau <Liviu.Dudau@....com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Charles.Garcia-Tobin@....com, linux-acpi@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linaro-acpi@...ts.linaro.org,
	Tomasz Nowicki <tomasz.nowicki@...aro.org>
Subject: Re: [PATCH v4 11/18] ARM64 / ACPI: Parse MADT for SMP initialization

On 2014年09月15日 15:00, Olof Johansson wrote:
> On Fri, Sep 12, 2014 at 10:00:09PM +0800, Hanjun Guo wrote:
>> MADT contains the information for MPIDR which is essential for
>> SMP initialization, parse the GIC cpu interface structures to
>> get the MPIDR value and map it to cpu_logical_map(), and add
>> enabled cpu with valid MPIDR into cpu_possible_map.
>>
>> ACPI 5.1 only has two explicit methods to boot up SMP, PSCI and
>> Parking protocol, but the Parking protocol is only specified for
>> ARMv7 now, so make PSCI as the only way for the SMP boot protocol
>> before some updates for the ACPI spec or the Parking protocol spec.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@...aro.org>
>> ---
>>  arch/arm64/include/asm/acpi.h    |    2 +
>>  arch/arm64/include/asm/cpu_ops.h |    1 +
>>  arch/arm64/include/asm/smp.h     |    5 +-
>>  arch/arm64/kernel/acpi.c         |  147 +++++++++++++++++++++++++++++++++++++-
>>  arch/arm64/kernel/cpu_ops.c      |    4 +-
>>  arch/arm64/kernel/setup.c        |    8 ++-
>>  arch/arm64/kernel/smp.c          |    2 +-
>>  7 files changed, 160 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>> index ecba671..da8f74a 100644
>> --- a/arch/arm64/include/asm/acpi.h
>> +++ b/arch/arm64/include/asm/acpi.h
>> @@ -51,11 +51,13 @@ static inline bool acpi_has_cpu_in_madt(void)
>>  }
>>  
>>  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>> +void __init acpi_smp_init_cpus(void);
>>  
>>  #else
>>  
>>  static inline bool acpi_psci_present(void) { return false; }
>>  static inline bool acpi_psci_use_hvc(void) { return false; }
>> +static inline void acpi_smp_init_cpus(void) { }
>>  
>>  #endif /* CONFIG_ACPI */
>>  
>> diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
>> index d7b4b38..d149580 100644
>> --- a/arch/arm64/include/asm/cpu_ops.h
>> +++ b/arch/arm64/include/asm/cpu_ops.h
>> @@ -61,6 +61,7 @@ struct cpu_operations {
>>  };
>>  
>>  extern const struct cpu_operations *cpu_ops[NR_CPUS];
>> +const struct cpu_operations *cpu_get_ops(const char *name);
>>  extern int __init cpu_read_ops(struct device_node *dn, int cpu);
>>  extern void __init cpu_read_bootcpu_ops(void);
>>  
>> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
>> index a498f2c..c877adc 100644
>> --- a/arch/arm64/include/asm/smp.h
>> +++ b/arch/arm64/include/asm/smp.h
>> @@ -39,9 +39,10 @@ extern void show_ipi_list(struct seq_file *p, int prec);
>>  extern void handle_IPI(int ipinr, struct pt_regs *regs);
>>  
>>  /*
>> - * Setup the set of possible CPUs (via set_cpu_possible)
>> + * Discover the set of possible CPUs and determine their
>> + * SMP operations.
>>   */
>> -extern void smp_init_cpus(void);
>> +extern void of_smp_init_cpus(void);
>>  
>>  /*
>>   * Provide a function to raise an IPI cross call on CPUs in callmap.
>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>> index 9e1ae37..644b8b8 100644
>> --- a/arch/arm64/kernel/acpi.c
>> +++ b/arch/arm64/kernel/acpi.c
>> @@ -24,6 +24,10 @@
>>  #include <linux/bootmem.h>
>>  #include <linux/smp.h>
>>  
>> +#include <asm/smp_plat.h>
>> +#include <asm/cputype.h>
>> +#include <asm/cpu_ops.h>
>> +
>>  int acpi_noirq;			/* skip ACPI IRQ initialization */
>>  int acpi_disabled;
>>  EXPORT_SYMBOL(acpi_disabled);
>> @@ -31,6 +35,8 @@ EXPORT_SYMBOL(acpi_disabled);
>>  int acpi_pci_disabled;		/* skip ACPI PCI scan and IRQ initialization */
>>  EXPORT_SYMBOL(acpi_pci_disabled);
>>  
>> +static int enabled_cpus;	/* Processors (GICC) with enabled flag in MADT */
>> +
>>  /*
>>   * __acpi_map_table() will be called before page_init(), so early_ioremap()
>>   * or early_memremap() should be called here to for ACPI table mapping.
>> @@ -51,6 +57,131 @@ void __init __acpi_unmap_table(char *map, unsigned long size)
>>  	early_memunmap(map, size);
>>  }
>>  
>> +/**
>> + * acpi_map_gic_cpu_interface - generates a logical cpu number
>> + * and map to MPIDR represented by GICC structure
>> + * @mpidr: CPU's hardware id to register, MPIDR represented in MADT
>> + * @enabled: this cpu is enabled or not
>> + *
>> + * Returns the logical cpu number which maps to MPIDR
>> + */
>> +static int acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled)
>> +{
>> +	int cpu;
>> +
>> +	if (mpidr == INVALID_HWID) {
>> +		pr_info("Skip invalid cpu hardware ID\n");
> This message, when showing up in dmesg, will probably mostly just make
> someone scratch their head. Something slightly more descriptive would
> be a good idea.

I agree. how about "Skip MADT cpu entry with invalid MPIDR" ?

>
>> +		return -EINVAL;
>> +	}
>> +
>> +	total_cpus++;
>> +	if (!enabled)
>> +		return -EINVAL;
>> +
>> +	if (enabled_cpus >=  NR_CPUS) {
>> +		pr_warn("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n",
>> +			NR_CPUS, total_cpus, mpidr);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* No need to check duplicate MPIDRs for the first CPU */
>> +	if (enabled_cpus) {
>> +		/*
>> +		 * Duplicate MPIDRs are a recipe for disaster. Scan
>> +		 * all initialized entries and check for
>> +		 * duplicates. If any is found just ignore the CPU.
>> +		 */
> But is it this entry or the other one that should be ignored? 

Only this entry will be ignored, I did the same thing as DT parsing
CPU nodes.

> Is
> this expected to be something that firmware vendors get wrong all the
> time? Would it be better to abort SMP alltogether?

I think we can still boot other CPUs with correct MPIDRs, and ignore
the wrong ones.

>
>> +		for_each_possible_cpu(cpu) {
>> +			if (cpu_logical_map(cpu) == mpidr) {
>> +				pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
>> +				mpidr);
> Misindented.

Will update.

>
>> +				return -EINVAL;
>> +			}
>> +		}
>> +		
>> +		/* allocate a logical cpu id for the new comer */
>> +		cpu = cpumask_next_zero(-1, cpu_possible_mask);
>> +	} else {
>> +		/* First GICC entry must be BSP as ACPI spec said */
> "spec said", would be nice to have a chapter/section reference.

ok.

>
>> +		if  (cpu_logical_map(0) != mpidr) {
>> +			pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n",
>> +			       mpidr);
> Same thing here about usefulness of error message. What is a user to do with this?

I'm using this to debug the MADT table in UEFI, and correct them if this printed.

>
>> +			return -EINVAL;
>> +		}
>> +
>> +		/*
>> +		 * boot_cpu_init() already hold bit 0 in cpu_present_mask
>> +		 * for BSP, no need to allocate again.
>> +		 */
>> +		cpu = 0;
>> +	}
>> +
>> +	/* CPU 0 was already initialized */
>> +	if (cpu) {
>> +		cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
>> +		if (!cpu_ops[cpu])
>> +			return -EINVAL;
>> +
>> +		if (cpu_ops[cpu]->cpu_init(NULL, cpu))
>> +			return -EOPNOTSUPP;
>> +
>> +		/* map the logical cpu id to cpu MPIDR */
>> +		cpu_logical_map(cpu) = mpidr;
>> +
>> +		set_cpu_possible(cpu, true);
>> +	} else {
>> +		/* get cpu0's ops, no need to return if ops is null */
>> +		cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
>> +	}
>> +
>> +	enabled_cpus++;
>> +	return cpu;
>> +}
>> +
>> +static int __init
>> +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
>> +				const unsigned long end)
>> +{
>> +	struct acpi_madt_generic_interrupt *processor;
>> +
>> +	processor = (struct acpi_madt_generic_interrupt *)header;
>> +
>> +	if (BAD_MADT_ENTRY(processor, end))
>> +		return -EINVAL;
>> +
>> +	acpi_table_print_madt_entry(header);
>> +
>> +	acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK,
>> +		processor->flags & ACPI_MADT_ENABLED);
>> +
>> +	return 0;
>> +}
>> +
>> +/* Parse GIC cpu interface entries in MADT for SMP init */
>> +void __init acpi_smp_init_cpus(void)
>> +{
>> +	int count;
>> +
>> +	/*
>> +	 * do a partial walk of MADT to determine how many CPUs
>> +	 * we have including disabled CPUs, and get information
>> +	 * we need for SMP init
>> +	 */
>> +	count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>> +			acpi_parse_gic_cpu_interface, 0);
>> +
>> +	if (!count) {
>> +		pr_err("No GIC CPU interface entries present\n");
>> +		return;
>> +	} else if (count < 0) {
>> +		pr_err("Error parsing GIC CPU interface entry\n");
>> +		return;
>> +	}
>> +
>> +	/* Make boot-up look pretty */
>> +	pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus);
>> +}
>> +
>>  static int __init acpi_parse_fadt(struct acpi_table_header *table)
>>  {
>>  	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
>> @@ -62,8 +193,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>>  	 * to get arm boot flags, or we will disable ACPI.
>>  	 */
>>  	if (table->revision > 5 ||
>> -	    (table->revision == 5 && fadt->minor_revision >= 1))
>> -		return 0;
>> +	    (table->revision == 5 && fadt->minor_revision >= 1)) {
>> +		/*
>> +		 * ACPI 5.1 only has two explicit methods to boot up SMP,
>> +		 * PSCI and Parking protocol, but the Parking protocol is
>> +		 * only specified for ARMv7 now, so make PSCI as the only
>> +		 * way for the SMP boot protocol before some updates for
>> +		 * the ACPI spec or the Parking protocol spec.
>> +		 */
>> +		if (acpi_psci_present())
>> +			return 0;
>> +
>> +		pr_warn("has no PSCI support, will not bring up secondary CPUs\n");
> s/has//
>
>> +		return -EOPNOTSUPP;
>> +	}
>>  
>>  	pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n",
>>  		table->revision, fadt->minor_revision);
>> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
>> index cce9524..1a04deb 100644
>> --- a/arch/arm64/kernel/cpu_ops.c
>> +++ b/arch/arm64/kernel/cpu_ops.c
>> @@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops;
>>  
>>  const struct cpu_operations *cpu_ops[NR_CPUS];
>>  
>> -static const struct cpu_operations *supported_cpu_ops[] __initconst = {
>> +static const struct cpu_operations *supported_cpu_ops[] = {
>>  #ifdef CONFIG_SMP
>>  	&smp_spin_table_ops,
>>  #endif
>> @@ -35,7 +35,7 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = {
>>  	NULL,
>>  };
>>  
>> -static const struct cpu_operations * __init cpu_get_ops(const char *name)
>> +const struct cpu_operations *cpu_get_ops(const char *name)
>>  {
>>  	const struct cpu_operations **ops = supported_cpu_ops;
>>  
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index 7ba20d4..281fa34 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -60,6 +60,7 @@
>>  #include <asm/memblock.h>
>>  #include <asm/psci.h>
>>  #include <asm/efi.h>
>> +#include <asm/acpi.h>
>>  
>>  unsigned int processor_id;
>>  EXPORT_SYMBOL(processor_id);
>> @@ -401,13 +402,16 @@ void __init setup_arch(char **cmdline_p)
>>  	if (acpi_disabled) {
>>  		unflatten_device_tree();
>>  		psci_dt_init();
>> +		cpu_read_bootcpu_ops();
>> +#ifdef CONFIG_SMP
>> +		of_smp_init_cpus();
>> +#endif
> Please create a !SMP stub so you can do without the ifdef here, just
> like you did for the ACPI case.

I like this, will update the patch. :)

Thanks
Hanjun
--
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