Re: [PATCH v8 13/21] ARM64 / ACPI: Parse MADT for SMP initialization

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Feb 02, 2015 at 12:45:41PM +0000, 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.
>
> CC: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> CC: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> Tested-by: Yijing Wang <wangyijing@huawei.com>
> Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
> Tested-by: Jon Masters <jcm@redhat.com>
> Tested-by: Timur Tabi <timur@codeaurora.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
>  arch/arm64/include/asm/acpi.h    |   2 +
>  arch/arm64/include/asm/cpu_ops.h |   1 +
>  arch/arm64/include/asm/smp.h     |   5 +-
>  arch/arm64/kernel/acpi.c         | 150 ++++++++++++++++++++++++++++++++++++++-
>  arch/arm64/kernel/cpu_ops.c      |   2 +-
>  arch/arm64/kernel/setup.c        |   7 +-
>  arch/arm64/kernel/smp.c          |   2 +-
>  7 files changed, 161 insertions(+), 8 deletions(-)

[...]

> +/**
> + * 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 __init 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_possible_mask
> +                * for BSP, no need to allocate again.
> +                */
> +               cpu = 0;
> +       }

If/when kexec comes, on systems where CPU0 can be hotplugged the next
kernel might boot on an AP rather than the BSP. Is there a requirement
Linux-side that CPU0 is the BSP, or is this just intended as a sanity
check of the tables the FW provided?

> +
> +       if (!acpi_psci_present())
> +               return -EOPNOTSUPP;
> +
> +       cpu_ops[cpu] = cpu_get_ops("psci");
> +       /* CPU 0 was already initialized */
> +       if (cpu) {
> +               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);
> +       }

In the OF case we only set CPUs possible once we've scanned all the
nodes, and only when the boot CPU was actually found in a table. We
should keep the ACPI case consistent with that.

Can we not handle all of this in a later call once we've scanned all of
the GICC structures?

[...]

> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 43ae914..1099ddc 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -449,13 +449,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

I was going to say that it would be a little nicer if we had empty stubs
for functions in the !SMP case, rather than #ifdefs all over the place.
Unfortunately it looks like the way asm/smp.h is handled is generally a
mess, so this isn't so bad for now.

Thanks,
Mark.