[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Grant Likely]

On Mon,  2 Dec 2013 23:44:47 +0800, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> When boot the kernel with MADT, the cpu possible and present maps should be
> prefilled for cpu topology and acpi based cpu hot-plug.
> 
> The logic cpu id maps to APIC id (GIC id) is also implemented, it is needed
> for acpi processor drivers.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
[...]
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index a0c2ca6..1428024 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -420,7 +420,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  		if (err)
>  			continue;
>  
> +#ifndef CONFIG_ACPI
>  		set_cpu_present(cpu, true);
> +#endif
>  		max_cpus--;
>  	}
>  }

This looks wrong. Will this break non-ACPI booting when CONFIG_ACPI is
enabled? The decision on whether or not to run code must be made at
runtime.

g.

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Hanjun Guo]

On 2013?12?10? 20:53, Grant Likely wrote:
> On Mon,  2 Dec 2013 23:44:47 +0800, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>> When boot the kernel with MADT, the cpu possible and present maps should be
>> prefilled for cpu topology and acpi based cpu hot-plug.
>>
>> The logic cpu id maps to APIC id (GIC id) is also implemented, it is needed
>> for acpi processor drivers.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
> [...]
>> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>> index a0c2ca6..1428024 100644
>> --- a/arch/arm64/kernel/smp.c
>> +++ b/arch/arm64/kernel/smp.c
>> @@ -420,7 +420,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>>   		if (err)
>>   			continue;
>>   
>> +#ifndef CONFIG_ACPI
>>   		set_cpu_present(cpu, true);
>> +#endif
>>   		max_cpus--;
>>   	}
>>   }
> This looks wrong. Will this break non-ACPI booting when CONFIG_ACPI is
> enabled? The decision on whether or not to run code must be made at
> runtime.

Yes, you are right. I'm reworking on this patch now.

Thanks
Hanjun

[RFC part2 PATCH 4/9] ARM64 / ACPI: Use Parked Address in GIC structure for spin table SMP initialisation

[Grant Likely]

On Mon,  2 Dec 2013 23:44:49 +0800, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> Parked Address in GIC structure can be used as cpu release address
> for spin table SMP initialisation.
> 
> This patch gets parked address from MADT and use it for SMP
> initialisation when DT is not available.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/include/asm/acpi.h      |    3 +++
>  arch/arm64/kernel/smp_spin_table.c |   16 +++++++++---
>  drivers/acpi/plat/arm-core.c       |   50 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 66 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 423a32c..180de4a 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -90,6 +90,9 @@ extern int boot_cpu_apic_id;
>  
>  extern void prefill_possible_map(void);
>  
> +extern int acpi_get_parked_address_with_gic_id(u32 gic_id,
> +			u64 *parked_address);
> +
>  #else	/* !CONFIG_ACPI */
>  #define acpi_disabled 1		/* ACPI sometimes enabled on ARM */
>  #define acpi_noirq 1		/* ACPI sometimes enabled on ARM */
> diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c
> index 44c2280..7997873 100644
> --- a/arch/arm64/kernel/smp_spin_table.c
> +++ b/arch/arm64/kernel/smp_spin_table.c
> @@ -25,6 +25,7 @@
>  #include <asm/cpu_ops.h>
>  #include <asm/cputype.h>
>  #include <asm/smp_plat.h>
> +#include <asm/acpi.h>
>  
>  extern void secondary_holding_pen(void);
>  volatile unsigned long secondary_holding_pen_release = INVALID_HWID;
> @@ -47,6 +48,11 @@ static void write_pen_release(u64 val)
>  	__flush_dcache_area(start, size);
>  }
>  
> +static int get_cpu_release_addr_acpi(unsigned int cpu, u64 *parked_address)
> +{
> +	return acpi_get_parked_address_with_gic_id(arm_cpu_to_apicid[cpu],
> +						parked_address);
> +}
>  
>  static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu)
>  {
> @@ -55,10 +61,14 @@ static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu)
>  	 */
>  	if (of_property_read_u64(dn, "cpu-release-addr",
>  				 &cpu_release_addr[cpu])) {
> -		pr_err("CPU %d: missing or invalid cpu-release-addr property\n",
> -		       cpu);
>  
> -		return -1;
> +		/* try ACPI way */
> +		if (get_cpu_release_addr_acpi(cpu, &cpu_release_addr[cpu])) {
> +			pr_err("CPU %d: missing or invalid cpu-release-addr property\n",
> +				cpu);
> +
> +			return -1;
> +		}
>  	}
>  
>  	return 0;
> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
> index 8527ecc..c4c8c68 100644
> --- a/drivers/acpi/plat/arm-core.c
> +++ b/drivers/acpi/plat/arm-core.c
> @@ -262,6 +262,56 @@ static int __init acpi_parse_madt_gic_entries(void)
>  	return 0;
>  }
>  
> +/* Parked Address in ACPI GIC structure can be used as cpu release addr */
> +int acpi_get_parked_address_with_gic_id(u32 gic_id, u64 *parked_address)
> +{
> +	struct acpi_table_header *table_header = NULL;
> +	struct acpi_subtable_header *entry;
> +	int err = 0;
> +	unsigned long table_end;
> +	acpi_size tbl_size;
> +	struct acpi_madt_generic_interrupt *processor = NULL;
> +
> +	if (!parked_address)
> +		return -EINVAL;
> +
> +	acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table_header, &tbl_size);
> +	if (!table_header) {
> +		pr_warn(PREFIX "MADT table not present\n");
> +		return -ENODEV;
> +	}
> +
> +	table_end = (unsigned long)table_header + table_header->length;
> +
> +	/* Parse all entries looking for a match. */
> +	entry = (struct acpi_subtable_header *)
> +	    ((unsigned long)table_header + sizeof(struct acpi_table_madt));
> +
> +	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
> +	       table_end) {
> +		if (entry->type != ACPI_MADT_TYPE_GENERIC_INTERRUPT
> +			|| BAD_MADT_ENTRY(entry, table_end))
> +			continue;
> +
> +		processor = (struct acpi_madt_generic_interrupt *)entry;
> +
> +		if (processor->gic_id == gic_id) {
> +			*parked_address = processor->parked_address;
> +			goto out;
> +		}
> +
> +		entry = (struct acpi_subtable_header *)
> +		    ((unsigned long)entry + entry->length);

All of the casting in this table looks suspicious. If you have to resort
to casting, then the variable types are very likely wrong.

In the case immediately above, it seems that the entry size doesn't
necessarily equal the acpi_subtable_header size, in which case you
should cast the values to a void* instead of an unsigned long. That
would mean you can do this:

	entry = ((void*)entry) + entry->length;

In fact, if I were writing the code, I would have two variables; the
iterator pointer as a void* and a header pointer as a struct
acpi_subtable_header*. Like so:

	void *entry, *table_end;
	struct acpi_subtable_header *header;

	entry = ((void*)table_header) + sizeof(struct acpi_table_madt);
	table_end = ((void*)table_header) + table_header->length;
	while (entry + sizeof(*header)) < table_end) {
		header = entry;

		if (header->type != ACPI_MADT_TYPE_GENERIC_INTERRUPT ||
			BAD_MADT_ENTRY(entry, table_end))
			continue;
		processor = entry;

		if (processor->gic_id == gic_id) {
			*parked_address = processor->parked_address;
			goto out;
		}

		entry += header->length;
	}

