| Message ID | 1431893048-5214-14-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
+shannon On 20 May 2015 at 22:08, Julien Grall <julien.grall@citrix.com> 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@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
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; + +/* + * 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 i; + + if ( mpidr == MPIDR_INVALID ) + { + printk("Skip MADT cpu entry with invalid MPIDR\n"); + return -EINVAL; + } + + total_cpus++; + if ( !enabled ) + return -EINVAL; + + 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++; + return enabled_cpus; +} + +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_map_gic_cpu_interface(processor->mpidr & MPIDR_HWID_MASK, + processor->flags & ACPI_MADT_ENABLED); + + 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); + + if ( count < 0 ) + { + printk("Error parsing GIC CPU interface entry\n"); + return; + } + + 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 @@ -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 */ 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; 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; + + 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); + 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);