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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
