lkml.org 
[lkml]   [2014]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 06/18] ARM64 / ACPI: Parse MADT to map logical cpu to MPIDR and get cpu_possible/present_map


On 04/08/14 16:28, 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 and
> cpu_present_map.
>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
> arch/arm64/include/asm/acpi.h | 2 +
> arch/arm64/kernel/acpi.c | 129 ++++++++++++++++++++++++++++++++++++++++-
> arch/arm64/kernel/smp.c | 10 +++-
> 3 files changed, 138 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 6e04868..e877967 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -64,6 +64,8 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> extern int (*acpi_suspend_lowlevel)(void);
> #define acpi_wakeup_address 0
>
> +#define MAX_GIC_CPU_INTERFACE 65535
> +

Did you get this information from GIC specification ?

IIUC you are trying to represent max number of interrupt controller
entries MADT can possibly have. So, I had suggested to change the name
like MAX_MADT_INTERRUPT_CONTROLLER_ENTRIES so that it is not GIC specific.

> #endif /* CONFIG_ACPI */
>
> #endif /*_ASM_ACPI_H*/
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 69a315d..9e07d99 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -22,6 +22,9 @@
> #include <linux/bootmem.h>
> #include <linux/smp.h>
>
> +#include <asm/smp_plat.h>
> +#include <asm/cputype.h>
> +
> int acpi_noirq; /* skip ACPI IRQ initialization */
> int acpi_disabled;
> EXPORT_SYMBOL(acpi_disabled);
> @@ -29,6 +32,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.
> @@ -49,6 +54,122 @@ void __init __acpi_unmap_table(char *map, unsigned long size)
> early_memunmap(map, size);
> }
>
> +/**
> + * acpi_register_gic_cpu_interface - register a gic cpu interface and
> + * generates a logical cpu number
> + * @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 the gic cpu interface
> + */
> +static int acpi_register_gic_cpu_interface(u64 mpidr, u8 enabled)
> +{

IMO the function name gives me a wrong idea that you are registering
something with GIC. How about acpi_map_gic_cpu_interface ?

> + int cpu;
> +
> + if (mpidr == INVALID_HWID) {
> + pr_info("Skip invalid cpu hardware ID\n");
> + 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;
> + }
> +
> + /* If it is the first CPU, no need to check duplicate MPIDRs */
> + if (!enabled_cpus)
> + goto skip_mpidr_check;
> +
> + /*
> + * Duplicate MPIDRs are a recipe for disaster. Scan
> + * all initialized entries and check for
> + * duplicates. If any is found just ignore the CPU.
> + */
> + for_each_present_cpu(cpu) {
> + if (cpu_logical_map(cpu) == mpidr) {
> + pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
> + mpidr);
> + return -EINVAL;
> + }
> + }
> +
> +skip_mpidr_check:
> + enabled_cpus++;
> +
> + /* allocate a logical cpu id for the new comer */
> + if (cpu_logical_map(0) == mpidr) {
> + /*
> + * boot_cpu_init() already hold bit 0 in cpu_present_mask
> + * for BSP, no need to allocate again.
> + */
> + cpu = 0;
> + } else {
> + cpu = cpumask_next_zero(-1, cpu_present_mask);
> + }
> +
> + /* map the logical cpu id to cpu MPIDR */
> + cpu_logical_map(cpu) = mpidr;
> +
> + set_cpu_possible(cpu, true);

IMO it better to keep all these updates to cpumasks contained in the
smp.c as I had mentioned before. I think you can refactor smp_init_cpus
to achieve that.

> + set_cpu_present(cpu, true);

This is totally wrong ? What would you do if the cpu failed to
initialize ? I don't see that handled.

> +
> + 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_register_gic_cpu_interface(processor->arm_mpidr,
> + processor->flags & ACPI_MADT_ENABLED);
> +
> + return 0;
> +}
> +
> +/*
> + * Parse GIC cpu interface related entries in MADT
> + * returns 0 on success, < 0 on error
> + */
> +static int __init acpi_parse_madt_gic_cpu_interface_entries(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, MAX_GIC_CPU_INTERFACE);
> +
> + if (!count) {
> + pr_err("No GIC CPU interface entries present\n");
> + return -ENODEV;
> + } else if (count < 0) {
> + pr_err("Error parsing GIC CPU interface entry\n");
> + return count;
> + }
> +
> + /* Make boot-up look pretty */
> + pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus);

Ideally just setup cpu_logical_map in acpi_parse_gic_cpu_interface and
setup cpumasks in smp_init_cpus.

> +
> + return 0;
> +}
> +
> static int __init acpi_parse_fadt(struct acpi_table_header *table)
> {
> struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
> @@ -99,8 +220,12 @@ int __init acpi_boot_init(void)
> return -ENODEV;
>
> err = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
> - if (err)
> - pr_err("Can't find FADT\n");
> + if (err) {
> + pr_err("Can't find FADT or error happened during parsing FADT\n");
> + return err;
> + }
> +
> + err = acpi_parse_madt_gic_cpu_interface_entries();
>
> return err;
> }
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 40f38f4..8f1d37c 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -36,6 +36,7 @@
> #include <linux/completion.h>
> #include <linux/of.h>
> #include <linux/irq_work.h>
> +#include <linux/acpi.h>
>
> #include <asm/atomic.h>
> #include <asm/cacheflush.h>
> @@ -458,7 +459,14 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> if (err)
> continue;
>
> - set_cpu_present(cpu, true);
> + /*
> + * In ACPI mode, cpu_present_map was initialised when
> + * MADT table was parsed which before this function
> + * is called.
> + */
> + if (acpi_disabled)
> + set_cpu_present(cpu, true);
> +

This is the right place to set the cpu present based on the return value
of cpu_prepare.

> max_cpus--;
> }
> }
>



\
 
 \ /
  Last update: 2014-08-18 21:01    [W:0.246 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site