See? Much cleaner code.

g.

[RFC part2 PATCH 4/9] ARM64 / ACPI: Use Parked Address in GIC structure for spin table SMP initialisation

[Hanjun Guo]

On 2013-12-10 21:03, Grant Likely wrote:
[...]
>> +/* Parked Address in ACPI GIC structure can be used as cpu release addr */
>> +int acpi_get_parked_address_with_gic_id(u32 gic_id, u64 *parked_address)
>> +{
>> +	struct acpi_table_header *table_header = NULL;
>> +	struct acpi_subtable_header *entry;
>> +	int err = 0;
>> +	unsigned long table_end;
>> +	acpi_size tbl_size;
>> +	struct acpi_madt_generic_interrupt *processor = NULL;
>> +
>> +	if (!parked_address)
>> +		return -EINVAL;
>> +
>> +	acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table_header, &tbl_size);
>> +	if (!table_header) {
>> +		pr_warn(PREFIX "MADT table not present\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	table_end = (unsigned long)table_header + table_header->length;
>> +
>> +	/* Parse all entries looking for a match. */
>> +	entry = (struct acpi_subtable_header *)
>> +	    ((unsigned long)table_header + sizeof(struct acpi_table_madt));
>> +
>> +	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>> +	       table_end) {
>> +		if (entry->type != ACPI_MADT_TYPE_GENERIC_INTERRUPT
>> +			|| BAD_MADT_ENTRY(entry, table_end))
>> +			continue;
>> +
>> +		processor = (struct acpi_madt_generic_interrupt *)entry;
>> +
>> +		if (processor->gic_id == gic_id) {
>> +			*parked_address = processor->parked_address;
>> +			goto out;
>> +		}
>> +
>> +		entry = (struct acpi_subtable_header *)
>> +		    ((unsigned long)entry + entry->length);
> 
> All of the casting in this table looks suspicious. If you have to resort
> to casting, then the variable types are very likely wrong.
> 
> In the case immediately above, it seems that the entry size doesn't
> necessarily equal the acpi_subtable_header size, in which case you
> should cast the values to a void* instead of an unsigned long. That
> would mean you can do this:
> 
> 	entry = ((void*)entry) + entry->length;
> 
> In fact, if I were writing the code, I would have two variables; the
> iterator pointer as a void* and a header pointer as a struct
> acpi_subtable_header*. Like so:
> 
> 	void *entry, *table_end;
> 	struct acpi_subtable_header *header;
> 
> 	entry = ((void*)table_header) + sizeof(struct acpi_table_madt);
> 	table_end = ((void*)table_header) + table_header->length;
> 	while (entry + sizeof(*header)) < table_end) {
> 		header = entry;
> 
> 		if (header->type != ACPI_MADT_TYPE_GENERIC_INTERRUPT ||
> 			BAD_MADT_ENTRY(entry, table_end))
> 			continue;
> 		processor = entry;
> 
> 		if (processor->gic_id == gic_id) {
> 			*parked_address = processor->parked_address;
> 			goto out;
> 		}
> 
> 		entry += header->length;
> 	}
> 
> See? Much cleaner code.

Aha, much much cleaner, thanks for the guidance, will rework my patch
and test it.

Hanjun

[RFC part2 PATCH 8/9] ACPI / ARM64: Update acpi_register_gsi to register with the core IRQ subsystem

[Grant Likely]

On Mon,  2 Dec 2013 23:44:53 +0800, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> This API is similar to DT based irq_of_parse_and_map but does link
> parent/child IRQ controllers. This is tested for primary GIC PPI and GIC SPI
> interrupts and not for secondary child irq controllers.
> 
> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  drivers/acpi/plat/arm-core.c |   36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
> index 9cc0208..17c99e1 100644
> --- a/drivers/acpi/plat/arm-core.c
> +++ b/drivers/acpi/plat/arm-core.c
> @@ -90,7 +90,7 @@ enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_GIC;
>  
>  static unsigned int gsi_to_irq(unsigned int gsi)
>  {
> -	int irq = irq_create_mapping(NULL, gsi);
> +	int irq = irq_find_mapping(NULL, gsi);

I suspect this will break FDT users that depend on the old behaviour.

g.

>  
>  	return irq;
>  }
> @@ -407,7 +407,39 @@ EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
>   */
>  int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
>  {
> -	return -1;
> +	unsigned int irq;
> +	unsigned int irq_type;
> + 
> +	/*
> +	 * ACPI have no bindings to indicate SPI or PPI, so we
> +	 * use different mappings from DT in ACPI.
> +	 *
> +	 * For FDT
> +	 * PPI interrupt: in the range [0, 15];
> +	 * SPI interrupt: in the range [0, 987];
> +	 *
> +	 * For ACPI, using identity mapping for hwirq:
> +	 * PPI interrupt: in the range [16, 31];
> +	 * SPI interrupt: in the range [32, 1019];
> +	 */
> +
> +	if (trigger == ACPI_EDGE_SENSITIVE &&
> +				polarity == ACPI_ACTIVE_LOW)
> +		irq_type = IRQ_TYPE_EDGE_FALLING;
> +	else if (trigger == ACPI_EDGE_SENSITIVE &&
> +				polarity == ACPI_ACTIVE_HIGH)
> +		irq_type = IRQ_TYPE_EDGE_RISING;
> +	else if (trigger == ACPI_LEVEL_SENSITIVE &&
> +				polarity == ACPI_ACTIVE_LOW)
> +		irq_type = IRQ_TYPE_LEVEL_LOW;
> +	else if (trigger == ACPI_LEVEL_SENSITIVE &&
> +				polarity == ACPI_ACTIVE_HIGH)
> +		irq_type = IRQ_TYPE_LEVEL_HIGH;
> +	else
> +		irq_type = IRQ_TYPE_NONE;
> +
> +	irq = irq_create_acpi_mapping(gsi, irq_type);
> + 	return irq; 
>  }
>  EXPORT_SYMBOL_GPL(acpi_register_gsi);
>  
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

[RFC part2 PATCH 8/9] ACPI / ARM64: Update acpi_register_gsi to register with the core IRQ subsystem

[Arnd Bergmann]

On Tuesday 10 December 2013, Grant Likely wrote:
> > --- a/drivers/acpi/plat/arm-core.c
> > +++ b/drivers/acpi/plat/arm-core.c
> > @@ -90,7 +90,7 @@ enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_GIC;
> >  
> >  static unsigned int gsi_to_irq(unsigned int gsi)
> >  {
> > -     int irq = irq_create_mapping(NULL, gsi);
> > +     int irq = irq_find_mapping(NULL, gsi);
> 
> I suspect this will break FDT users that depend on the old behaviour.

I think not, given this is only in drivers/acpi and gets added in one
of the prior patches of the same series.

	Arnd

[RFC part2 PATCH 8/9] ACPI / ARM64: Update acpi_register_gsi to register with the core IRQ subsystem

[Grant Likely]

On Wed, 11 Dec 2013 06:23:04 +0100, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 10 December 2013, Grant Likely wrote:
> > > --- a/drivers/acpi/plat/arm-core.c
> > > +++ b/drivers/acpi/plat/arm-core.c
> > > @@ -90,7 +90,7 @@ enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_GIC;
> > >  
> > >  static unsigned int gsi_to_irq(unsigned int gsi)
> > >  {
> > > -     int irq = irq_create_mapping(NULL, gsi);
> > > +     int irq = irq_find_mapping(NULL, gsi);
> > 
> > I suspect this will break FDT users that depend on the old behaviour.
> 
> I think not, given this is only in drivers/acpi and gets added in one
> of the prior patches of the same series.

Ah, okay.

g.

[RFC part2 PATCH 0/9] Using ACPI MADT table to initialise SMP and GIC

[Hanjun Guo]

This patch set is based on part1 "Make ACPI core running on ARM64" patch
set.

After we can get the ACPI tables from UEFI, we can use these tables
to initialise the system now.

GIC (means GIC cpu interface) structure and GIC distributor structure in
MADT table contains the information of GIC cpu interface base address
and GIC distributor base address, which can be used to initialise GIC.

Further more, parked address in GIC structure can be used as cpu release
address for spin table SMP initialisation.

This patch set use these information to init SMP and GIC.

Please refer to chapter 5.2.12.14/15 of ACPI 5.0 spec for GIC and GIC
distributor structure information.

Amit Daniel Kachhap (1):
  irqdomain: Add a new API irq_create_acpi_mapping()

Hanjun Guo (8):
  ARM64 / ACPI: Implement core functions for parsing MADT table
  ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu
    id to APIC id
  ARM64 / ACPI: Introduce map_gic_id() to get apic id from MADT or _MAT
    method
  ARM64 / ACPI: Use Parked Address in GIC structure for spin table SMP
    initialisation
  ACPI: Define ACPI_IRQ_MODEL_GIC needed for arm
  Irqchip / gic: Set as default domain so we can access from ACPI
  ACPI / ARM64: Update acpi_register_gsi to register with the core IRQ
    subsystem
  ACPI / GIC: Initialize GIC using the information in MADT

 arch/arm64/include/asm/acpi.h      |   16 +-
 arch/arm64/kernel/irq.c            |    5 +
 arch/arm64/kernel/setup.c          |    2 +
 arch/arm64/kernel/smp.c            |    2 +
 arch/arm64/kernel/smp_spin_table.c |   16 +-
 drivers/acpi/bus.c                 |    3 +
 drivers/acpi/plat/arm-core.c       |  397 +++++++++++++++++++++++++++++++++++-
 drivers/acpi/processor_core.c      |   26 +++
 drivers/acpi/tables.c              |   21 ++
 drivers/irqchip/irq-gic.c          |    7 +
 include/linux/acpi.h               |    9 +
 kernel/irq/irqdomain.c             |   27 +++
 12 files changed, 521 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Hanjun Guo]

When boot the kernel with MADT, the cpu possible and present maps should be
prefilled for cpu topology and acpi based cpu hot-plug.

The logic cpu id maps to APIC id (GIC id) is also implemented, it is needed
for acpi processor drivers.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/acpi.h |   10 ++--
 arch/arm64/kernel/setup.c     |    2 +
 arch/arm64/kernel/smp.c       |    2 +
 drivers/acpi/plat/arm-core.c  |  118 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 129 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index be2951c..423a32c 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -76,9 +76,6 @@ static inline void acpi_disable_pci(void)
 /* FIXME: this function should be moved to topology.h when it's ready */
 void arch_fix_phys_package_id(int num, u32 slot);
 
-/* temperally define -1 to make acpi core compilerable */
-#define cpu_physical_id(cpu) -1
-
 /* Low-level suspend routine. */
 extern int (*acpi_suspend_lowlevel)(void);
 #define acpi_wakeup_address (0)
@@ -86,6 +83,13 @@ extern int (*acpi_suspend_lowlevel)(void);
 #define MAX_GIC_CPU_INTERFACE 256
 #define MAX_GIC_DISTRIBUTOR   1		/* should be the same as MAX_GIC_NR */
 
+/* map logic cpu id to physical GIC id */
+extern int arm_cpu_to_apicid[NR_CPUS];
+extern int boot_cpu_apic_id;
+#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu]
+
+extern void prefill_possible_map(void);
+
 #else	/* !CONFIG_ACPI */
 #define acpi_disabled 1		/* ACPI sometimes enabled on ARM */
 #define acpi_noirq 1		/* ACPI sometimes enabled on ARM */
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 8199360..08f11e2 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -231,7 +231,9 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	acpi_boot_table_init();
 	early_acpi_boot_init();
+	boot_cpu_apic_id = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
 	acpi_boot_init();
+	prefill_possible_map();
 
 	paging_init();
 	request_standard_resources();
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index a0c2ca6..1428024 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -420,7 +420,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 		if (err)
 			continue;
 
+#ifndef CONFIG_ACPI
 		set_cpu_present(cpu, true);
+#endif
 		max_cpus--;
 	}
 }
diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
index 45ff625..8527ecc 100644
--- a/drivers/acpi/plat/arm-core.c
+++ b/drivers/acpi/plat/arm-core.c
@@ -58,6 +58,13 @@ EXPORT_SYMBOL(acpi_pci_disabled);
  */
 static u64 acpi_lapic_addr __initdata;
 
+/* available_cpus here means enabled cpu in MADT */
+int available_cpus;
+
+/* Map logic cpu id to physical GIC id. */
+int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 };
+int boot_cpu_apic_id = -1;
+
 #define BAD_MADT_ENTRY(entry, end) (					\
 	(!entry) || (unsigned long)entry + sizeof(*entry) > end ||	\
 	((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
@@ -142,6 +149,39 @@ static int __init acpi_parse_madt(struct acpi_table_header *table)
  * Please refer to chapter5.2.12.14/15 of ACPI 5.0
  */
 
+static void acpi_register_gic_cpu_interface(int id, u8 enabled)
+{
+	int cpu;
+
+	if (id >= MAX_GIC_CPU_INTERFACE) {
+		pr_info(PREFIX "skipped apicid that is too big\n");
+		return;
+	}
+
+	total_cpus++;
+	if (!enabled)
+		return;
+
+	available_cpus++;
+
+	/* allocate a logic cpu id for the new comer */
+	if (boot_cpu_apic_id == id) {
+		/*
+		 * boot_cpu_init() already hold bit 0 in cpu_present_mask
+		 * for BSP, no need to allocte again.
+		 */
+		cpu = 0;
+	} else {
+		cpu = cpumask_next_zero(-1, cpu_present_mask);
+	}
+
+	/* map the logic cpu id to APIC id */
+	arm_cpu_to_apicid[cpu] = id;
+
+	set_cpu_present(cpu, true);
+	set_cpu_possible(cpu, true);
+}
+
 static int __init
 acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
 {
@@ -154,6 +194,16 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
 
 	acpi_table_print_madt_entry(header);
 
+	/*
+	 * We need to register disabled CPU as well to permit
+	 * counting disabled CPUs. This allows us to size
+	 * cpus_possible_map more accurately, to permit
+	 * to not preallocating memory for all NR_CPUS
+	 * when we use CPU hotplug.
+	 */
+	acpi_register_gic_cpu_interface(processor->gic_id,
+			processor->flags & ACPI_MADT_ENABLED);
+
 	return 0;
 }
 
@@ -196,6 +246,19 @@ static int __init acpi_parse_madt_gic_entries(void)
 		return count;
 	}
 
+#ifdef CONFIG_SMP
+	if (available_cpus == 0) {
+		pr_info(PREFIX "Found 0 CPUs; assuming 1\n");
+		/* FIXME: should be the real GIC id read from hardware */
+		arm_cpu_to_apicid[available_cpus] = 0;
+		available_cpus = 1;	/* We've got at least one of these */
+	}
+#endif
+
+	/* Make boot-up look pretty */
+	pr_info("%d CPUs available, %d CPUs total\n", available_cpus,
+		total_cpus);
+
 	return 0;
 }
 
