Re: [PATCH v2 13/41] arm/acpi : parse MADT to map logical cpu to MPIDR and get cpu_possible_map

[Julien Grall <julien.grall@citrix.com>]

Hi Parth,

On 17/05/15 21:03, Parth Dixit 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.
> 
> 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>
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>

This patch looks very similar to Linux. Unless you say that we have to
diverge, I would prefer if you important the patch as it is from Linux
(i.e no coding style changes, stubbing out necessary code) and modify
only a little bit.

It will be easier to backport fixes from Linux. There are other patches
where this comment apply.

Also, please check that we are using the latest version of the patch.

> ---
>  xen/arch/arm/acpi/boot.c     | 135 +++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/arm64/smpboot.c |   7 ++-
>  xen/arch/arm/psci.c          |  16 +++++
>  xen/arch/arm/smpboot.c       |   7 ++-
>  xen/include/asm-arm/acpi.h   |   2 +
>  xen/include/xen/acpi.h       |   5 ++
>  6 files changed, 170 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 57eb33c..7d9758f 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -29,6 +29,141 @@
>  #include <xen/mm.h>
>  
>  #include <asm/acpi.h>
> +#include <asm/smp.h>
> +
> +/* Processors with enabled flag and sane MPIDR */
> +static int enabled_cpus;
> +
> +/* Boot CPU is valid or not in MADT */
> +static bool_t __initdata bootcpu_valid;
> +
> +/* arch-optional setting to enable display of offline cpus >= nr_cpu_ids */
> +static unsigned int total_cpus = 0;

Given the usage within this file, the comment is wrong.

Also, please add __init as it's only used during initialization.

> +
> +/*
> + * 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)

You never use the return value. Please make this function void.

> +{
> +    int i;
> +
> +    if ( mpidr == MPIDR_INVALID )
> +    {
> +        printk("Skip MADT cpu entry with invalid MPIDR\n");
> +        return -EINVAL;
> +    }
> +
> +    total_cpus++;
> +    if ( !enabled )
> +        return -EINVAL;

You are skipping the CPU if it's not enabled. How would that fit with
CPU hotplug?

It looks like that on x86 we allocate disabled CPUs in the possible map
(see acpi_parse_lapic in arch/x86/acpi/boot.c).

> +
> +    if ( enabled_cpus >=  NR_CPUS )
> +    {
> +        printk("NR_CPUS limit of %d reached, Processor %d/0x%"PRIx64" ignored.\n",
> +                NR_CPUS, total_cpus, mpidr);
> +        return -EINVAL;
> +    }
> +
> +    /* Check if GICC structure of boot CPU is available in the MADT */
> +    if ( cpu_logical_map(0) == mpidr )
> +    {
> +        if ( bootcpu_valid )
> +        {
> +            printk("Firmware bug, duplicate CPU MPIDR: 0x%"PRIx64" in MADT\n",
> +                    mpidr);
> +            return -EINVAL;
> +        }
> +
> +        bootcpu_valid = TRUE;
> +    }
> +
> +    /*
> +     * Duplicate MPIDRs are a recipe for disaster. Scan
> +     * all initialized entries and check for
> +     * duplicates. If any is found just ignore the CPU.
> +     */
> +    for ( i = 1; i < enabled_cpus; i++ )
> +    {
> +        if ( cpu_logical_map(i) == mpidr )
> +        {
> +            printk("Firmware bug, duplicate CPU MPIDR: 0x%"PRIx64" in MADT\n",
> +                    mpidr);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    if ( !acpi_psci_present() )
> +        return -EOPNOTSUPP;
> +
> +    /* CPU 0 was already initialized */
> +    if ( enabled_cpus )
> +    {
> +        if ( arch_cpu_init(enabled_cpus, NULL) < 0 )
> +            return -EOPNOTSUPP;
> +
> +        /* map the logical cpu id to cpu MPIDR */
> +        cpu_logical_map(enabled_cpus) = mpidr;
> +    }
> +
> +    enabled_cpus++;

Missing new line.

> +    return enabled_cpus;
> +}
> +
> +static int __init
> +    acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
> +            const unsigned long end)

The indentation looks wrong.

> +{
> +    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->mpidr & MPIDR_HWID_MASK,
> +            processor->flags & ACPI_MADT_ENABLED);

It would be cleaner if you pass directly processor to the function (as
Linux does).

