| Message ID | 1421247905-3749-11-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/14/2015 09:04 AM, Hanjun Guo wrote: > MADT contains the information for MPIDR which is essential for > SMP initialization, parse the GIC cpu interface structures to > get the MPIDR value and map it to cpu_logical_map(), and add > enabled cpu with valid MPIDR into cpu_possible_map. > > ACPI 5.1 only has two explicit methods to boot up SMP, PSCI and > Parking protocol, but the Parking protocol is only specified for > ARMv7 now, so make PSCI as the only way for the SMP boot protocol > before some updates for the ACPI spec or the Parking protocol spec. > > Parking protocol patches for SMP boot will be sent to upstream when > the new version of Parking protocol is ready. > > Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Tested-by: Yijing Wang <wangyijing@huawei.com> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > --- Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
Hi Hanjun, On Wed, Jan 14, 2015 at 03:04:58PM +0000, Hanjun Guo wrote: [...] > diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h > index 780f82c..bf22650 100644 > --- a/arch/arm64/include/asm/smp.h > +++ b/arch/arm64/include/asm/smp.h > @@ -39,9 +39,10 @@ extern void show_ipi_list(struct seq_file *p, int prec); > extern void handle_IPI(int ipinr, struct pt_regs *regs); > > /* > - * Setup the set of possible CPUs (via set_cpu_possible) > + * Discover the set of possible CPUs and determine their > + * SMP operations. > */ > -extern void smp_init_cpus(void); > +extern void of_smp_init_cpus(void); > > /* > * Provide a function to raise an IPI cross call on CPUs in callmap. > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index 4177758..55d99d9 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -24,6 +24,10 @@ > #include <linux/bootmem.h> > #include <linux/smp.h> > > +#include <asm/smp_plat.h> > +#include <asm/cputype.h> > +#include <asm/cpu_ops.h> Alphabetical order please. > + > int acpi_noirq; /* skip ACPI IRQ initialization */ > int acpi_disabled; > EXPORT_SYMBOL(acpi_disabled); > @@ -31,6 +35,8 @@ EXPORT_SYMBOL(acpi_disabled); > int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */ > EXPORT_SYMBOL(acpi_pci_disabled); > > +static int enabled_cpus; /* Processors (GICC) with enabled flag in MADT */ Needed for hotplug ? Remember that each patch must be self-contained. This patch is clearly not, if I did not know you need to carry this stuff over after boot for physical cpu hotplugging it would be hard to understand most of this code. Think about that for anyone looking at your commit-log as a stand alone entity in the kernel history. > + > /* > * __acpi_map_table() will be called before page_init(), so early_ioremap() > * or early_memremap() should be called here to for ACPI table mapping. > @@ -51,6 +57,134 @@ void __init __acpi_unmap_table(char *map, unsigned long size) > early_memunmap(map, size); > } > > +/** > + * acpi_map_gic_cpu_interface - generates a logical cpu number > + * and map to MPIDR represented by GICC structure > + * @mpidr: CPU's hardware id to register, MPIDR represented in MADT > + * @enabled: this cpu is enabled or not > + * > + * Returns the logical cpu number which maps to MPIDR > + */ > +static int acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) It would be __init, except that you might need it after boot for hotplug, correct ? > +{ > + int cpu; > + > + if (mpidr == INVALID_HWID) { > + pr_info("Skip MADT cpu entry with invalid MPIDR\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; > + } > + > + /* No need to check duplicate MPIDRs for the first CPU */ > + if (enabled_cpus) { > + /* > + * Duplicate MPIDRs are a recipe for disaster. Scan > + * all initialized entries and check for > + * duplicates. If any is found just ignore the CPU. > + */ > + for_each_possible_cpu(cpu) { > + if (cpu_logical_map(cpu) == mpidr) { > + pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", > + mpidr); > + return -EINVAL; > + } > + } > + > + /* allocate a logical cpu id for the new comer */ > + cpu = cpumask_next_zero(-1, cpu_possible_mask); > + } else { > + /* > + * First GICC entry must be BSP as ACPI spec said > + * in section 5.2.12.15 > + */ > + if (cpu_logical_map(0) != mpidr) { > + pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n", > + mpidr); > + return -EINVAL; > + } > + > + /* > + * boot_cpu_init() already hold bit 0 in cpu_present_mask You mean cpu_possible_mask ? That's what you allocate from above. > + * for BSP, no need to allocate again. > + */ > + cpu = 0; > + } > + > + /* CPU 0 was already initialized */ > + if (cpu) { > + cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); > + if (!cpu_ops[cpu]) > + return -EINVAL; > + > + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) > + return -EOPNOTSUPP; > + > + /* map the logical cpu id to cpu MPIDR */ > + cpu_logical_map(cpu) = mpidr; > + > + set_cpu_possible(cpu, true); > + } else { > + /* get cpu0's ops, no need to return if ops is null */ > + cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); > + } I do not see much point in calling cpu_get_ops with NULL, and adding the check in it to return NULL when the parameter is NULL. What would you expect from cpu_get_ops when called with NULL other than a NULL pointer ? You could move: cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); out of the if and remove the else, do not know if it makes code clearer, shorter for certain. > + > + enabled_cpus++; > + return cpu; > +} > + > +static int __init > +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct acpi_madt_generic_interrupt *processor; > + > + processor = (struct acpi_madt_generic_interrupt *)header; > + > + if (BAD_MADT_ENTRY(processor, end)) > + return -EINVAL; > + > + acpi_table_print_madt_entry(header); > + > + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, > + processor->flags & ACPI_MADT_ENABLED); > + > + return 0; > +} > + > +/* Parse GIC cpu interface entries in MADT for SMP init */ > +void __init acpi_smp_init_cpus(void) > +{ > + int count; > + > + /* > + * do a partial walk of MADT to determine how many CPUs > + * we have including disabled CPUs, and get information > + * we need for SMP init > + */ > + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, > + acpi_parse_gic_cpu_interface, 0); > + > + if (!count) { > + pr_err("No GIC CPU interface entries present\n"); > + return; > + } else if (count < 0) { > + pr_err("Error parsing GIC CPU interface entry\n"); > + return; > + } > + > + /* Make boot-up look pretty */ > + pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus); > +} > + > static int __init acpi_parse_fadt(struct acpi_table_header *table) > { > struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; > @@ -62,8 +196,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) > * to get arm boot flags, or we will disable ACPI. > */ > if (table->revision > 5 || > - (table->revision == 5 && fadt->minor_revision >= 1)) > - return 0; > + (table->revision == 5 && fadt->minor_revision >= 1)) { > + /* > + * ACPI 5.1 only has two explicit methods to boot up SMP, > + * PSCI and Parking protocol, but the Parking protocol is > + * only specified for ARMv7 now, so make PSCI as the only > + * way for the SMP boot protocol before some updates for > + * the ACPI spec or the Parking protocol spec. > + */ > + if (acpi_psci_present()) > + return 0; > + > + pr_warn("No PSCI support, will not bring up secondary CPUs\n"); > + return -EOPNOTSUPP; > + } > > pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n", > table->revision, fadt->minor_revision); > diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c > index cce9524..1ea7b9f 100644 > --- a/arch/arm64/kernel/cpu_ops.c > +++ b/arch/arm64/kernel/cpu_ops.c > @@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops; > > const struct cpu_operations *cpu_ops[NR_CPUS]; > > -static const struct cpu_operations *supported_cpu_ops[] __initconst = { > +static const struct cpu_operations *supported_cpu_ops[] = { This __initconst removal should be explained either with code needing it or through a comment. You can't make changes with future patches in mind, since they may never get merged and you leave code in this patch incomplete. As far as I know if physical CPU hotplug can't/won't be done on ARM64 your patch would make changes that are not needed, and miss some changes that are (eg removing enabled_cpus or make it __initdata). You can't write a patch with assumptions on subsequent patches. > #ifdef CONFIG_SMP > &smp_spin_table_ops, > #endif > @@ -35,10 +35,13 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = { > NULL, > }; > > -static const struct cpu_operations * __init cpu_get_ops(const char *name) > +const struct cpu_operations *cpu_get_ops(const char *name) Ditto. > { > const struct cpu_operations **ops = supported_cpu_ops; > > + if (!name) > + return NULL; > + See above. > while (*ops) { > if (!strcmp(name, (*ops)->name)) > return *ops; > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index ef5b1e1..54e39e3 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -414,13 +414,16 @@ void __init setup_arch(char **cmdline_p) > if (acpi_disabled) { > unflatten_device_tree(); > psci_dt_init(); > + cpu_read_bootcpu_ops(); > +#ifdef CONFIG_SMP > + of_smp_init_cpus(); > +#endif > } else { > psci_acpi_init(); > + acpi_smp_init_cpus(); With DT you call cpu_read_bootcpu_ops() and then of_smp_init_cpus() with acpi you have one function that does both, it is not really neat. Thanks, Lorenzo > } > > - cpu_read_bootcpu_ops(); > #ifdef CONFIG_SMP > - smp_init_cpus(); > smp_build_mpidr_hash(); > #endif > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index 7ae6ee0..5aaf5a4 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -323,7 +323,7 @@ void __init smp_prepare_boot_cpu(void) > * cpu logical map array containing MPIDR values related to logical > * cpus. Assumes that cpu_logical_map(0) has already been initialized. > */ > -void __init smp_init_cpus(void) > +void __init of_smp_init_cpus(void) > { > struct device_node *dn = NULL; > unsigned int i, cpu = 1; > -- > 1.9.1 > >
Hi Lorenzo, Sorry for the late reply, I was updating this patch according to your comments and see if it works, inline reply below. On 2015?01?17? 02:18, Lorenzo Pieralisi wrote: > Hi Hanjun, > > On Wed, Jan 14, 2015 at 03:04:58PM +0000, Hanjun Guo wrote: > > [...] > >> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h >> index 780f82c..bf22650 100644 >> --- a/arch/arm64/include/asm/smp.h >> +++ b/arch/arm64/include/asm/smp.h >> @@ -39,9 +39,10 @@ extern void show_ipi_list(struct seq_file *p, int prec); >> extern void handle_IPI(int ipinr, struct pt_regs *regs); >> >> /* >> - * Setup the set of possible CPUs (via set_cpu_possible) >> + * Discover the set of possible CPUs and determine their >> + * SMP operations. >> */ >> -extern void smp_init_cpus(void); >> +extern void of_smp_init_cpus(void); >> >> /* >> * Provide a function to raise an IPI cross call on CPUs in callmap. >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >> index 4177758..55d99d9 100644 >> --- a/arch/arm64/kernel/acpi.c >> +++ b/arch/arm64/kernel/acpi.c >> @@ -24,6 +24,10 @@ >> #include <linux/bootmem.h> >> #include <linux/smp.h> >> >> +#include <asm/smp_plat.h> >> +#include <asm/cputype.h> >> +#include <asm/cpu_ops.h> > > Alphabetical order please. OK, already updated. > >> + >> int acpi_noirq; /* skip ACPI IRQ initialization */ >> int acpi_disabled; >> EXPORT_SYMBOL(acpi_disabled); >> @@ -31,6 +35,8 @@ EXPORT_SYMBOL(acpi_disabled); >> int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */ >> EXPORT_SYMBOL(acpi_pci_disabled); >> >> +static int enabled_cpus; /* Processors (GICC) with enabled flag in MADT */ > > Needed for hotplug ? Remember that each patch must be self-contained. > > This patch is clearly not, if I did not know you need to carry this > stuff over after boot for physical cpu hotplugging it would be hard to > understand most of this code. > > Think about that for anyone looking at your commit-log as a stand alone > entity in the kernel history. I will say partly yes :) Processor (GICC) structures in MADT have a flag to indicate it is enabled or not, and yes, it can be used as the base support for CPU hotplug to generate possible cpus with disabled CPU counted (no implementation in this patch) in, and then we can hot add CPUs which disabled in MADT table. But we still got other use cases, such as we just enable cores we wanted (for example, we can boot 16 cores in a 32 cores system, with disabled flag for another 16 cores), and update the flag in firmware without adding/deleting GICC entries, enabled_cpus will count CPUs with enabled flag and I think this information it is useful to know. > >> + >> /* >> * __acpi_map_table() will be called before page_init(), so early_ioremap() >> * or early_memremap() should be called here to for ACPI table mapping. >> @@ -51,6 +57,134 @@ void __init __acpi_unmap_table(char *map, unsigned long size) >> early_memunmap(map, size); >> } >> >> +/** >> + * acpi_map_gic_cpu_interface - generates a logical cpu number >> + * and map to MPIDR represented by GICC structure >> + * @mpidr: CPU's hardware id to register, MPIDR represented in MADT >> + * @enabled: this cpu is enabled or not >> + * >> + * Returns the logical cpu number which maps to MPIDR >> + */ >> +static int acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) > > It would be __init, except that you might need it after boot for > hotplug, correct ? Yes, good catch :) I agree with you here totally that I shouldn't add hot-plug code in this patch, I will update it. > >> +{ >> + int cpu; >> + >> + if (mpidr == INVALID_HWID) { >> + pr_info("Skip MADT cpu entry with invalid MPIDR\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; >> + } >> + >> + /* No need to check duplicate MPIDRs for the first CPU */ >> + if (enabled_cpus) { >> + /* >> + * Duplicate MPIDRs are a recipe for disaster. Scan >> + * all initialized entries and check for >> + * duplicates. If any is found just ignore the CPU. >> + */ >> + for_each_possible_cpu(cpu) { >> + if (cpu_logical_map(cpu) == mpidr) { >> + pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", >> + mpidr); >> + return -EINVAL; >> + } >> + } >> + >> + /* allocate a logical cpu id for the new comer */ >> + cpu = cpumask_next_zero(-1, cpu_possible_mask); >> + } else { >> + /* >> + * First GICC entry must be BSP as ACPI spec said >> + * in section 5.2.12.15 >> + */ >> + if (cpu_logical_map(0) != mpidr) { >> + pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n", >> + mpidr); >> + return -EINVAL; >> + } >> + >> + /* >> + * boot_cpu_init() already hold bit 0 in cpu_present_mask > > You mean cpu_possible_mask ? That's what you allocate from above. Another hot-plug piece leaved, will update it. > >> + * for BSP, no need to allocate again. >> + */ >> + cpu = 0; >> + } >> + >> + /* CPU 0 was already initialized */ >> + if (cpu) { >> + cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); >> + if (!cpu_ops[cpu]) >> + return -EINVAL; >> + >> + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) >> + return -EOPNOTSUPP; >> + >> + /* map the logical cpu id to cpu MPIDR */ >> + cpu_logical_map(cpu) = mpidr; >> + >> + set_cpu_possible(cpu, true); >> + } else { >> + /* get cpu0's ops, no need to return if ops is null */ >> + cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); >> + } > > I do not see much point in calling cpu_get_ops with NULL, and adding > the check in it to return NULL when the parameter is NULL. > > What would you expect from cpu_get_ops when called with NULL other than > a NULL pointer ? I'm lost here since it is best way for the implementation I think, any suggestions? > > You could move: > > cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); > > out of the if and remove the else, do not know if it makes code clearer, > shorter for certain. > >> + >> + enabled_cpus++; >> + return cpu; >> +} >> + >> +static int __init >> +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct acpi_madt_generic_interrupt *processor; >> + >> + processor = (struct acpi_madt_generic_interrupt *)header; >> + >> + if (BAD_MADT_ENTRY(processor, end)) >> + return -EINVAL; >> + >> + acpi_table_print_madt_entry(header); >> + >> + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, >> + processor->flags & ACPI_MADT_ENABLED); >> + >> + return 0; >> +} >> + >> +/* Parse GIC cpu interface entries in MADT for SMP init */ >> +void __init acpi_smp_init_cpus(void) >> +{ >> + int count; >> + >> + /* >> + * do a partial walk of MADT to determine how many CPUs >> + * we have including disabled CPUs, and get information >> + * we need for SMP init >> + */ >> + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, >> + acpi_parse_gic_cpu_interface, 0); >> + >> + if (!count) { >> + pr_err("No GIC CPU interface entries present\n"); >> + return; >> + } else if (count < 0) { >> + pr_err("Error parsing GIC CPU interface entry\n"); >> + return; >> + } >> + >> + /* Make boot-up look pretty */ >> + pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus); >> +} >> + >> static int __init acpi_parse_fadt(struct acpi_table_header *table) >> { >> struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; >> @@ -62,8 +196,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) >> * to get arm boot flags, or we will disable ACPI. >> */ >> if (table->revision > 5 || >> - (table->revision == 5 && fadt->minor_revision >= 1)) >> - return 0; >> + (table->revision == 5 && fadt->minor_revision >= 1)) { >> + /* >> + * ACPI 5.1 only has two explicit methods to boot up SMP, >> + * PSCI and Parking protocol, but the Parking protocol is >> + * only specified for ARMv7 now, so make PSCI as the only >> + * way for the SMP boot protocol before some updates for >> + * the ACPI spec or the Parking protocol spec. >> + */ >> + if (acpi_psci_present()) >> + return 0; >> + >> + pr_warn("No PSCI support, will not bring up secondary CPUs\n"); >> + return -EOPNOTSUPP; >> + } >> >> pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n", >> table->revision, fadt->minor_revision); >> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c >> index cce9524..1ea7b9f 100644 >> --- a/arch/arm64/kernel/cpu_ops.c >> +++ b/arch/arm64/kernel/cpu_ops.c >> @@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops; >> >> const struct cpu_operations *cpu_ops[NR_CPUS]; >> >> -static const struct cpu_operations *supported_cpu_ops[] __initconst = { >> +static const struct cpu_operations *supported_cpu_ops[] = { > > This __initconst removal should be explained either with code needing > it or through a comment. You can't make changes with future patches > in mind, since they may never get merged and you leave code in this > patch incomplete. > > As far as I know if physical CPU hotplug can't/won't be done on ARM64 your > patch would make changes that are not needed, and miss some changes > that are (eg removing enabled_cpus or make it __initdata). I agree with you :) > > You can't write a patch with assumptions on subsequent patches. > >> #ifdef CONFIG_SMP >> &smp_spin_table_ops, >> #endif >> @@ -35,10 +35,13 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = { >> NULL, >> }; >> >> -static const struct cpu_operations * __init cpu_get_ops(const char *name) >> +const struct cpu_operations *cpu_get_ops(const char *name) > > Ditto. > >> { >> const struct cpu_operations **ops = supported_cpu_ops; >> >> + if (!name) >> + return NULL; >> + > > See above. > >> while (*ops) { >> if (!strcmp(name, (*ops)->name)) >> return *ops; >> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c >> index ef5b1e1..54e39e3 100644 >> --- a/arch/arm64/kernel/setup.c >> +++ b/arch/arm64/kernel/setup.c >> @@ -414,13 +414,16 @@ void __init setup_arch(char **cmdline_p) >> if (acpi_disabled) { >> unflatten_device_tree(); >> psci_dt_init(); >> + cpu_read_bootcpu_ops(); >> +#ifdef CONFIG_SMP >> + of_smp_init_cpus(); >> +#endif >> } else { >> psci_acpi_init(); >> + acpi_smp_init_cpus(); > > With DT you call cpu_read_bootcpu_ops() and then of_smp_init_cpus() > with acpi you have one function that does both, it is not really > neat. The mechanism for ACPI table entry scanning is that for every matched structure (such as GICC) found, the parse function will be called, so if we separate them it will duplicate the scanning of ACPI tables. Thanks Hanjun
On Tue, Jan 20, 2015 at 01:09:55PM +0000, Hanjun Guo wrote: [...] > >> +{ > >> + int cpu; > >> + > >> + if (mpidr == INVALID_HWID) { > >> + pr_info("Skip MADT cpu entry with invalid MPIDR\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; > >> + } > >> + > >> + /* No need to check duplicate MPIDRs for the first CPU */ > >> + if (enabled_cpus) { > >> + /* > >> + * Duplicate MPIDRs are a recipe for disaster. Scan > >> + * all initialized entries and check for > >> + * duplicates. If any is found just ignore the CPU. > >> + */ > >> + for_each_possible_cpu(cpu) { > >> + if (cpu_logical_map(cpu) == mpidr) { > >> + pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", > >> + mpidr); > >> + return -EINVAL; > >> + } > >> + } > >> + > >> + /* allocate a logical cpu id for the new comer */ > >> + cpu = cpumask_next_zero(-1, cpu_possible_mask); > >> + } else { > >> + /* > >> + * First GICC entry must be BSP as ACPI spec said > >> + * in section 5.2.12.15 > >> + */ > >> + if (cpu_logical_map(0) != mpidr) { > >> + pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n", > >> + mpidr); > >> + return -EINVAL; > >> + } > >> + > >> + /* > >> + * boot_cpu_init() already hold bit 0 in cpu_present_mask > > > > You mean cpu_possible_mask ? That's what you allocate from above. > > Another hot-plug piece leaved, will update it. > > > > >> + * for BSP, no need to allocate again. > >> + */ > >> + cpu = 0; > >> + } > >> + > >> + /* CPU 0 was already initialized */ > >> + if (cpu) { > >> + cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); > >> + if (!cpu_ops[cpu]) > >> + return -EINVAL; > >> + > >> + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) > >> + return -EOPNOTSUPP; > >> + > >> + /* map the logical cpu id to cpu MPIDR */ > >> + cpu_logical_map(cpu) = mpidr; > >> + > >> + set_cpu_possible(cpu, true); > >> + } else { > >> + /* get cpu0's ops, no need to return if ops is null */ > >> + cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); > >> + } > > > > I do not see much point in calling cpu_get_ops with NULL, and adding > > the check in it to return NULL when the parameter is NULL. > > > > What would you expect from cpu_get_ops when called with NULL other than > > a NULL pointer ? > > I'm lost here since it is best way for the implementation I think, any > suggestions? My suggestion is: no PSCI, no secondaries booting, that's what your code wants to achieve, right ? What's the point in calling cpu_get_ops() when PSCI is not present then ? What do you expect from calling cpu_get_ops(NULL) other than a NULL pointer in return ? Put it differently, if !acpi_psci_present() parsing code should bail out, there are no CPU ops to initialize for secondaries, that's what I think your aim is, correct ? On a side note, if ACPI PSCI is not present you still keep booting on the boot processor. What piece of code initialize cpu_ops[0] in that case ? I could not find any, basically you would run the kernel with cpu_ops[0] == NULL. > > > > > You could move: > > > > cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); > > > > out of the if and remove the else, do not know if it makes code clearer, > > shorter for certain. > > > >> + > >> + enabled_cpus++; > >> + return cpu; > >> +} > >> + > >> +static int __init > >> +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, > >> + const unsigned long end) > >> +{ > >> + struct acpi_madt_generic_interrupt *processor; > >> + > >> + processor = (struct acpi_madt_generic_interrupt *)header; > >> + > >> + if (BAD_MADT_ENTRY(processor, end)) > >> + return -EINVAL; > >> + > >> + acpi_table_print_madt_entry(header); > >> + > >> + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, > >> + processor->flags & ACPI_MADT_ENABLED); > >> + > >> + return 0; > >> +} > >> + > >> +/* Parse GIC cpu interface entries in MADT for SMP init */ > >> +void __init acpi_smp_init_cpus(void) > >> +{ > >> + int count; > >> + > >> + /* > >> + * do a partial walk of MADT to determine how many CPUs > >> + * we have including disabled CPUs, and get information > >> + * we need for SMP init > >> + */ > >> + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, > >> + acpi_parse_gic_cpu_interface, 0); > >> + > >> + if (!count) { > >> + pr_err("No GIC CPU interface entries present\n"); > >> + return; > >> + } else if (count < 0) { > >> + pr_err("Error parsing GIC CPU interface entry\n"); > >> + return; > >> + } > >> + > >> + /* Make boot-up look pretty */ > >> + pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus); > >> +} > >> + > >> static int __init acpi_parse_fadt(struct acpi_table_header *table) > >> { > >> struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; > >> @@ -62,8 +196,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) > >> * to get arm boot flags, or we will disable ACPI. > >> */ > >> if (table->revision > 5 || > >> - (table->revision == 5 && fadt->minor_revision >= 1)) > >> - return 0; > >> + (table->revision == 5 && fadt->minor_revision >= 1)) { > >> + /* > >> + * ACPI 5.1 only has two explicit methods to boot up SMP, > >> + * PSCI and Parking protocol, but the Parking protocol is > >> + * only specified for ARMv7 now, so make PSCI as the only > >> + * way for the SMP boot protocol before some updates for > >> + * the ACPI spec or the Parking protocol spec. > >> + */ > >> + if (acpi_psci_present()) > >> + return 0; > >> + > >> + pr_warn("No PSCI support, will not bring up secondary CPUs\n"); > >> + return -EOPNOTSUPP; > >> + } > >> > >> pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n", > >> table->revision, fadt->minor_revision); > >> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c > >> index cce9524..1ea7b9f 100644 > >> --- a/arch/arm64/kernel/cpu_ops.c > >> +++ b/arch/arm64/kernel/cpu_ops.c > >> @@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops; > >> > >> const struct cpu_operations *cpu_ops[NR_CPUS]; > >> > >> -static const struct cpu_operations *supported_cpu_ops[] __initconst = { > >> +static const struct cpu_operations *supported_cpu_ops[] = { > > > > This __initconst removal should be explained either with code needing > > it or through a comment. You can't make changes with future patches > > in mind, since they may never get merged and you leave code in this > > patch incomplete. > > > > As far as I know if physical CPU hotplug can't/won't be done on ARM64 your > > patch would make changes that are not needed, and miss some changes > > that are (eg removing enabled_cpus or make it __initdata). > > I agree with you :) > > > > > You can't write a patch with assumptions on subsequent patches. > > > >> #ifdef CONFIG_SMP > >> &smp_spin_table_ops, > >> #endif > >> @@ -35,10 +35,13 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = { > >> NULL, > >> }; > >> > >> -static const struct cpu_operations * __init cpu_get_ops(const char *name) > >> +const struct cpu_operations *cpu_get_ops(const char *name) > > > > Ditto. > > > >> { > >> const struct cpu_operations **ops = supported_cpu_ops; > >> > >> + if (!name) > >> + return NULL; > >> + > > > > See above. > > > >> while (*ops) { > >> if (!strcmp(name, (*ops)->name)) > >> return *ops; > >> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > >> index ef5b1e1..54e39e3 100644 > >> --- a/arch/arm64/kernel/setup.c > >> +++ b/arch/arm64/kernel/setup.c > >> @@ -414,13 +414,16 @@ void __init setup_arch(char **cmdline_p) > >> if (acpi_disabled) { > >> unflatten_device_tree(); > >> psci_dt_init(); > >> + cpu_read_bootcpu_ops(); > >> +#ifdef CONFIG_SMP > >> + of_smp_init_cpus(); > >> +#endif > >> } else { > >> psci_acpi_init(); > >> + acpi_smp_init_cpus(); > > > > With DT you call cpu_read_bootcpu_ops() and then of_smp_init_cpus() > > with acpi you have one function that does both, it is not really > > neat. > > The mechanism for ACPI table entry scanning is that for every matched > structure (such as GICC) found, the parse function will be called, so > if we separate them it will duplicate the scanning of ACPI tables. Call it acpi_init_cpus() then. Lorenzo
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 221ff15..c82d4a1 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -58,11 +58,13 @@ static inline bool acpi_has_cpu_in_madt(void) } static inline void arch_fix_phys_package_id(int num, u32 slot) { } +void __init acpi_smp_init_cpus(void); #else static inline void disable_acpi(void) { } static inline bool acpi_psci_present(void) { return false; } static inline bool acpi_psci_use_hvc(void) { return false; } +static inline void acpi_smp_init_cpus(void) { } #endif /* CONFIG_ACPI */ #endif /*_ASM_ACPI_H*/ diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h index 6f8e2ef..5615970 100644 --- a/arch/arm64/include/asm/cpu_ops.h +++ b/arch/arm64/include/asm/cpu_ops.h @@ -66,5 +66,6 @@ struct cpu_operations { extern const struct cpu_operations *cpu_ops[NR_CPUS]; int __init cpu_read_ops(struct device_node *dn, int cpu); void __init cpu_read_bootcpu_ops(void); +const struct cpu_operations *cpu_get_ops(const char *name); #endif /* ifndef __ASM_CPU_OPS_H */ diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h index 780f82c..bf22650 100644 --- a/arch/arm64/include/asm/smp.h +++ b/arch/arm64/include/asm/smp.h @@ -39,9 +39,10 @@ extern void show_ipi_list(struct seq_file *p, int prec); extern void handle_IPI(int ipinr, struct pt_regs *regs); /* - * Setup the set of possible CPUs (via set_cpu_possible) + * Discover the set of possible CPUs and determine their + * SMP operations. */ -extern void smp_init_cpus(void); +extern void of_smp_init_cpus(void); /* * Provide a function to raise an IPI cross call on CPUs in callmap. diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 4177758..55d99d9 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -24,6 +24,10 @@ #include <linux/bootmem.h> #include <linux/smp.h> +#include <asm/smp_plat.h> +#include <asm/cputype.h> +#include <asm/cpu_ops.h> + int acpi_noirq; /* skip ACPI IRQ initialization */ int acpi_disabled; EXPORT_SYMBOL(acpi_disabled); @@ -31,6 +35,8 @@ EXPORT_SYMBOL(acpi_disabled); int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */ EXPORT_SYMBOL(acpi_pci_disabled); +static int enabled_cpus; /* Processors (GICC) with enabled flag in MADT */ + /* * __acpi_map_table() will be called before page_init(), so early_ioremap() * or early_memremap() should be called here to for ACPI table mapping. @@ -51,6 +57,134 @@ void __init __acpi_unmap_table(char *map, unsigned long size) early_memunmap(map, size); } +/** + * acpi_map_gic_cpu_interface - generates a logical cpu number + * and map to MPIDR represented by GICC structure + * @mpidr: CPU's hardware id to register, MPIDR represented in MADT + * @enabled: this cpu is enabled or not + * + * Returns the logical cpu number which maps to MPIDR + */ +static int acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) +{ + int cpu; + + if (mpidr == INVALID_HWID) { + pr_info("Skip MADT cpu entry with invalid MPIDR\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; + } + + /* No need to check duplicate MPIDRs for the first CPU */ + if (enabled_cpus) { + /* + * Duplicate MPIDRs are a recipe for disaster. Scan + * all initialized entries and check for + * duplicates. If any is found just ignore the CPU. + */ + for_each_possible_cpu(cpu) { + if (cpu_logical_map(cpu) == mpidr) { + pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", + mpidr); + return -EINVAL; + } + } + + /* allocate a logical cpu id for the new comer */ + cpu = cpumask_next_zero(-1, cpu_possible_mask); + } else { + /* + * First GICC entry must be BSP as ACPI spec said + * in section 5.2.12.15 + */ + if (cpu_logical_map(0) != mpidr) { + pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n", + mpidr); + return -EINVAL; + } + + /* + * boot_cpu_init() already hold bit 0 in cpu_present_mask + * for BSP, no need to allocate again. + */ + cpu = 0; + } + + /* CPU 0 was already initialized */ + if (cpu) { + cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); + if (!cpu_ops[cpu]) + return -EINVAL; + + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) + return -EOPNOTSUPP; + + /* map the logical cpu id to cpu MPIDR */ + cpu_logical_map(cpu) = mpidr; + + set_cpu_possible(cpu, true); + } else { + /* get cpu0's ops, no need to return if ops is null */ + cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL); + } + + enabled_cpus++; + return cpu; +} + +static int __init +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_madt_generic_interrupt *processor; + + processor = (struct acpi_madt_generic_interrupt *)header; + + if (BAD_MADT_ENTRY(processor, end)) + return -EINVAL; + + acpi_table_print_madt_entry(header); + + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, + processor->flags & ACPI_MADT_ENABLED); + + return 0; +} + +/* Parse GIC cpu interface entries in MADT for SMP init */ +void __init acpi_smp_init_cpus(void) +{ + int count; + + /* + * do a partial walk of MADT to determine how many CPUs + * we have including disabled CPUs, and get information + * we need for SMP init + */ + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, + acpi_parse_gic_cpu_interface, 0); + + if (!count) { + pr_err("No GIC CPU interface entries present\n"); + return; + } else if (count < 0) { + pr_err("Error parsing GIC CPU interface entry\n"); + return; + } + + /* Make boot-up look pretty */ + pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus); +} + static int __init acpi_parse_fadt(struct acpi_table_header *table) { struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; @@ -62,8 +196,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) * to get arm boot flags, or we will disable ACPI. */ if (table->revision > 5 || - (table->revision == 5 && fadt->minor_revision >= 1)) - return 0; + (table->revision == 5 && fadt->minor_revision >= 1)) { + /* + * ACPI 5.1 only has two explicit methods to boot up SMP, + * PSCI and Parking protocol, but the Parking protocol is + * only specified for ARMv7 now, so make PSCI as the only + * way for the SMP boot protocol before some updates for + * the ACPI spec or the Parking protocol spec. + */ + if (acpi_psci_present()) + return 0; + + pr_warn("No PSCI support, will not bring up secondary CPUs\n"); + return -EOPNOTSUPP; + } pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n", table->revision, fadt->minor_revision); diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c index cce9524..1ea7b9f 100644 --- a/arch/arm64/kernel/cpu_ops.c +++ b/arch/arm64/kernel/cpu_ops.c @@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops; const struct cpu_operations *cpu_ops[NR_CPUS]; -static const struct cpu_operations *supported_cpu_ops[] __initconst = { +static const struct cpu_operations *supported_cpu_ops[] = { #ifdef CONFIG_SMP &smp_spin_table_ops, #endif @@ -35,10 +35,13 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = { NULL, }; -static const struct cpu_operations * __init cpu_get_ops(const char *name) +const struct cpu_operations *cpu_get_ops(const char *name) { const struct cpu_operations **ops = supported_cpu_ops; + if (!name) + return NULL; + while (*ops) { if (!strcmp(name, (*ops)->name)) return *ops; diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index ef5b1e1..54e39e3 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -414,13 +414,16 @@ void __init setup_arch(char **cmdline_p) if (acpi_disabled) { unflatten_device_tree(); psci_dt_init(); + cpu_read_bootcpu_ops(); +#ifdef CONFIG_SMP + of_smp_init_cpus(); +#endif } else { psci_acpi_init(); + acpi_smp_init_cpus(); } - cpu_read_bootcpu_ops(); #ifdef CONFIG_SMP - smp_init_cpus(); smp_build_mpidr_hash(); #endif diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 7ae6ee0..5aaf5a4 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -323,7 +323,7 @@ void __init smp_prepare_boot_cpu(void) * cpu logical map array containing MPIDR values related to logical * cpus. Assumes that cpu_logical_map(0) has already been initialized. */ -void __init smp_init_cpus(void) +void __init of_smp_init_cpus(void) { struct device_node *dn = NULL; unsigned int i, cpu = 1;