[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



+shannon

On 20 May 2015 at 22:08, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
>> Signed-off-by: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
>> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
>
> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.