From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH RFC 11/35] ARM64 / ACPI: Parse MADT to map logical cpu to MPIDR and get cpu_possible/present_map Date: Wed, 04 Feb 2015 21:44:56 +0000 Message-ID: <54D292D8.9080808@linaro.org> References: <1423058539-26403-1-git-send-email-parth.dixit@linaro.org> <1423058539-26403-12-git-send-email-parth.dixit@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1423058539-26403-12-git-send-email-parth.dixit@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: parth.dixit@linaro.org, xen-devel@lists.xen.org Cc: ian.campbell@citrix.com, Naresh Bhat , tim@xen.org, Tomasz Nowicki , stefano.stabellini@citrix.com, Hanjun Guo , jbeulich@suse.com, christoffer.dall@linaro.org List-Id: xen-devel@lists.xenproject.org Hi Parth, On 04/02/2015 14:01, parth.dixit@linaro.org wrote: > From: Naresh Bhat > > 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 and > cpu_present_map. > > Signed-off-by: Hanjun Guo > Signed-off-by: Tomasz Nowicki > Signed-off-by: Naresh Bhat > --- > xen/arch/arm/arm64/acpi/arm-core.c | 139 +++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/acpi.h | 2 + > xen/include/xen/acpi.h | 5 ++ > 3 files changed, 146 insertions(+) > > diff --git a/xen/arch/arm/arm64/acpi/arm-core.c b/xen/arch/arm/arm64/acpi/arm-core.c > index 2b7e2ef..84b0032 100644 > --- a/xen/arch/arm/arm64/acpi/arm-core.c > +++ b/xen/arch/arm/arm64/acpi/arm-core.c > @@ -26,7 +26,10 @@ > #include > #include > #include > +#include > +#include > > +#include > #include > > /* > @@ -49,10 +52,141 @@ int acpi_psci_present; > /* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */ > int acpi_psci_use_hvc; > > +/* available_cpus means enabled cpu in MADT */ > +static int available_cpus; > + Why adding available_cpus here, and total_cpus a bit below? > enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM; > > struct acpi_arm_root acpi_arm_rsdp_info; /* info about RSDP from FDT */ > > +/* arch-optional setting to enable display of offline cpus >= nr_cpu_ids */ > +unsigned int total_cpus = 0; Set but never used, neither exported... > +/* > + * acpi_register_gic_cpu_interface - register a gic cpu interface and > + * generates a logic cpu number > + * @mpidr: CPU's hardware id to register, MPIDR represented in MADT > + * @enabled: this cpu is enabled or not > + * > + * Returns the logic cpu number which maps to the gic cpu interface > + */ > +static int acpi_register_gic_cpu_interface(u64 mpidr, u8 enabled) > +{ > + int cpu; > + > + if ( mpidr == INVALID_HWID ) > + { > + printk("Skip invalid cpu hardware ID\n"); > + return -EINVAL; > + } > + > + total_cpus++; > + if ( !enabled ) > + return -EINVAL; > + > + if ( available_cpus >= NR_CPUS ) > + { > + printk("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n", > + NR_CPUS, total_cpus, (long long unsigned int)mpidr); Why the (long long unsigned int) cast? Please use PRIx64. > + return -EINVAL; > + } > + > + /* If it is the first CPU, no need to check duplicate MPIDRs */ > + if ( !available_cpus ) > + goto skip_mpidr_check; I don't think it's necessary, for_each_present_cpu should not contain the first CPU... > + > + /* > + * Duplicate MPIDRs are a recipe for disaster. Scan > + * all initialized entries and check for > + * duplicates. If any is found just ignore the CPU. > + */ > + for_each_present_cpu(cpu) > + { > + if ( cpu_logical_map(cpu) == mpidr ) > + { > + printk("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n", > + (long long unsigned int)mpidr); Ditto for the cast. > + return -EINVAL; > + } > + } > + > +skip_mpidr_check: > + available_cpus++; > + > + /* allocate a logic cpu id for the new comer */ > + if ( cpu_logical_map(0) == mpidr ) > + { > + /* > + * boot_cpu_init() already hold bit 0 in cpu_present_mask > + * for BSP, no need to allocte again. allocate > + */ > + cpu = 0; > + } > + else > + cpu = cpumask_next_zero(-1, &cpu_present_map); > + > + /* map the logic cpu id to cpu MPIDR */ > + cpu_logical_map(cpu) = mpidr; > + > + set_cpu_possible(cpu, true); > + set_cpu_present(cpu, true); > + > + return cpu; > +} > + > +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_register_gic_cpu_interface(processor->mpidr, > + processor->flags & ACPI_MADT_ENABLED); > + > + return 0; > +} > + > +/* > + * Parse GIC cpu interface related entries in MADT > + * returns 0 on success, < 0 on error > + */ > +static int __init acpi_parse_madt_gic_cpu_interface_entries(void) > +{ > + int count; > + > + /* > + * 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, MAX_GIC_CPU_INTERFACE); > + > + if ( !count ) > + { > + printk("No GIC CPU interface entries present\n"); > + return -ENODEV; > + } > + else if ( count < 0 ) > + { > + printk("Error parsing GIC CPU interface entry\n"); > + return count; > + } > + > + /* Make boot-up look pretty */ > + printk("%d CPUs available, %d CPUs total\n", available_cpus, > + total_cpus); > + > + return 0; > +} > + > int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > { > *irq = -1; > @@ -140,6 +274,11 @@ int __init acpi_boot_init(void) > if ( err ) > printk("Can't find FADT\n"); If we don't find the FADT, we should give up rather than trying to parse the GIC ACPI table. > > + /* Get the boot CPU's MPIDR before MADT parsing */ > + cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; > + > + err = acpi_parse_madt_gic_cpu_interface_entries(); > + > return err; > } > #endif > diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h > index 03051ef..c2d25db 100644 > --- a/xen/include/asm-arm/acpi.h > +++ b/xen/include/asm-arm/acpi.h > @@ -105,4 +105,6 @@ static inline void acpi_disable_pci(void) > #define acpi_strict 1 /* no ACPI spec workarounds on ARM */ > #endif > > +#define MAX_GIC_CPU_INTERFACE 65535 > + > #endif /*_ASM_ARM_ACPI_H*/ > diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h > index ff96336..9387b36 100644 > --- a/xen/include/xen/acpi.h > +++ b/xen/include/xen/acpi.h > @@ -67,6 +67,11 @@ typedef int (*acpi_table_handler) (struct acpi_table_header *table); > typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, const unsigned long end); > > unsigned int acpi_get_processor_id (unsigned int cpu); > + > +#define BAD_MADT_ENTRY(entry, end) ( \ > + (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ > + ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) > + IHMO, this should belong to a separate patch. > char * __acpi_map_table (paddr_t phys_addr, unsigned long size); > int acpi_boot_init (void); > int acpi_boot_table_init (void); > Regards, -- Julien Grall