Re: [PATCH v7 10/17] ARM64 / ACPI: Parse MADT for SMP initialization

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

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