@@ -221,6 +284,61 @@ static int __init acpi_parse_madt_gic_distributor_entries(void)
 	return 0;
 }
 
+static int setup_possible_cpus __initdata = -1;
+static int __init _setup_possible_cpus(char *str)
+{
+	get_option(&str, &setup_possible_cpus);
+	return 0;
+}
+early_param("possible_cpus", _setup_possible_cpus);
+
+/*
+ * cpu_possible_mask should be static, it cannot change as cpu's
+ * are onlined, or offlined. The reason is per-cpu data-structures
+ * are allocated by some modules at init time, and dont expect to
+ * do this dynamically on cpu arrival/departure.
+ * cpu_present_mask on the other hand can change dynamically.
+ * In case when cpu_hotplug is not compiled, then we resort to current
+ * behaviour, which is cpu_possible == cpu_present.
+ * - Ashok Raj
+ *
+ * Three ways to find out the number of additional hotplug CPUs:
+ * - If the BIOS specified disabled CPUs in ACPI/mptables use that.
+ * - The user can overwrite it with possible_cpus=NUM
+ * - Otherwise don't reserve additional CPUs.
+ * We do this because additional CPUs waste a lot of memory.
+ * -AK
+ */
+void __init prefill_possible_map(void)
+{
+	int i;
+	int possible, disabled_cpus;
+
+	disabled_cpus = total_cpus - available_cpus;
+
+	if (setup_possible_cpus == -1) {
+		if (disabled_cpus > 0)
+			setup_possible_cpus = disabled_cpus;
+		else
+			setup_possible_cpus = 0;
+	}
+
+	possible = available_cpus + setup_possible_cpus;
+
+	pr_info("SMP: the system is limited to %d CPUs\n", nr_cpu_ids);
+
+	if (possible > nr_cpu_ids)
+		possible = nr_cpu_ids;
+
+	pr_info("SMP: Allowing %d CPUs, %d hotplug CPUs\n",
+		possible, max((possible - available_cpus), 0));
+
+	for (i = 0; i < possible; i++)
+		set_cpu_possible(i, true);
+	for (; i < NR_CPUS; i++)
+		set_cpu_possible(i, false);
+}
+
 int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
 {
 	*irq = gsi_to_irq(gsi);
-- 
1.7.9.5

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[One Thousand Gnomes]

> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
> index 45ff625..8527ecc 100644
> --- a/drivers/acpi/plat/arm-core.c
> +++ b/drivers/acpi/plat/arm-core.c
> @@ -58,6 +58,13 @@ EXPORT_SYMBOL(acpi_pci_disabled);
>   */
>  static u64 acpi_lapic_addr __initdata;
>  
> +/* available_cpus here means enabled cpu in MADT */
> +int available_cpus;
> +
> +/* Map logic cpu id to physical GIC id. */
> +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 };
> +int boot_cpu_apic_id = -1;
> +

