| Message ID | 1407166105-17675-7-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| 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--; > } > } >
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
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
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.
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
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
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--; } }