| Message ID | 1453540813-15764-9-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 23.01.16 at 10:20, <zhaoshenglong@huawei.com> wrote: > --- a/xen/include/xen/acpi.h > +++ b/xen/include/xen/acpi.h > @@ -39,6 +39,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)) If you move or otherwise anyway touch existing code, please always take a critical look at it and at least fix obvious problems. Read, here: Properly parenthesize all uses of the macro's parameters. While not desirable, failing to do so may be acceptable when the scope of such definitions is very limited (as it was before you moving it), but once globally exposed it needs to be made safe. Jan
On Sat, 23 Jan 2016, Shannon Zhao wrote: > From: Parth Dixit <parth.dixit@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. > > Move BAD_MADT_ENTRY to common place. > > Cc: Jan Beulich <jbeulich@suse.com> > 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> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > V4: fix coding style and address some comments > --- > xen/arch/arm/acpi/boot.c | 129 +++++++++++++++++++++++++++++++++++++++++++++ > xen/arch/x86/acpi/boot.c | 4 -- > xen/include/asm-arm/acpi.h | 2 + > xen/include/xen/acpi.h | 4 ++ > 4 files changed, 135 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c > index 6b33fbe..c9135f2 100644 > --- a/xen/arch/arm/acpi/boot.c > +++ b/xen/arch/arm/acpi/boot.c > @@ -32,6 +32,135 @@ > #include <xen/mm.h> > > #include <asm/acpi.h> > +#include <asm/smp.h> > + > +/* Processors with enabled flag and sane MPIDR */ > +static unsigned int enabled_cpus; > + > +/* Boot CPU is valid or not in MADT */ > +static bool_t __initdata bootcpu_valid; > + > +/* total number of cpus in this system */ > +static unsigned int __initdata total_cpus; > + > +/* > + * acpi_map_gic_cpu_interface - generates a logical cpu number > + * and map to MPIDR represented by GICC structure > + */ > +static void __init > +acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) > +{ > + int i; > + u64 mpidr = processor->arm_mpidr & MPIDR_HWID_MASK; > + bool_t enabled = !!(processor->flags & ACPI_MADT_ENABLED); > + > + if ( mpidr == MPIDR_INVALID ) > + { > + printk("Skip MADT cpu entry with invalid MPIDR\n"); > + return; > + } > + > + total_cpus++; > + if ( !enabled ) > + return; > + > + if ( enabled_cpus >= NR_CPUS ) > + { > + printk("NR_CPUS limit of %d reached, Processor %d/0x%"PRIx64" ignored.\n", > + NR_CPUS, total_cpus, mpidr); > + return; > + } > + > + /* 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; > + } > + > + bootcpu_valid = true; > + } cpu0 is always initialized first, right? In that case I think we should remove bootcpu_valid and turn this into a more explicit check, such as: if ( (enabled_cpus == 0) && (cpu_logical_map(0) != mpidr) ) { printk("Firmware bug, invalid CPU MPIDR for cpu0: 0x%"PRIx64" in MADT\n", mpidr); return; } > + /* > + * 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; > + } > + } Please remove bootcpu_valid and merge this loop with the previous if statement, like this: for ( i = 0; i < enabled_cpus; i++ ) { if ( cpu_logical_map(i) == mpidr ) { printk("Firmware bug... return; } } > + if ( !acpi_psci_present() ) > + return; > + > + /* CPU 0 was already initialized */ > + if ( enabled_cpus ) > + { > + if ( arch_cpu_init(enabled_cpus, NULL) < 0 ) > + return; > + > + /* map the logical cpu id to cpu MPIDR */ > + cpu_logical_map(enabled_cpus) = mpidr; > + } > + > + 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 = > + container_of(header, 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); > + return 0; > +} > + > +/* Parse GIC cpu interface entries in MADT for SMP init */ > +void __init acpi_smp_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/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c > index fac36c6..385c0be 100644 > --- a/xen/arch/x86/acpi/boot.c > +++ b/xen/arch/x86/acpi/boot.c > @@ -43,10 +43,6 @@ > #include <mach_apic.h> > #include <mach_mpparse.h> > > -#define BAD_MADT_ENTRY(entry, end) ( \ > - (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ > - ((struct acpi_subtable_header *)entry)->length != sizeof(*entry)) > - > #define PREFIX "ACPI: " > > bool_t __initdata acpi_noirq; /* skip ACPI IRQ initialization */ > diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h > index 1ce88f8..89d17e8 100644 > --- a/xen/include/asm-arm/acpi.h > +++ b/xen/include/asm-arm/acpi.h > @@ -35,9 +35,11 @@ extern bool_t acpi_disabled; > #ifdef CONFIG_ACPI > bool_t __init acpi_psci_present(void); > bool_t __init acpi_psci_hvc_present(void); > +void __init acpi_smp_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_smp_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 65e53a6..bc73310 100644 > --- a/xen/include/xen/acpi.h > +++ b/xen/include/xen/acpi.h > @@ -39,6 +39,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 { > -- > 2.0.4 > >
diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c index 6b33fbe..c9135f2 100644 --- a/xen/arch/arm/acpi/boot.c +++ b/xen/arch/arm/acpi/boot.c @@ -32,6 +32,135 @@ #include <xen/mm.h> #include <asm/acpi.h> +#include <asm/smp.h> + +/* Processors with enabled flag and sane MPIDR */ +static unsigned int enabled_cpus; + +/* Boot CPU is valid or not in MADT */ +static bool_t __initdata bootcpu_valid; + +/* total number of cpus in this system */ +static unsigned int __initdata total_cpus; + +/* + * acpi_map_gic_cpu_interface - generates a logical cpu number + * and map to MPIDR represented by GICC structure + */ +static void __init +acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) +{ + int i; + u64 mpidr = processor->arm_mpidr & MPIDR_HWID_MASK; + bool_t enabled = !!(processor->flags & ACPI_MADT_ENABLED); + + if ( mpidr == MPIDR_INVALID ) + { + printk("Skip MADT cpu entry with invalid MPIDR\n"); + return; + } + + total_cpus++; + if ( !enabled ) + return; + + if ( enabled_cpus >= NR_CPUS ) + { + printk("NR_CPUS limit of %d reached, Processor %d/0x%"PRIx64" ignored.\n", + NR_CPUS, total_cpus, mpidr); + return; + } + + /* 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; + } + + 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; + } + } + + if ( !acpi_psci_present() ) + return; + + /* CPU 0 was already initialized */ + if ( enabled_cpus ) + { + if ( arch_cpu_init(enabled_cpus, NULL) < 0 ) + return; + + /* map the logical cpu id to cpu MPIDR */ + cpu_logical_map(enabled_cpus) = mpidr; + } + + 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 = + container_of(header, 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); + return 0; +} + +/* Parse GIC cpu interface entries in MADT for SMP init */ +void __init acpi_smp_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/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c index fac36c6..385c0be 100644 --- a/xen/arch/x86/acpi/boot.c +++ b/xen/arch/x86/acpi/boot.c @@ -43,10 +43,6 @@ #include <mach_apic.h> #include <mach_mpparse.h> -#define BAD_MADT_ENTRY(entry, end) ( \ - (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ - ((struct acpi_subtable_header *)entry)->length != sizeof(*entry)) - #define PREFIX "ACPI: " bool_t __initdata acpi_noirq; /* skip ACPI IRQ initialization */ diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h index 1ce88f8..89d17e8 100644 --- a/xen/include/asm-arm/acpi.h +++ b/xen/include/asm-arm/acpi.h @@ -35,9 +35,11 @@ extern bool_t acpi_disabled; #ifdef CONFIG_ACPI bool_t __init acpi_psci_present(void); bool_t __init acpi_psci_hvc_present(void); +void __init acpi_smp_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_smp_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 65e53a6..bc73310 100644 --- a/xen/include/xen/acpi.h +++ b/xen/include/xen/acpi.h @@ -39,6 +39,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 {