static ?

Really shouldn't be leaking names like "available_cpus" out of ACPI into
the global namespace


> +#ifdef CONFIG_SMP
> +	if (available_cpus == 0) {
> +		pr_info(PREFIX "Found 0 CPUs; assuming 1\n");
> +		/* FIXME: should be the real GIC id read from hardware */
> +		arm_cpu_to_apicid[available_cpus] = 0;
> +		available_cpus = 1;	/* We've got at least one of these */
> +	}
> +#endif

Isn't this true uniprocessor (by definition in fact)


> + */
> +void __init prefill_possible_map(void)

leaking more unprefixed names into the global namespace

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Hanjun Guo]

On 2013?12?04? 00:57, One Thousand Gnomes wrote:
>> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
>> index 45ff625..8527ecc 100644
>> --- a/drivers/acpi/plat/arm-core.c
>> +++ b/drivers/acpi/plat/arm-core.c
>> @@ -58,6 +58,13 @@ EXPORT_SYMBOL(acpi_pci_disabled);
>>    */
>>   static u64 acpi_lapic_addr __initdata;
>>   
>> +/* available_cpus here means enabled cpu in MADT */
>> +int available_cpus;
>> +
>> +/* Map logic cpu id to physical GIC id. */
>> +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 };
>> +int boot_cpu_apic_id = -1;
>> +
> static ?
>
> Really shouldn't be leaking names like "available_cpus" out of ACPI into
> the global namespace

Ok, will update in next version.


>> +#ifdef CONFIG_SMP
>> +	if (available_cpus == 0) {
>> +		pr_info(PREFIX "Found 0 CPUs; assuming 1\n");
>> +		/* FIXME: should be the real GIC id read from hardware */
>> +		arm_cpu_to_apicid[available_cpus] = 0;
>> +		available_cpus = 1;	/* We've got at least one of these */
>> +	}
>> +#endif
> Isn't this true uniprocessor (by definition in fact)

This code is intend to handle some buggy firmware I think.


>> + */
>> +void __init prefill_possible_map(void)
> leaking more unprefixed names into the global namespace

prefill_possible_map() will be called in setup_arch() in setup.c,
and should be gloabl, is this incorrect?
Look forward to your advice

Thanks
Hanjun

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Rob Herring]

On Wed, Dec 4, 2013 at 8:21 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> On 2013?12?04? 00:57, One Thousand Gnomes wrote:
>>>
>>> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
>>> index 45ff625..8527ecc 100644
>>> --- a/drivers/acpi/plat/arm-core.c
>>> +++ b/drivers/acpi/plat/arm-core.c
>>> @@ -58,6 +58,13 @@ EXPORT_SYMBOL(acpi_pci_disabled);
>>>    */
>>>   static u64 acpi_lapic_addr __initdata;
>>>   +/* available_cpus here means enabled cpu in MADT */
>>> +int available_cpus;
>>> +
>>> +/* Map logic cpu id to physical GIC id. */
>>> +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 };
>>> +int boot_cpu_apic_id = -1;
>>> +
>>
>> static ?
>>
>> Really shouldn't be leaking names like "available_cpus" out of ACPI into
>> the global namespace
>
>
> Ok, will update in next version.
>
>
>
>>> +#ifdef CONFIG_SMP
>>> +       if (available_cpus == 0) {
>>> +               pr_info(PREFIX "Found 0 CPUs; assuming 1\n");
>>> +               /* FIXME: should be the real GIC id read from hardware */
>>> +               arm_cpu_to_apicid[available_cpus] = 0;
>>> +               available_cpus = 1;     /* We've got at least one of
>>> these */
>>> +       }
>>> +#endif
>>
>> Isn't this true uniprocessor (by definition in fact)
>
>
> This code is intend to handle some buggy firmware I think.

Really? We have production firmware already that we need to
work-around? That's impressive given there is no production h/w.

Rob

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Rob Herring]

