|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |