| Message ID | 1407166105-17675-7-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Aug 04, 2014 at 04:28:13PM +0100, Hanjun Guo wrote: > 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 Does this need to be more than NR_CPUS? > +/** > + * 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) > +{ > + 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); > + set_cpu_present(cpu, true); > + > + return cpu; > +} Why not only set the cpu possible here (with the additional mpidr validity checks changed to for_each_possible_cpu)... > 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); > + > max_cpus--; ... and don't touch this at all.
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--; > } > } > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014-8-18 22:27, Catalin Marinas wrote: > On Mon, Aug 04, 2014 at 04:28:13PM +0100, Hanjun Guo wrote: >> 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 > > Does this need to be more than NR_CPUS? Sometimes yes, CPU structure entries in MADT just like CPU nodes in device tree, the number of them may more than NR_CPUS. > >> +/** >> + * 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) >> +{ >> + 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); >> + set_cpu_present(cpu, true); >> + >> + return cpu; >> +} > > Why not only set the cpu possible here (with the additional mpidr > validity checks changed to for_each_possible_cpu)... > >> 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); >> + >> max_cpus--; > > ... and don't touch this at all. the original intent was to support CPU hotplug, I will update the patch as you suggested and introduce the change when CPU hotplug patch is upstreamed, then it will make the patch less confusing. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hi Sudeep, On 2014-8-19 2:33, Sudeep Holla wrote: > 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 ? Actually not. since MAX_GIC_CPU_INTERFACE represents the maximum GIC CPU interfaces (CPUs) entries in MADT, so I use a number big enough to represent max CPUs in the system. > > 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. Will INTERRUPT_CONTROLLER confuse people? There is only one GIC redistributor (some people regard it as interrupt controller) in ARM system, if we use INTERRUPT_CONTROLLER people will regard it as multi-redistributors in the system, I think GIC_CPU_INTERFACE would be better, what do you think? > >> #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 ? Great, acpi_map_gic_cpu_interface is better. > >> + 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. Could you give me more hints? smp_init_cpus() use the CPU node in device tree to map cpu, but in ACPI, they are table entries, if you can give me more hints about how to refactor it, I will appreciate a lot. > >> + 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. Yes, as I replied in previous email, I will update the patch and consider CPU hotplug in the future. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hanjun, On Tue, Aug 19, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > Will INTERRUPT_CONTROLLER confuse people? There is only one GIC redistributor > (some people regard it as interrupt controller) in ARM system, if we use > INTERRUPT_CONTROLLER people will regard it as multi-redistributors in the > system, I think GIC_CPU_INTERFACE would be better, what do you think? I think you meant to say "one GIC _distributor_" (GICD) instead of redistributor (GICR). ACPI5.1 only describes one GICD per system, but we can most certainly describe multiple GICRs. For multi-cpu systems, you'll definitely see multiple GIC redistributors. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014-8-20 0:46, Zi Shen Lim wrote: > Hanjun, Hi Lim, > > On Tue, Aug 19, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >> Will INTERRUPT_CONTROLLER confuse people? There is only one GIC redistributor >> (some people regard it as interrupt controller) in ARM system, if we use >> INTERRUPT_CONTROLLER people will regard it as multi-redistributors in the >> system, I think GIC_CPU_INTERFACE would be better, what do you think? > > I think you meant to say "one GIC _distributor_" (GICD) instead of > redistributor (GICR). > ACPI5.1 only describes one GICD per system, but we can most certainly > describe multiple GICRs. > For multi-cpu systems, you'll definitely see multiple GIC redistributors. Yes, you are right, thanks for pointing it out, I think I need more coffee :) Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Aug 19, 2014 at 08:36:46AM +0100, Hanjun Guo wrote: > On 2014-8-18 22:27, Catalin Marinas wrote: > > On Mon, Aug 04, 2014 at 04:28:13PM +0100, Hanjun Guo wrote: > >> 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 > > > > Does this need to be more than NR_CPUS? > > Sometimes yes, CPU structure entries in MADT just like CPU nodes in > device tree, the number of them may more than NR_CPUS. I have a more general question here. In ACPI, is MADT the only way to build a CPU topology? It looks weird that we use GIC properties to create the cpu_logical_map(). A side-effect is that the GIC-related functions are now scattered all over the kernel rather than being contained in the GIC driver itself.
On 2014-8-20 22:38, Catalin Marinas wrote: > On Tue, Aug 19, 2014 at 08:36:46AM +0100, Hanjun Guo wrote: >> On 2014-8-18 22:27, Catalin Marinas wrote: >>> On Mon, Aug 04, 2014 at 04:28:13PM +0100, Hanjun Guo wrote: >>>> 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 >>> >>> Does this need to be more than NR_CPUS? >> >> Sometimes yes, CPU structure entries in MADT just like CPU nodes in >> device tree, the number of them may more than NR_CPUS. > > I have a more general question here. In ACPI, is MADT the only way to > build a CPU topology? Unfortunately yes as far as I can tell. > It looks weird that we use GIC properties to > create the cpu_logical_map(). Actually information in GICC structures in MADT will both used for GIC init and SMP init, GICC structures represents CPUs in the system. > A side-effect is that the GIC-related > functions are now scattered all over the kernel rather than being > contained in the GIC driver itself. As patch 12/18 shows, all the GIC related code all contained in the GIC driver, GICC structure is more than GIC-related but also CPUs in the system (I have to admit that the name of GICC in ACPI is confusing). Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 + #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) +{ + 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); + set_cpu_present(cpu, true); + + 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); + + 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); + max_cpus--; } }