On Tue, Dec 3, 2013 at 10:39 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> When boot the kernel with MADT, the cpu possible and present maps should be
> prefilled for cpu topology and acpi based cpu hot-plug.
>
> The logic cpu id maps to APIC id (GIC id) is also implemented, it is needed
> for acpi processor drivers.
>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/include/asm/acpi.h |   10 ++--
>  arch/arm64/kernel/setup.c     |    2 +
>  arch/arm64/kernel/smp.c       |    2 +
>  drivers/acpi/plat/arm-core.c  |  118 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 129 insertions(+), 3 deletions(-)

[snip]

> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index a0c2ca6..1428024 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -420,7 +420,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>                 if (err)
>                         continue;
>
> +#ifndef CONFIG_ACPI
>                 set_cpu_present(cpu, true);
> +#endif

Should this be moved to DT cpu topology related code?

>                 max_cpus--;
>         }
>  }
> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
> index 45ff625..8527ecc 100644
> --- a/drivers/acpi/plat/arm-core.c
> +++ b/drivers/acpi/plat/arm-core.c

[snip]

> @@ -221,6 +284,61 @@ static int __init acpi_parse_madt_gic_distributor_entries(void)
>         return 0;
>  }
>
> +static int setup_possible_cpus __initdata = -1;
> +static int __init _setup_possible_cpus(char *str)
> +{
> +       get_option(&str, &setup_possible_cpus);
> +       return 0;
> +}
> +early_param("possible_cpus", _setup_possible_cpus);

This does not seem ACPI or ARM specific.

> +
> +/*
> + * cpu_possible_mask should be static, it cannot change as cpu's
> + * are onlined, or offlined. The reason is per-cpu data-structures
> + * are allocated by some modules at init time, and dont expect to
> + * do this dynamically on cpu arrival/departure.
> + * cpu_present_mask on the other hand can change dynamically.
> + * In case when cpu_hotplug is not compiled, then we resort to current
> + * behaviour, which is cpu_possible == cpu_present.
> + * - Ashok Raj
> + *
> + * Three ways to find out the number of additional hotplug CPUs:
> + * - If the BIOS specified disabled CPUs in ACPI/mptables use that.
> + * - The user can overwrite it with possible_cpus=NUM
> + * - Otherwise don't reserve additional CPUs.
> + * We do this because additional CPUs waste a lot of memory.
> + * -AK
> + */
> +void __init prefill_possible_map(void)
> +{
> +       int i;
> +       int possible, disabled_cpus;
> +
> +       disabled_cpus = total_cpus - available_cpus;
> +
> +       if (setup_possible_cpus == -1) {
> +               if (disabled_cpus > 0)
> +                       setup_possible_cpus = disabled_cpus;
> +               else
> +                       setup_possible_cpus = 0;
> +       }
> +
> +       possible = available_cpus + setup_possible_cpus;
> +
> +       pr_info("SMP: the system is limited to %d CPUs\n", nr_cpu_ids);
> +
> +       if (possible > nr_cpu_ids)
> +               possible = nr_cpu_ids;
> +
> +       pr_info("SMP: Allowing %d CPUs, %d hotplug CPUs\n",
> +               possible, max((possible - available_cpus), 0));
> +
> +       for (i = 0; i < possible; i++)
> +               set_cpu_possible(i, true);
> +       for (; i < NR_CPUS; i++)
> +               set_cpu_possible(i, false);
> +}

This does not seem ACPI or ARM specific either.

Rob

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Mark Brown]

On Wed, Dec 04, 2013 at 09:47:49AM -0600, Rob Herring wrote:
> On Tue, Dec 3, 2013 at 10:39 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote:

> > +#ifndef CONFIG_ACPI
> >                 set_cpu_present(cpu, true);
> > +#endif

> Should this be moved to DT cpu topology related code?

Part of the trick there being that there isn't any CPU topology code for
ARMv8 in mainline yet, DT or otherwise - I'm working with some patches
for doing that with DT which I'm hoping to get out soon.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131205/709c050f/attachment.sig>

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Hanjun Guo]

On 2013?12?04? 23:47, Rob Herring wrote:
> On Tue, Dec 3, 2013 at 10:39 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>> When boot the kernel with MADT, the cpu possible and present maps should be
>> prefilled for cpu topology and acpi based cpu hot-plug.
>>
>> The logic cpu id maps to APIC id (GIC id) is also implemented, it is needed
>> for acpi processor drivers.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>   arch/arm64/include/asm/acpi.h |   10 ++--
>>   arch/arm64/kernel/setup.c     |    2 +
>>   arch/arm64/kernel/smp.c       |    2 +
>>   drivers/acpi/plat/arm-core.c  |  118 +++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 129 insertions(+), 3 deletions(-)
> [snip]
>
>> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>> index a0c2ca6..1428024 100644
>> --- a/arch/arm64/kernel/smp.c
>> +++ b/arch/arm64/kernel/smp.c
>> @@ -420,7 +420,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>>                  if (err)
>>                          continue;
>>
>> +#ifndef CONFIG_ACPI
>>                  set_cpu_present(cpu, true);
>> +#endif
> Should this be moved to DT cpu topology related code?