> +
> +    return 0;
> +}
> +
> +/* Parse GIC cpu interface entries in MADT for SMP init */
> +void __init acpi_init_cpus(void)
> +{
> +    int count, i;
> +
> +    /*
> +     * 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);

Wrong indentation.

> +
> +    if ( count < 0 )
> +    {
> +        printk("Error parsing GIC CPU interface entry\n");
> +        return;
> +    }

What about the case where count == 0?

> +
> +    if ( !bootcpu_valid )
> +    {
> +        printk("MADT missing boot CPU MPIDR, not enabling secondaries\n");
> +        return;
> +    }
> +
> +    for ( i = 0; i < enabled_cpus; i++ )
> +        cpumask_set_cpu(i, &cpu_possible_map);
> +
> +    /* Make boot-up look pretty */
> +    printk("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus);
> +}
>  
>  static int __init acpi_parse_fadt(struct acpi_table_header *table)
>  {
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index 7928f69..c3d9e00 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c

All the changes within this file and arm/psci.c is not mentioned in the
commit message.

As it's related to this patch, it would be better to have a separate
patch for it.

> @@ -7,6 +7,7 @@
>  #include <xen/vmap.h>
>  #include <asm/io.h>
>  #include <asm/psci.h>
> +#include <asm/acpi.h>
>  
>  struct smp_enable_ops {
>          int             (*prepare_cpu)(int);
> @@ -94,9 +95,13 @@ static int __init dt_arch_cpu_init(int cpu, struct dt_device_node *dn)
>      return 0;
>  }
>  
> +/* acpi only supports psci at present */

This comment should be in the else part below.

>  int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
>  {
> -    return dt_arch_cpu_init(cpu, dn);
> +    if( acpi_disabled )
> +        return dt_arch_cpu_init(cpu, dn);
> +    else
> +        return smp_psci_init(cpu);
>  }
>  
>  int __init arch_cpu_up(int cpu)
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 4066309..20040aa 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -22,6 +22,7 @@
>  #include <xen/mm.h>
>  #include <xen/smp.h>
>  #include <asm/psci.h>
> +#include <asm/acpi.h>
>  
>  uint32_t psci_ver;
>  
> @@ -74,6 +75,9 @@ int __init psci_init_0_1(void)
>      int ret;
>      const struct dt_device_node *psci;
>  
> +    if ( !acpi_disabled )
> +        return -EINVAL;
> +
>      psci = dt_find_compatible_node(NULL, NULL, "arm,psci");
>      if ( !psci )
>          return -EOPNOTSUPP;
> @@ -98,6 +102,9 @@ int __init psci_init_0_1(void)
>  int __init psci_init_0_2(void)
>  {
>      int ret;
> +
> +    if( acpi_disabled )
> +    {
>      const struct dt_device_node *psci;

The block within the if should be indented correctly. Although, why
didn't you refactor the code as you did on some other part?

>      psci = dt_find_compatible_node(NULL, NULL, "arm,psci-0.2");
> @@ -107,6 +114,15 @@ int __init psci_init_0_2(void)
>      ret = psci_is_smc_method(psci);
>      if ( ret )
>          return -EINVAL;
> +    }
> +    else
> +    {
> +        if( !acpi_psci_present() )
> +            return -EOPNOTSUPP;

This should be done in psci_init and not here.

> +
> +        if ( acpi_psci_hvc_present() )
> +            return -EINVAL;
> +    }
>  
>      psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
>  
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 90f9ef2..1385dbe 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -31,6 +31,7 @@
>  #include <xen/console.h>
>  #include <asm/gic.h>
>  #include <asm/psci.h>
> +#include <asm/acpi.h>
>  
>  cpumask_t cpu_online_map;
>  cpumask_t cpu_present_map;
> @@ -246,7 +247,11 @@ void __init smp_init_cpus(void)
>          return;
>      }
>  
> -    dt_smp_init_cpus();
> +    if ( acpi_disabled )
> +        dt_smp_init_cpus();
> +    else
> +        acpi_init_cpus();
> +
>  }
>  
>  int __init
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 4a6cb37..0845f14 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -35,9 +35,11 @@ extern bool_t acpi_disabled;
>  bool_t acpi_psci_present(void);
>  /* 1 to indicate HVC is present instead of SMC as the PSCI conduit */
>  bool_t acpi_psci_hvc_present(void);
> +void __init acpi_init_cpus(void);
>  #else
>  static inline bool_t acpi_psci_present(void) { return false; }
>  static inline bool_t acpi_psci_hvc_present(void) {return false; }
> +static inline void acpi_init_cpus(void) { }
>  #endif /* CONFIG_ACPI */
>  
>  /* Basic configuration for ACPI */
> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
> index afe2dca..ee6a5ea 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -40,6 +40,10 @@
>  #define ACPI_MADT_GET_POLARITY(inti)	ACPI_MADT_GET_(POLARITY, inti)
>  #define ACPI_MADT_GET_TRIGGER(inti)	ACPI_MADT_GET_(TRIGGER, inti)
>  
> +#define BAD_MADT_ENTRY(entry, end) (                                        \
> +                (!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
> +                ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
> +
>  #ifdef CONFIG_ACPI_BOOT
>  
>  enum acpi_interrupt_id {
> @@ -57,6 +61,7 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co
>  
>  void __iomem *
>  acpi_os_map_iomem(acpi_physical_address phys, acpi_size size);
> +

Spurious change

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


Regards,

-- 
Julien Grall