Re: [PATCH RFC 11/35] ARM64 / ACPI: Parse MADT to map logical cpu to MPIDR and get cpu_possible/present_map

[Julien Grall <julien.grall@linaro.org>]

Hi Parth,

On 04/02/2015 14:01, parth.dixit@linaro.org wrote:
> From: Naresh Bhat <naresh.bhat@linaro.org>
>
> 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 and
> cpu_present_map.
>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> ---
>   xen/arch/arm/arm64/acpi/arm-core.c | 139 +++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/acpi.h         |   2 +
>   xen/include/xen/acpi.h             |   5 ++
>   3 files changed, 146 insertions(+)
>
> diff --git a/xen/arch/arm/arm64/acpi/arm-core.c b/xen/arch/arm/arm64/acpi/arm-core.c
> index 2b7e2ef..84b0032 100644
> --- a/xen/arch/arm/arm64/acpi/arm-core.c
> +++ b/xen/arch/arm/arm64/acpi/arm-core.c
> @@ -26,7 +26,10 @@
>   #include <xen/init.h>
>   #include <xen/acpi.h>
>   #include <xen/errno.h>
> +#include <xen/stdbool.h>
> +#include <xen/cpumask.h>
>
> +#include <asm/cputype.h>
>   #include <asm/acpi.h>
>
>   /*
> @@ -49,10 +52,141 @@ int acpi_psci_present;
>   /* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */
>   int acpi_psci_use_hvc;
>
> +/* available_cpus means enabled cpu in MADT */
> +static int available_cpus;
> +

Why adding available_cpus here, and total_cpus a bit below?

>   enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
>
>   struct acpi_arm_root acpi_arm_rsdp_info;     /* info about RSDP from FDT */
>
> +/* arch-optional setting to enable display of offline cpus >= nr_cpu_ids */
> +unsigned int total_cpus = 0;

Set but never used, neither exported...

> +/*
> + * acpi_register_gic_cpu_interface - register a gic cpu interface and
> + * generates a logic cpu number
> + * @mpidr: CPU's hardware id to register, MPIDR represented in MADT
> + * @enabled: this cpu is enabled or not
> + *
> + * Returns the logic cpu number which maps to the gic cpu interface
> + */
> +static int acpi_register_gic_cpu_interface(u64 mpidr, u8 enabled)
> +{
> +    int cpu;
> +
> +    if ( mpidr == INVALID_HWID )
> +    {
> +        printk("Skip invalid cpu hardware ID\n");
> +        return -EINVAL;
> +    }
> +
> +    total_cpus++;
> +    if ( !enabled )
> +        return -EINVAL;
> +
> +    if ( available_cpus >=  NR_CPUS )
> +    {
> +        printk("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n",
> +                            NR_CPUS, total_cpus, (long long unsigned int)mpidr);

Why the (long long unsigned int) cast? Please use PRIx64.

> +        return -EINVAL;
> +    }
> +
> +    /* If it is the first CPU, no need to check duplicate MPIDRs */
> +    if ( !available_cpus )
> +        goto skip_mpidr_check;

I don't think it's necessary, for_each_present_cpu should not contain 
the first CPU...

> +
> +    /*
> +    * 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_present_cpu(cpu)
> +    {
> +        if ( cpu_logical_map(cpu) == mpidr )
> +        {
> +            printk("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
> +                (long long unsigned int)mpidr);

Ditto for the cast.

> +            return -EINVAL;
> +        }
> +    }
> +
> +skip_mpidr_check:
> +    available_cpus++;
> +
> +    /* allocate a logic 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 allocte again.

allocate

> +         */
> +        cpu = 0;
> +    }
> +    else
> +        cpu = cpumask_next_zero(-1, &cpu_present_map);
> +
> +    /* map the logic cpu id to cpu MPIDR */
> +    cpu_logical_map(cpu) = mpidr;
> +
> +    set_cpu_possible(cpu, true);
> +    set_cpu_present(cpu, true);
> +
> +    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_register_gic_cpu_interface(processor->mpidr,
> +        processor->flags & ACPI_MADT_ENABLED);
> +
> +    return 0;
> +}
> +
> +/*
> + * Parse GIC cpu interface related entries in MADT
> + * returns 0 on success, < 0 on error
> + */
> +static int __init acpi_parse_madt_gic_cpu_interface_entries(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, MAX_GIC_CPU_INTERFACE);
> +
> +    if ( !count )
> +    {
> +        printk("No GIC CPU interface entries present\n");
> +        return -ENODEV;
> +    }
> +    else if ( count < 0 )
> +    {
> +        printk("Error parsing GIC CPU interface entry\n");
> +        return count;
> +    }
> +
> +    /* Make boot-up look pretty */
> +    printk("%d CPUs available, %d CPUs total\n", available_cpus,
> +        total_cpus);
> +
> +    return 0;
> +}
> +
>   int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
>   {
>       *irq = -1;
> @@ -140,6 +274,11 @@ int __init acpi_boot_init(void)
>       if ( err )
>           printk("Can't find FADT\n");

If we don't find the FADT, we should give up rather than trying to parse 
the GIC ACPI table.

>
> +    /* Get the boot CPU's MPIDR before MADT parsing */
> +    cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
> +
> +    err = acpi_parse_madt_gic_cpu_interface_entries();
> +
>       return err;
>   }
>   #endif
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 03051ef..c2d25db 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -105,4 +105,6 @@ static inline void acpi_disable_pci(void)
>   #define acpi_strict 1   /* no ACPI spec workarounds on ARM */
>   #endif
>
> +#define MAX_GIC_CPU_INTERFACE 65535
> +
>   #endif /*_ASM_ARM_ACPI_H*/
> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
> index ff96336..9387b36 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -67,6 +67,11 @@ typedef int (*acpi_table_handler) (struct acpi_table_header *table);
>   typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, const unsigned long end);
>
>   unsigned int acpi_get_processor_id (unsigned int cpu);
> +
> +#define BAD_MADT_ENTRY(entry, end) (                                        \
> +                (!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
> +                ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
> +

IHMO, this should belong to a separate patch.

>   char * __acpi_map_table (paddr_t phys_addr, unsigned long size);
>   int acpi_boot_init (void);
>   int acpi_boot_table_init (void);
>

Regards,

-- 
Julien Grall