| Message ID | 1409583475-6978-10-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 01, 2014 at 03:57:47PM +0100, 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. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > --- > arch/arm64/include/asm/acpi.h | 4 ++ > arch/arm64/include/asm/cpu_ops.h | 1 + > arch/arm64/include/asm/smp.h | 5 +- > arch/arm64/kernel/acpi.c | 144 ++++++++++++++++++++++++++++++++++++++ > arch/arm64/kernel/cpu_ops.c | 4 +- > arch/arm64/kernel/setup.c | 8 ++- > arch/arm64/kernel/smp.c | 2 +- > 7 files changed, 161 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index 620057c..e013dbb 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -51,6 +51,7 @@ 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); > > /* Low-level suspend routine. > * > @@ -64,10 +65,13 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { } > extern int (*acpi_suspend_lowlevel)(void); > #define acpi_wakeup_address 0 > > +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 > + > #else > > 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 */ > > diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h > index d7b4b38..d149580 100644 > --- a/arch/arm64/include/asm/cpu_ops.h > +++ b/arch/arm64/include/asm/cpu_ops.h > @@ -61,6 +61,7 @@ struct cpu_operations { > }; > > extern const struct cpu_operations *cpu_ops[NR_CPUS]; > +const struct cpu_operations *cpu_get_ops(const char *name); > extern int __init cpu_read_ops(struct device_node *dn, int cpu); > extern void __init cpu_read_bootcpu_ops(void); > > diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h > index a498f2c..c877adc 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 470570c..fbaaf01 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 */ Will this be ever different from (num_possible_cpus() - 1) ? > + > /* > * __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,144 @@ 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 invalid cpu hardware ID\n"); > + return -EINVAL; > + } > + > + total_cpus++; What's this used for ? > + 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; > + } > + } > + } else { > + /* Fist GICC entry must be BSP as ACPI spec said */ s/Fist/First/ > + if (cpu_logical_map(0) != mpidr) { > + pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n", > + mpidr); > + return -EINVAL; > + } Interesting, this means that if I want to change the boot CPU I have to recompile the ACPI tables. Is that really true ? > + } > + > + /* 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_possible_mask); > + } You may use a ternary operator, more compact and clearer. BTW you seem to be contradicting yourself. On one hand you keep a counter for enabled_cpus, and then use cpu_possible_mask to allocate a logical cpu id. Make a decision, either you use a counter or you use cpu_possible_mask and its bitweight. > + /* > + * 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()) { > + pr_warn("CPU %d has no PSCI support, will not boot\n", cpu); > + return -EOPNOTSUPP; > + } This check really does not belong here. You do not even start parsing the gic cpu interfaces if psci is missing or I am missing something myself. Anyway, this check must not be in this function. > + > + /* Get cpu_ops include the boot CPU */ > + cpu_ops[cpu] = cpu_get_ops("psci"); > + if (!cpu_ops[cpu]) > + return -EINVAL; > + > + /* CPU 0 was already initialized */ > + if (cpu) { > + 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); > + } > + > + enabled_cpus++; See above to me enabled_cpus and (num_possible_cpus() - 1) are identical. > + 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, > + processor->flags & ACPI_MADT_ENABLED); Ehm. You must check the return value here right (and return an error if that's an error, otherwise the count value below can be botched ?!). Or you do not consider a parsing error as an error and want to keep parsing remaining GIC CPU IF entries ? > + > + 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, > + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); > + > + 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; > + } What would you consider an error ? A single GIC CPU IF entry error ? Thanks, Lorenzo > + /* 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; > diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c > index cce9524..1a04deb 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,7 +35,7 @@ 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; > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index ac9ec55..a45ceb3 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -60,6 +60,7 @@ > #include <asm/memblock.h> > #include <asm/psci.h> > #include <asm/efi.h> > +#include <asm/acpi.h> > > unsigned int processor_id; > EXPORT_SYMBOL(processor_id); > @@ -398,13 +399,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 4743397..4e390ac 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -321,7 +321,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.7.9.5 > >
Hi Lorenzo, On 2014?09?04? 01:21, Lorenzo Pieralisi wrote: > On Mon, Sep 01, 2014 at 03:57:47PM +0100, 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. [...] >> 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 */ > Will this be ever different from (num_possible_cpus() - 1) ? Yes, num_possible_cpus() will much more than enabled cpus in MADT, when ACPI based CPU hot plug is introduced, you can refer to the code in x86. > >> + >> /* >> * __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,144 @@ 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 invalid cpu hardware ID\n"); >> + return -EINVAL; >> + } >> + >> + total_cpus++; > What's this used for ? It is for all the CPU entries in MADT table, it is used to let people know how many CPUs in MADT (enabled and disabled). > >> + 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; >> + } >> + } >> + } else { >> + /* Fist GICC entry must be BSP as ACPI spec said */ > s/Fist/First/ > >> + if (cpu_logical_map(0) != mpidr) { >> + pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n", >> + mpidr); >> + return -EINVAL; >> + } > Interesting, this means that if I want to change the boot CPU I have to > recompile the ACPI tables. Is that really true ? No, you needn't. there is a logic problem here, we just need to print some message here and continue, OS will still ok with that. > >> + } >> + >> + /* 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_possible_mask); >> + } > You may use a ternary operator, more compact and clearer. > > BTW you seem to be contradicting yourself. On one hand you keep a > counter for enabled_cpus, and then use cpu_possible_mask to allocate > a logical cpu id. Make a decision, either you use a counter or you > use cpu_possible_mask and its bitweight. ok. > >> + /* >> + * 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()) { >> + pr_warn("CPU %d has no PSCI support, will not boot\n", cpu); >> + return -EOPNOTSUPP; >> + } > This check really does not belong here. You do not even start parsing the gic > cpu interfaces if psci is missing or I am missing something myself. Anyway, > this check must not be in this function. I agree with you, i will update the patch. > >> + >> + /* Get cpu_ops include the boot CPU */ >> + cpu_ops[cpu] = cpu_get_ops("psci"); >> + if (!cpu_ops[cpu]) >> + return -EINVAL; >> + >> + /* CPU 0 was already initialized */ >> + if (cpu) { >> + 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); >> + } >> + >> + enabled_cpus++; > See above to me enabled_cpus and (num_possible_cpus() - 1) are identical. I think I need to remove all the CPU hotplug related code and make this function as simple as possible and introduce them when needed. > >> + 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, >> + processor->flags & ACPI_MADT_ENABLED); > Ehm. You must check the return value here right (and return an error if > that's an error, otherwise the count value below can be botched ?!). > > Or you do not consider a parsing error as an error and want to keep > parsing remaining GIC CPU IF entries ? yes, this is my intension. we can skip the error ones and boot other CPUs which have no errors. > >> + >> + 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, >> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >> + >> + 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; >> + } > What would you consider an error ? A single GIC CPU IF entry error ? could you please explain it in detail? I can't catch up with you, my apologizes. Thanks Hanjun
On 09/01/2014 10:57 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. > + /* CPU 0 was already initialized */ > + if (cpu) { > + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) > + return -EOPNOTSUPP; > + > + /* map the logical cpu id to cpu MPIDR */ > + cpu_logical_map(cpu) = mpidr; I'm not sure it's worth noting in a comment or just in the dialogue that none of these MPIDR values is literally the value in the MPIDR. Linux doesn't store that anyway (even in the cpu_logical_map), since it is pre-filtered against MPIDR_HWID_BITMASK to remove the non-affinity level bits. And since the ACPI5.1 specification requires that non-affinity bits be zero everything works. But it relies upon this assumption so it might be worth explicitly masking out the bits when making the call into: acpi_map_gic_cpu_interface(processor->arm_mpidr, processor->flags & ACPI_MADT_ENABLED); During the parsing of the processor object's MPIDR value. Jon.
Hi Hanjun, Lorenzo, Resending due to my mail client removing list CCs...sorry about that. On 09/04/2014 11:29 AM, Hanjun Guo wrote: >>> + } else { >>> + /* Fist GICC entry must be BSP as ACPI spec said */ >> s/Fist/First/ >> >>> + if (cpu_logical_map(0) != mpidr) { >>> + pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n", >>> + mpidr); >>> + return -EINVAL; >>> + } >> Interesting, this means that if I want to change the boot CPU I have to >> recompile the ACPI tables. Is that really true ? Well, the ACPI5.1 specification does require that the PEs (cores) be listed in a very specific order, with the boot CPU first, and then a precisely defined sequence of interleaving of any possible SMT threads with other cores. So I think you would in practice update your tables. >>> + /* >>> + * 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. >>> + */ The Parking Protocol may be updated for a (limited) number of platforms that may use it in the early days. The preferred option (as described in the SBBR) is to use PSCI when at all possible. Some implementations of the architecture may not be able to use PSCI for MP-Boot. Thus while there may be some limited early use of the parking protocol (including while PSCI firmware is being finalized during bringup activities), it will ultimately be completely replaced by PSCI based boot over time. Jon.
Hi Jon, On 2014?09?09? 12:23, Jon Masters wrote: > On 09/01/2014 10:57 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. >> + /* CPU 0 was already initialized */ >> + if (cpu) { >> + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) >> + return -EOPNOTSUPP; >> + >> + /* map the logical cpu id to cpu MPIDR */ >> + cpu_logical_map(cpu) = mpidr; > I'm not sure it's worth noting in a comment or just in the dialogue that > none of these MPIDR values is literally the value in the MPIDR. Linux > doesn't store that anyway (even in the cpu_logical_map), since it is > pre-filtered against MPIDR_HWID_BITMASK to remove the non-affinity level > bits. And since the ACPI5.1 specification requires that non-affinity > bits be zero everything works. But it relies upon this assumption so it > might be worth explicitly masking out the bits when making the call into: > > acpi_map_gic_cpu_interface(processor->arm_mpidr, > processor->flags & ACPI_MADT_ENABLED); > > During the parsing of the processor object's MPIDR value. Yes, I agree with you. When I tested this patch set on our ARM64 platform, I found this problem too. some firmware will just present the real MPIDR value to OS which some reserved bit set to 1, and it will lead to some logic problem in this patch. (actually firmware didn't obey with ACPI spec) I had updated the patch with: + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, + processor->flags & ACPI_MADT_ENABLED); and then the problem was gone :) Thanks Hanjun
On 2014?09?09? 12:29, Jon Masters wrote: > Hi Hanjun, Lorenzo, Hi Jon, > > Resending due to my mail client removing list CCs...sorry about that. > > On 09/04/2014 11:29 AM, Hanjun Guo wrote: > >>>> + } else { >>>> + /* Fist GICC entry must be BSP as ACPI spec said */ >>> s/Fist/First/ >>> >>>> + if (cpu_logical_map(0) != mpidr) { >>>> + pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n", >>>> + mpidr); >>>> + return -EINVAL; >>>> + } >>> Interesting, this means that if I want to change the boot CPU I have to >>> recompile the ACPI tables. Is that really true ? > Well, the ACPI5.1 specification does require that the PEs (cores) be > listed in a very specific order, with the boot CPU first, and then a > precisely defined sequence of interleaving of any possible SMT threads > with other cores. So I think you would in practice update your tables. Thanks for the clarify. > >>>> + /* >>>> + * 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. >>>> + */ > The Parking Protocol may be updated for a (limited) number of platforms > that may use it in the early days. The preferred option (as described in > the SBBR) is to use PSCI when at all possible. Some implementations of > the architecture may not be able to use PSCI for MP-Boot. Thus while > there may be some limited early use of the parking protocol (including > while PSCI firmware is being finalized during bringup activities), it > will ultimately be completely replaced by PSCI based boot over time. Thank you for the clarify again :) Will Parking Protocol be upstreamed? If yes, I think we can update the comments when Parking Protocol driver upstreamed. Thanks Hanjun
On 09/09/2014 01:11 AM, Hanjun Guo wrote: > On 2014?09?09? 12:29, Jon Masters wrote: >> Hi Hanjun, Lorenzo, > > Hi Jon, > >> >> Resending due to my mail client removing list CCs...sorry about that. >> >> On 09/04/2014 11:29 AM, Hanjun Guo wrote: >> >>>>> + } else { >>>>> + /* Fist GICC entry must be BSP as ACPI spec said */ >>>> s/Fist/First/ >>>> >>>>> + if (cpu_logical_map(0) != mpidr) { >>>>> + pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n", >>>>> + mpidr); >>>>> + return -EINVAL; >>>>> + } >>>> Interesting, this means that if I want to change the boot CPU I have to >>>> recompile the ACPI tables. Is that really true ? >> Well, the ACPI5.1 specification does require that the PEs (cores) be >> listed in a very specific order, with the boot CPU first, and then a >> precisely defined sequence of interleaving of any possible SMT threads >> with other cores. So I think you would in practice update your tables. > > Thanks for the clarify. No problem :) I'm trying to go through the various threads and ensure various things are documented in the record. I'm skipping (for now) notes on e.g. CPU cache flush behavior because we're not pushing for C3 support (or really any runtime power management) in the first round. >> >>>>> + /* >>>>> + * 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. >>>>> + */ >> The Parking Protocol may be updated for a (limited) number of platforms >> that may use it in the early days. The preferred option (as described in >> the SBBR) is to use PSCI when at all possible. Some implementations of >> the architecture may not be able to use PSCI for MP-Boot. Thus while >> there may be some limited early use of the parking protocol (including >> while PSCI firmware is being finalized during bringup activities), it >> will ultimately be completely replaced by PSCI based boot over time. > > Thank you for the clarify again :) > > Will Parking Protocol be upstreamed? Yes, a version against v8 will be posted (perhaps in the next few days), but we should not wait for it in this general discussion. The default in v8 will be PSCI other than for early bringup, or for an architecture implementation that cannot use PSCI, which is rare. (When we wrote the SBBR we had many discussions about this. A goal is to be fair to everyone - those rare instances where PSCI is not possible must be covered by a specification, but there must be a strong preference established for a preferred course over the longer term) > If yes, I think we can update the > comments when Parking Protocol driver upstreamed. Indeed so. For the moment, the logic you have in your patch to fail setup in the case that there is no defined PSCI cpu ops for the associated processor is fine IMHO. Jon.
On 09/09/2014 12:57 AM, Hanjun Guo wrote: > Hi Jon, > > On 2014?09?09? 12:23, Jon Masters wrote: >> On 09/01/2014 10:57 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. >>> + /* CPU 0 was already initialized */ >>> + if (cpu) { >>> + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) >>> + return -EOPNOTSUPP; >>> + >>> + /* map the logical cpu id to cpu MPIDR */ >>> + cpu_logical_map(cpu) = mpidr; >> I'm not sure it's worth noting in a comment or just in the dialogue that >> none of these MPIDR values is literally the value in the MPIDR. Linux >> doesn't store that anyway (even in the cpu_logical_map), since it is >> pre-filtered against MPIDR_HWID_BITMASK to remove the non-affinity level >> bits. And since the ACPI5.1 specification requires that non-affinity >> bits be zero everything works. But it relies upon this assumption so it >> might be worth explicitly masking out the bits when making the call into: >> >> acpi_map_gic_cpu_interface(processor->arm_mpidr, >> processor->flags & ACPI_MADT_ENABLED); >> >> During the parsing of the processor object's MPIDR value. > > Yes, I agree with you. When I tested this patch set on our > ARM64 platform, I found this problem too. some firmware > will just present the real MPIDR value to OS which some reserved > bit set to 1, and it will lead to some logic problem in this patch. > (actually firmware didn't obey with ACPI spec) > > I had updated the patch with: > > + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, > + processor->flags & ACPI_MADT_ENABLED); > > and then the problem was gone :) Did I miss an updated patch posting then? It is possible...I was keeping out of this thread for "obvious" reasons (I'm somewhat biased in favor of ACPI on 64-bit ARM server platforms and thus not objective in all cases...so I am confining my feedback to technical specifics). But it's necessary that there be a little more discussion here. I've got a couple of requests into various vendors to get more vocal too. Jon.
On 2014?09?09? 13:44, Jon Masters wrote: > On 09/09/2014 12:57 AM, Hanjun Guo wrote: >> Hi Jon, >> >> On 2014?09?09? 12:23, Jon Masters wrote: >>> On 09/01/2014 10:57 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. >>>> + /* CPU 0 was already initialized */ >>>> + if (cpu) { >>>> + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) >>>> + return -EOPNOTSUPP; >>>> + >>>> + /* map the logical cpu id to cpu MPIDR */ >>>> + cpu_logical_map(cpu) = mpidr; >>> I'm not sure it's worth noting in a comment or just in the dialogue that >>> none of these MPIDR values is literally the value in the MPIDR. Linux >>> doesn't store that anyway (even in the cpu_logical_map), since it is >>> pre-filtered against MPIDR_HWID_BITMASK to remove the non-affinity level >>> bits. And since the ACPI5.1 specification requires that non-affinity >>> bits be zero everything works. But it relies upon this assumption so it >>> might be worth explicitly masking out the bits when making the call into: >>> >>> acpi_map_gic_cpu_interface(processor->arm_mpidr, >>> processor->flags & ACPI_MADT_ENABLED); >>> >>> During the parsing of the processor object's MPIDR value. >> Yes, I agree with you. When I tested this patch set on our >> ARM64 platform, I found this problem too. some firmware >> will just present the real MPIDR value to OS which some reserved >> bit set to 1, and it will lead to some logic problem in this patch. >> (actually firmware didn't obey with ACPI spec) >> >> I had updated the patch with: >> >> + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, >> + processor->flags & ACPI_MADT_ENABLED); >> >> and then the problem was gone :) > Did I miss an updated patch posting then? It is possible... No, you didn't miss it, I'm still working on the new version, sorry I didn't clarify that in my previous email. Thanks Hanjun
On 09/09/2014 12:00 PM, Hanjun Guo wrote: > On 2014?09?09? 13:44, Jon Masters wrote: >> On 09/09/2014 12:57 AM, Hanjun Guo wrote: >>> Hi Jon, >>> >>> On 2014?09?09? 12:23, Jon Masters wrote: >>>> On 09/01/2014 10:57 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. >>>>> + /* CPU 0 was already initialized */ >>>>> + if (cpu) { >>>>> + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) >>>>> + return -EOPNOTSUPP; >>>>> + >>>>> + /* map the logical cpu id to cpu MPIDR */ >>>>> + cpu_logical_map(cpu) = mpidr; >>>> I'm not sure it's worth noting in a comment or just in the dialogue that >>>> none of these MPIDR values is literally the value in the MPIDR. Linux >>>> doesn't store that anyway (even in the cpu_logical_map), since it is >>>> pre-filtered against MPIDR_HWID_BITMASK to remove the non-affinity level >>>> bits. And since the ACPI5.1 specification requires that non-affinity >>>> bits be zero everything works. But it relies upon this assumption so it >>>> might be worth explicitly masking out the bits when making the call into: >>>> >>>> acpi_map_gic_cpu_interface(processor->arm_mpidr, >>>> processor->flags & ACPI_MADT_ENABLED); >>>> >>>> During the parsing of the processor object's MPIDR value. >>> Yes, I agree with you. When I tested this patch set on our >>> ARM64 platform, I found this problem too. some firmware >>> will just present the real MPIDR value to OS which some reserved >>> bit set to 1, and it will lead to some logic problem in this patch. >>> (actually firmware didn't obey with ACPI spec) >>> >>> I had updated the patch with: >>> >>> + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, >>> + processor->flags & ACPI_MADT_ENABLED); >>> >>> and then the problem was gone :) >> Did I miss an updated patch posting then? It is possible... > > No, you didn't miss it, I'm still working on the new version, sorry I didn't > clarify that in my previous email. Thanks. If you could copy me on the next posting that would rock. In a few hours we should have another platform posted as an example. In addition, a couple of lower priority patches (building upon the core ACPI pieces) should be posted as well. Jon.
On 2014?09?10? 00:04, Jon Masters wrote: > On 09/09/2014 12:00 PM, Hanjun Guo wrote: >> On 2014?09?09? 13:44, Jon Masters wrote: >>> On 09/09/2014 12:57 AM, Hanjun Guo wrote: >>>> Hi Jon, >>>> >>>> On 2014?09?09? 12:23, Jon Masters wrote: >>>>> On 09/01/2014 10:57 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. >>>>>> + /* CPU 0 was already initialized */ >>>>>> + if (cpu) { >>>>>> + if (cpu_ops[cpu]->cpu_init(NULL, cpu)) >>>>>> + return -EOPNOTSUPP; >>>>>> + >>>>>> + /* map the logical cpu id to cpu MPIDR */ >>>>>> + cpu_logical_map(cpu) = mpidr; >>>>> I'm not sure it's worth noting in a comment or just in the dialogue that >>>>> none of these MPIDR values is literally the value in the MPIDR. Linux >>>>> doesn't store that anyway (even in the cpu_logical_map), since it is >>>>> pre-filtered against MPIDR_HWID_BITMASK to remove the non-affinity level >>>>> bits. And since the ACPI5.1 specification requires that non-affinity >>>>> bits be zero everything works. But it relies upon this assumption so it >>>>> might be worth explicitly masking out the bits when making the call into: >>>>> >>>>> acpi_map_gic_cpu_interface(processor->arm_mpidr, >>>>> processor->flags & ACPI_MADT_ENABLED); >>>>> >>>>> During the parsing of the processor object's MPIDR value. >>>> Yes, I agree with you. When I tested this patch set on our >>>> ARM64 platform, I found this problem too. some firmware >>>> will just present the real MPIDR value to OS which some reserved >>>> bit set to 1, and it will lead to some logic problem in this patch. >>>> (actually firmware didn't obey with ACPI spec) >>>> >>>> I had updated the patch with: >>>> >>>> + acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK, >>>> + processor->flags & ACPI_MADT_ENABLED); >>>> >>>> and then the problem was gone :) >>> Did I miss an updated patch posting then? It is possible... >> No, you didn't miss it, I'm still working on the new version, sorry I didn't >> clarify that in my previous email. > Thanks. If you could copy me on the next posting that would rock. Sure I will. > In a > few hours we should have another platform posted as an example. In > addition, a couple of lower priority patches (building upon the core > ACPI pieces) should be posted as well. That will be great! :) Thanks Hanjun
On Thu, Sep 04, 2014 at 04:29:15PM +0100, Hanjun Guo wrote: > Hi Lorenzo, > > On 2014?09?04? 01:21, Lorenzo Pieralisi wrote: > > On Mon, Sep 01, 2014 at 03:57:47PM +0100, 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. > [...] > >> 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 */ > > Will this be ever different from (num_possible_cpus() - 1) ? > > Yes, num_possible_cpus() will much more than enabled cpus > in MADT, when ACPI based CPU hot plug is introduced, you can refer > to the code in x86. Ok, but in the context of this patch to me they represent the same value. I understand you need a counter, which you should probably use to enumerate the logical cpus instead of resorting to the first empty slot in cpu_possible_mask. Anyway, it is a minor point, please be consistent that's all I am asking. > >> + > >> /* > >> * __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,144 @@ 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 invalid cpu hardware ID\n"); > >> + return -EINVAL; > >> + } > >> + > >> + total_cpus++; > > What's this used for ? > > It is for all the CPU entries in MADT table, it is used to let > people know how many CPUs in MADT (enabled and disabled). I think its usage is very limited at the moment, again it is not a major point, I was just asking, I certainly do not think it is essential at this stage (apart from debugging the parsing code). > >> + 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; > >> + } > >> + } > >> + } else { > >> + /* Fist GICC entry must be BSP as ACPI spec said */ > > s/Fist/First/ > > > >> + if (cpu_logical_map(0) != mpidr) { > >> + pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n", > >> + mpidr); > >> + return -EINVAL; > >> + } > > Interesting, this means that if I want to change the boot CPU I have to > > recompile the ACPI tables. Is that really true ? > > No, you needn't. there is a logic problem here, we just need to print > some message here and continue, OS will still ok with that. I need to look at the specs here. I do not like fixed dependencies on the boot CPU, which risk being translated in dependencies on first/last CPU going-to/getting-out-of idle and that is a major concern, among others. > >> + } > >> + > >> + /* 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_possible_mask); > >> + } > > You may use a ternary operator, more compact and clearer. > > > > BTW you seem to be contradicting yourself. On one hand you keep a > > counter for enabled_cpus, and then use cpu_possible_mask to allocate > > a logical cpu id. Make a decision, either you use a counter or you > > use cpu_possible_mask and its bitweight. > > ok. > > > > >> + /* > >> + * 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()) { > >> + pr_warn("CPU %d has no PSCI support, will not boot\n", cpu); > >> + return -EOPNOTSUPP; > >> + } > > This check really does not belong here. You do not even start parsing the gic > > cpu interfaces if psci is missing or I am missing something myself. Anyway, > > this check must not be in this function. > > I agree with you, i will update the patch. > > > > >> + > >> + /* Get cpu_ops include the boot CPU */ > >> + cpu_ops[cpu] = cpu_get_ops("psci"); > >> + if (!cpu_ops[cpu]) > >> + return -EINVAL; > >> + > >> + /* CPU 0 was already initialized */ > >> + if (cpu) { > >> + 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); > >> + } > >> + > >> + enabled_cpus++; > > See above to me enabled_cpus and (num_possible_cpus() - 1) are identical. > > I think I need to remove all the CPU hotplug related code and make this function > as simple as possible and introduce them when needed. Yes that makes sense, even though a bit of foresight is always appreciated; I certainly do not want you to completely rewrite this code to support CPU hotplug to be 100% clear. "Disabled" CPUs is a concept that is not managed at the moment with DT (on ARM and ARM64), and we need to introduce it properly. Again, I was asking questions, to understand why you would need those variables. Have a look at this discussion: https://lkml.org/lkml/2013/6/6/470 > > > > >> + 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, > >> + processor->flags & ACPI_MADT_ENABLED); > > Ehm. You must check the return value here right (and return an error if > > that's an error, otherwise the count value below can be botched ?!). > > > > Or you do not consider a parsing error as an error and want to keep > > parsing remaining GIC CPU IF entries ? > > yes, this is my intension. we can skip the error ones and boot > other CPUs which have no errors. > > > > >> + > >> + 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, > >> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); > >> + > >> + 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; > >> + } > > What would you consider an error ? A single GIC CPU IF entry error ? > > could you please explain it in detail? I can't catch up with you, my apologizes. You explained to me above. A bogus entry does not stop you from parsing other CPUs, this is a design choice and that's what we do in ARM64 DT today, so I would say that's fine. Lorenzo
On 09/09/2014 12:52 PM, Lorenzo Pieralisi wrote: > On Thu, Sep 04, 2014 at 04:29:15PM +0100, Hanjun Guo wrote: >> Hi Lorenzo, >>>> + 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; >>>> + } >>>> + } >>>> + } else { >>>> + /* Fist GICC entry must be BSP as ACPI spec said */ >>> s/Fist/First/ >>> >>>> + if (cpu_logical_map(0) != mpidr) { >>>> + pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n", >>>> + mpidr); >>>> + return -EINVAL; >>>> + } >>> Interesting, this means that if I want to change the boot CPU I have to >>> recompile the ACPI tables. Is that really true ? >> >> No, you needn't. there is a logic problem here, we just need to print >> some message here and continue, OS will still ok with that. > > I need to look at the specs here. I do not like fixed dependencies on > the boot CPU, which risk being translated in dependencies on first/last > CPU going-to/getting-out-of idle and that is a major concern, among > others. See page 149 of the ACPI5.1 specification document (5.2.12.14 GICC Structure): "Note: GICC descriptor structures are listed immediately after the Flags field in the MADT, one descriptor for each GICC, followed by one for each GICC Distributor. The Local GICC corresponding to the boot processor must be the first entry in the Interrupt Controller Structure list." Jon.
On 09/09/2014 12:52 PM, Lorenzo Pieralisi wrote: > On Thu, Sep 04, 2014 at 04:29:15PM +0100, Hanjun Guo wrote: >>>> + >>>> + /* Get cpu_ops include the boot CPU */ >>>> + cpu_ops[cpu] = cpu_get_ops("psci"); >>>> + if (!cpu_ops[cpu]) >>>> + return -EINVAL; >>>> + >>>> + /* CPU 0 was already initialized */ >>>> + if (cpu) { >>>> + 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); >>>> + } >>>> + >>>> + enabled_cpus++; >>> See above to me enabled_cpus and (num_possible_cpus() - 1) are identical. >> >> I think I need to remove all the CPU hotplug related code and make this function >> as simple as possible and introduce them when needed. > > Yes that makes sense, even though a bit of foresight is always appreciated; > I certainly do not want you to completely rewrite this code to support CPU > hotplug to be 100% clear. "Disabled" CPUs is a concept that is not > managed at the moment with DT (on ARM and ARM64), and we need to introduce it > properly. Again, I was asking questions, to understand why you would need > those variables. > > Have a look at this discussion: > > https://lkml.org/lkml/2013/6/6/470 There could also be other reasons to disable "CPUs". For example, in an SMT system configured not to use SMT threads, you might (in one instantiation) decide to simply mark such GICCs as not enabled. Jon.
On Mon, 1 Sep 2014 22:57:47 +0800, Hanjun Guo <hanjun.guo@linaro.org> 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. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > --- > +/** > + * 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 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; > + } > + > + /* 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; > + } > + } > + } else { > + /* Fist GICC entry must be BSP as ACPI spec said */ > + if (cpu_logical_map(0) != mpidr) { > + pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n", > + mpidr); > + return -EINVAL; > + } > + } > + > + /* 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_possible_mask); > + } Nit: so the above two if/else blocks are essentially testing for the same condition: Is this the first cpu? or a secondary cpu? I would merge the two into a single if/else block. g.
On Tue, Sep 09, 2014 at 05:04:15PM +0100, Jon Masters wrote: > On 09/09/2014 12:00 PM, Hanjun Guo wrote: > > No, you didn't miss it, I'm still working on the new version, sorry I didn't > > clarify that in my previous email. > > Thanks. If you could copy me on the next posting that would rock. In a > few hours we should have another platform posted as an example. In > addition, a couple of lower priority patches (building upon the core > ACPI pieces) should be posted as well. Can I breath out yet or did I miss the platform posting? Will
On 09/11/2014 10:15 AM, Will Deacon wrote: > On Tue, Sep 09, 2014 at 05:04:15PM +0100, Jon Masters wrote: >> On 09/09/2014 12:00 PM, Hanjun Guo wrote: >>> No, you didn't miss it, I'm still working on the new version, sorry I didn't >>> clarify that in my previous email. >> >> Thanks. If you could copy me on the next posting that would rock. In a >> few hours we should have another platform posted as an example. In >> addition, a couple of lower priority patches (building upon the core >> ACPI pieces) should be posted as well. > > Can I breath out yet or did I miss the platform posting? There's a posting for AMD Seattle coming soon as an example. Also, there should be some hardware at Connect on which patches can run. Jon.
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 620057c..e013dbb 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -51,6 +51,7 @@ 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); /* Low-level suspend routine. * @@ -64,10 +65,13 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { } extern int (*acpi_suspend_lowlevel)(void); #define acpi_wakeup_address 0 +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 + #else 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 */ diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h index d7b4b38..d149580 100644 --- a/arch/arm64/include/asm/cpu_ops.h +++ b/arch/arm64/include/asm/cpu_ops.h @@ -61,6 +61,7 @@ struct cpu_operations { }; extern const struct cpu_operations *cpu_ops[NR_CPUS]; +const struct cpu_operations *cpu_get_ops(const char *name); extern int __init cpu_read_ops(struct device_node *dn, int cpu); extern void __init cpu_read_bootcpu_ops(void); diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h index a498f2c..c877adc 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 470570c..fbaaf01 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,144 @@ 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 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; + } + + /* 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; + } + } + } else { + /* Fist GICC entry must be BSP as ACPI spec said */ + if (cpu_logical_map(0) != mpidr) { + pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n", + mpidr); + return -EINVAL; + } + } + + /* 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_possible_mask); + } + + /* + * 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()) { + pr_warn("CPU %d has no PSCI support, will not boot\n", cpu); + return -EOPNOTSUPP; + } + + /* Get cpu_ops include the boot CPU */ + cpu_ops[cpu] = cpu_get_ops("psci"); + if (!cpu_ops[cpu]) + return -EINVAL; + + /* CPU 0 was already initialized */ + if (cpu) { + 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); + } + + 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, + 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, + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); + + 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; diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c index cce9524..1a04deb 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,7 +35,7 @@ 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; diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index ac9ec55..a45ceb3 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -60,6 +60,7 @@ #include <asm/memblock.h> #include <asm/psci.h> #include <asm/efi.h> +#include <asm/acpi.h> unsigned int processor_id; EXPORT_SYMBOL(processor_id); @@ -398,13 +399,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 4743397..4e390ac 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -321,7 +321,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;