I didn't see the code in the mainline, Mark Brown is
working on it now?

>>                  max_cpus--;
>>          }
>>   }
>> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
>> index 45ff625..8527ecc 100644
>> --- a/drivers/acpi/plat/arm-core.c
>> +++ b/drivers/acpi/plat/arm-core.c
> [snip]
>
>> @@ -221,6 +284,61 @@ static int __init acpi_parse_madt_gic_distributor_entries(void)
>>          return 0;
>>   }
>>
>> +static int setup_possible_cpus __initdata = -1;
>> +static int __init _setup_possible_cpus(char *str)
>> +{
>> +       get_option(&str, &setup_possible_cpus);
>> +       return 0;
>> +}
>> +early_param("possible_cpus", _setup_possible_cpus);
> This does not seem ACPI or ARM specific.
>
>> +
>> +/*
>> + * cpu_possible_mask should be static, it cannot change as cpu's
>> + * are onlined, or offlined. The reason is per-cpu data-structures
>> + * are allocated by some modules at init time, and dont expect to
>> + * do this dynamically on cpu arrival/departure.
>> + * cpu_present_mask on the other hand can change dynamically.
>> + * In case when cpu_hotplug is not compiled, then we resort to current
>> + * behaviour, which is cpu_possible == cpu_present.
>> + * - Ashok Raj
>> + *
>> + * Three ways to find out the number of additional hotplug CPUs:
>> + * - If the BIOS specified disabled CPUs in ACPI/mptables use that.
>> + * - The user can overwrite it with possible_cpus=NUM
>> + * - Otherwise don't reserve additional CPUs.
>> + * We do this because additional CPUs waste a lot of memory.
>> + * -AK
>> + */
>> +void __init prefill_possible_map(void)
>> +{
>> +       int i;
>> +       int possible, disabled_cpus;
>> +
>> +       disabled_cpus = total_cpus - available_cpus;
>> +
>> +       if (setup_possible_cpus == -1) {
>> +               if (disabled_cpus > 0)
>> +                       setup_possible_cpus = disabled_cpus;
>> +               else
>> +                       setup_possible_cpus = 0;
>> +       }
>> +
>> +       possible = available_cpus + setup_possible_cpus;
>> +
>> +       pr_info("SMP: the system is limited to %d CPUs\n", nr_cpu_ids);
>> +
>> +       if (possible > nr_cpu_ids)
>> +               possible = nr_cpu_ids;
>> +
>> +       pr_info("SMP: Allowing %d CPUs, %d hotplug CPUs\n",
>> +               possible, max((possible - available_cpus), 0));
>> +
>> +       for (i = 0; i < possible; i++)
>> +               set_cpu_possible(i, true);
>> +       for (; i < NR_CPUS; i++)
>> +               set_cpu_possible(i, false);
>> +}
> This does not seem ACPI or ARM specific either.

I think possible map here is related to ACPI based CPU hot-plug,
that's why I introduce the code here, if anything I'm wrong, please
correct me.

Thanks
Hanjun

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Arnd Bergmann]

On Wednesday 04 December 2013, Rob Herring wrote:
> > index a0c2ca6..1428024 100644
> > --- a/arch/arm64/kernel/smp.c
> > +++ b/arch/arm64/kernel/smp.c
> > @@ -420,7 +420,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> >                 if (err)
> >                         continue;
> >
> > +#ifndef CONFIG_ACPI
> >                 set_cpu_present(cpu, true);
> > +#endif
> 
> Should this be moved to DT cpu topology related code?

More importantly, the #ifndef is certainly wrong here: It is important that you can
turn CONFIG_ACPI on or off without impacting the run-time code path for non-ACPI
systems. The snippet above breaks this because we no longer set the
cpu mask when ACPI is turned on but not used.

	Arnd

[RFC part2 PATCH 2/9] ARM64 / ACPI: Prefill cpu possible/present maps and map logical cpu id to APIC id

[Hanjun Guo]

On 2013-12-6 7:09, Arnd Bergmann wrote:
> On Wednesday 04 December 2013, Rob Herring wrote:
>>> index a0c2ca6..1428024 100644
>>> --- a/arch/arm64/kernel/smp.c
>>> +++ b/arch/arm64/kernel/smp.c
>>> @@ -420,7 +420,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>>>                 if (err)
>>>                         continue;
>>>
>>> +#ifndef CONFIG_ACPI
>>>                 set_cpu_present(cpu, true);
>>> +#endif
>>
>> Should this be moved to DT cpu topology related code?
> 
> More importantly, the #ifndef is certainly wrong here: It is important that you can
> turn CONFIG_ACPI on or off without impacting the run-time code path for non-ACPI
> systems. The snippet above breaks this because we no longer set the
> cpu mask when ACPI is turned on but not used.

Good point, I'll rework this patch to find a better solution.

Thanks for your comments.

Hanjun