| Message ID | 1389961514-13562-11-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/01/14 12:25, Hanjun Guo wrote: > When boot the kernel with MADT, the cpu possible and present sets should > be enumerated for later smp initialization. > > The logic cpu id maps to APIC id (GIC id) is also implemented, it is > needed for acpi processor drivers. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > arch/arm64/include/asm/acpi.h | 7 ++-- > drivers/acpi/plat/arm-core.c | 83 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 87 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index c335c6d..7edd39e 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -76,9 +76,6 @@ static inline void acpi_disable_pci(void) > /* FIXME: this function should be moved to topology.h when it's ready */ > void arch_fix_phys_package_id(int num, u32 slot); > > -/* temperally define -1 to make acpi core compilerable */ > -#define cpu_physical_id(cpu) -1 > - > /* Low-level suspend routine. */ > extern int (*acpi_suspend_lowlevel)(void); > #define acpi_wakeup_address (0) > @@ -86,6 +83,10 @@ extern int (*acpi_suspend_lowlevel)(void); > #define MAX_GIC_CPU_INTERFACE 256 > #define MAX_GIC_DISTRIBUTOR 1 /* should be the same as MAX_GIC_NR */ > > +/* map logic cpu id to physical GIC id */ > +extern int arm_cpu_to_apicid[NR_CPUS]; > +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu] > + You refer arm_cpu_to_apicid as a mapping from logical cpu id to GIC id, then why is it assigned to cpu_physical_id. cpu_physical_id must be same as cpu_logical_map which is from logical cpu id to MPIDRs. [...] > @@ -321,6 +401,9 @@ int __init early_acpi_boot_init(void) > if (acpi_disabled) > return -ENODEV; > > + /* Get the boot CPU's GIC cpu interface id before MADT parsing */ > + boot_cpu_apic_id = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; > + Code contradicts your comment. You need GIC cpu interface ID(referred as GIC ID in the MADT), but you are assigning the MPIDR(physical CPU ID on ARM) to boot_cpu_apic_id. I think you are assuming GIC ID and MPIDR to be same which is wrong. With ACPI5.0, IIUC the only possible way to manage mapping from GIC CPU interface to MPIDR is to assume(in a way enforce on ARM) ACPI Processor UID in MADT and CPU Device definition match MPIDR. Regards, Sudeep
On 2014?01?18? 01:37, Sudeep Holla wrote: > On 17/01/14 12:25, Hanjun Guo wrote: >> When boot the kernel with MADT, the cpu possible and present sets should >> be enumerated for later smp initialization. >> >> The logic cpu id maps to APIC id (GIC id) is also implemented, it is >> needed for acpi processor drivers. >> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> arch/arm64/include/asm/acpi.h | 7 ++-- >> drivers/acpi/plat/arm-core.c | 83 +++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 87 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >> index c335c6d..7edd39e 100644 >> --- a/arch/arm64/include/asm/acpi.h >> +++ b/arch/arm64/include/asm/acpi.h >> @@ -76,9 +76,6 @@ static inline void acpi_disable_pci(void) >> /* FIXME: this function should be moved to topology.h when it's ready */ >> void arch_fix_phys_package_id(int num, u32 slot); >> >> -/* temperally define -1 to make acpi core compilerable */ >> -#define cpu_physical_id(cpu) -1 >> - >> /* Low-level suspend routine. */ >> extern int (*acpi_suspend_lowlevel)(void); >> #define acpi_wakeup_address (0) >> @@ -86,6 +83,10 @@ extern int (*acpi_suspend_lowlevel)(void); >> #define MAX_GIC_CPU_INTERFACE 256 >> #define MAX_GIC_DISTRIBUTOR 1 /* should be the same as MAX_GIC_NR */ >> >> +/* map logic cpu id to physical GIC id */ >> +extern int arm_cpu_to_apicid[NR_CPUS]; >> +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu] >> + > You refer arm_cpu_to_apicid as a mapping from logical cpu id to GIC id, > then why is it assigned to cpu_physical_id. cpu_physical_id must be same as > cpu_logical_map which is from logical cpu id to MPIDRs. > > [...] > >> @@ -321,6 +401,9 @@ int __init early_acpi_boot_init(void) >> if (acpi_disabled) >> return -ENODEV; >> >> + /* Get the boot CPU's GIC cpu interface id before MADT parsing */ >> + boot_cpu_apic_id = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; >> + > Code contradicts your comment. You need GIC cpu interface ID(referred as GIC ID > in the MADT), but you are assigning the MPIDR(physical CPU ID on ARM) to > boot_cpu_apic_id. I think you are assuming GIC ID and MPIDR to be same which is > wrong. > > With ACPI5.0, IIUC the only possible way to manage mapping from GIC CPU > interface to MPIDR is to assume(in a way enforce on ARM) ACPI Processor UID in > MADT and CPU Device definition match MPIDR. yes, agree with you. I mixed up the GIC Id and UID, as you said, UID can be MPIDR, and then use as hardware id to map to the logical id, it is not the same as x86 and ia64, will update this patch. Thanks Hanjun
On Fri, Jan 17, 2014 at 12:25:04PM +0000, Hanjun Guo wrote: [...] > +/* map logic cpu id to physical GIC id */ > +extern int arm_cpu_to_apicid[NR_CPUS]; > +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu] Sudeep already commented on this, please update it accordingly. > + > #else /* !CONFIG_ACPI */ > #define acpi_disabled 1 /* ACPI sometimes enabled on ARM */ > #define acpi_noirq 1 /* ACPI sometimes enabled on ARM */ > diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c > index 8ba3e6f..1d9b789 100644 > --- a/drivers/acpi/plat/arm-core.c > +++ b/drivers/acpi/plat/arm-core.c > @@ -31,6 +31,7 @@ > #include <linux/smp.h> > > #include <asm/pgtable.h> > +#include <asm/cputype.h> > > /* > * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this > @@ -52,6 +53,13 @@ EXPORT_SYMBOL(acpi_pci_disabled); > */ > static u64 acpi_lapic_addr __initdata; Is this variable actually needed ? > > +/* available_cpus here means enabled cpu in MADT */ > +static int available_cpus; Ditto. > + > +/* Map logic cpu id to physical GIC id (physical CPU id). */ > +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; > +static int boot_cpu_apic_id = -1; Do we need all these variables ? I think we should reuse cpu_logical_map data structures for that, it looks suspiciously familiar. > #define BAD_MADT_ENTRY(entry, end) ( \ > (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ > ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) > @@ -132,6 +140,55 @@ static int __init acpi_parse_madt(struct acpi_table_header *table) > * Please refer to chapter5.2.12.14/15 of ACPI 5.0 > */ > > +/** > + * acpi_register_gic_cpu_interface - register a gic cpu interface and > + * generates a logic cpu number > + * @id: gic cpu interface id to register > + * @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(int id, u8 enabled) > +{ > + int cpu; > + > + if (id >= MAX_GIC_CPU_INTERFACE) { > + pr_info(PREFIX "skipped apicid that is too big\n"); > + return -EINVAL; > + } > + > + total_cpus++; > + if (!enabled) > + return -EINVAL; > + > + if (available_cpus >= NR_CPUS) { > + pr_warn(PREFIX "NR_CPUS limit of %d reached," > + " Processor %d/0x%x ignored.\n", NR_CPUS, total_cpus, id); > + return -EINVAL; > + } Hmm...what if you are missing the boot CPU ? It is a worthy check. Have a look at smp_init_cpus(), it does not bail out on cpu>= NR_CPUS because you do want to make sure that the DT contains the boot CPU node. Same logic applies. > + > + available_cpus++; > + Is available_cpus != num_possible_cpus() ? It does not look like hence available_cpus can go. > + /* allocate a logic cpu id for the new comer */ > + if (boot_cpu_apic_id == id) { > + /* > + * boot_cpu_init() already hold bit 0 in cpu_present_mask > + * for BSP, no need to allocte again. > + */ > + cpu = 0; > + } else { > + cpu = cpumask_next_zero(-1, cpu_present_mask); > + } > + > + /* map the logic cpu id to APIC id */ > + arm_cpu_to_apicid[cpu] = id; > + > + set_cpu_present(cpu, true); > + set_cpu_possible(cpu, true); This is getting nasty. Before adding this patch and previous ones we need to put in place a method for the kernel to make a definite choice between ACPI and DT and stick to that. We can't initialize the logical map twice (which will happen if your DT has valid cpu nodes and a chosen node pointing to proper ACPI tables) or even having some entries initialized from DT and others by ACPI. It is a big fat no-no, please update the series accordingly. > + > + return cpu; > +} > + > static int __init > acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) > { > @@ -144,6 +201,16 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) > > acpi_table_print_madt_entry(header); > > + /* > + * We need to register disabled CPU as well to permit > + * counting disabled CPUs. This allows us to size > + * cpus_possible_map more accurately, to permit > + * to not preallocating memory for all NR_CPUS > + * when we use CPU hotplug. > + */ > + acpi_register_gic_cpu_interface(processor->gic_id, > + processor->flags & ACPI_MADT_ENABLED); > + > return 0; > } > > @@ -186,6 +253,19 @@ static int __init acpi_parse_madt_gic_entries(void) > return count; > } > > +#ifdef CONFIG_SMP > + if (available_cpus == 0) { > + pr_info(PREFIX "Found 0 CPUs; assuming 1\n"); > + arm_cpu_to_apicid[available_cpus] = > + read_cpuid_mpidr() & MPIDR_HWID_BITMASK; > + available_cpus = 1; /* We've got at least one of these */ > + } I'd rather check the MADT for at least the boot cpu to present, if it is not ACPI tables are horribly buggy and the kernel should barf on that. > +#endif > + > + /* Make boot-up look pretty */ > + pr_info("%d CPUs available, %d CPUs total\n", available_cpus, > + total_cpus); Ok, now, how can we use the "disabled" CPUs == (total_cpus - available_cpus) ? Are we keeping track of them in the kernel at all ? It does not look like, so I wonder whether we need this bit of info. I do not see why it is pretty to know that there are disabled, aka unusable CPUs. > return 0; > } > > @@ -321,6 +401,9 @@ int __init early_acpi_boot_init(void) > if (acpi_disabled) > return -ENODEV; > > + /* Get the boot CPU's GIC cpu interface id before MADT parsing */ > + boot_cpu_apic_id = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; See Sudeep's comment. Thanks, Lorenzo
Hi Lorenzo, On 2014?01?22? 23:53, Lorenzo Pieralisi wrote: > On Fri, Jan 17, 2014 at 12:25:04PM +0000, Hanjun Guo wrote: > > [...] > >> +/* map logic cpu id to physical GIC id */ >> +extern int arm_cpu_to_apicid[NR_CPUS]; >> +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu] > Sudeep already commented on this, please update it accordingly. Actually after some careful review of the ACPI code, I can't update it as MPIDR here. MPIDR can be the ACPI uid and NOT the GIC id, the mapping of them are something like this in ACPI driver now: logic cpu id <---> APIC Id (GIC ID) <---> ACPI uid (MPIDR on ARM) but not logic cpu id <---> ACPI uid directly, you can refer to the code of processor_core.c So here I can only map GIC id to logical cpu id. > >> + >> #else /* !CONFIG_ACPI */ >> #define acpi_disabled 1 /* ACPI sometimes enabled on ARM */ >> #define acpi_noirq 1 /* ACPI sometimes enabled on ARM */ >> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c >> index 8ba3e6f..1d9b789 100644 >> --- a/drivers/acpi/plat/arm-core.c >> +++ b/drivers/acpi/plat/arm-core.c >> @@ -31,6 +31,7 @@ >> #include <linux/smp.h> >> >> #include <asm/pgtable.h> >> +#include <asm/cputype.h> >> >> /* >> * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this >> @@ -52,6 +53,13 @@ EXPORT_SYMBOL(acpi_pci_disabled); >> */ >> static u64 acpi_lapic_addr __initdata; > Is this variable actually needed ? Yes, needed for GIC initialization. > >> >> +/* available_cpus here means enabled cpu in MADT */ >> +static int available_cpus; > Ditto. > >> + >> +/* Map logic cpu id to physical GIC id (physical CPU id). */ >> +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; >> +static int boot_cpu_apic_id = -1; > Do we need all these variables ? I think we should reuse cpu_logical_map > data structures for that, it looks suspiciously familiar. MPIDR is the different part. if we use MPIDR as GIC id, i think we can reuse cpu_logical_map, but Sudeep suggested not use MPIDR as GIC id. > >> #define BAD_MADT_ENTRY(entry, end) ( \ >> (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ >> ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) >> @@ -132,6 +140,55 @@ static int __init acpi_parse_madt(struct acpi_table_header *table) >> * Please refer to chapter5.2.12.14/15 of ACPI 5.0 >> */ >> >> +/** >> + * acpi_register_gic_cpu_interface - register a gic cpu interface and >> + * generates a logic cpu number >> + * @id: gic cpu interface id to register >> + * @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(int id, u8 enabled) >> +{ >> + int cpu; >> + >> + if (id >= MAX_GIC_CPU_INTERFACE) { >> + pr_info(PREFIX "skipped apicid that is too big\n"); >> + return -EINVAL; >> + } >> + >> + total_cpus++; >> + if (!enabled) >> + return -EINVAL; >> + >> + if (available_cpus >= NR_CPUS) { >> + pr_warn(PREFIX "NR_CPUS limit of %d reached," >> + " Processor %d/0x%x ignored.\n", NR_CPUS, total_cpus, id); >> + return -EINVAL; >> + } > Hmm...what if you are missing the boot CPU ? It is a worthy check. > Have a look at smp_init_cpus(), it does not bail out on cpu>= NR_CPUS > because you do want to make sure that the DT contains the boot CPU > node. Same logic applies. Thanks, I will review he code you mentioned and find a solution for ACPI part. > >> + >> + available_cpus++; >> + > Is available_cpus != num_possible_cpus() ? It does not look like hence > available_cpus can go. No, possible cpus include available cpus and disabled cpus this is useful for ACPI based CPU hot-plug features. > >> + /* allocate a logic cpu id for the new comer */ >> + if (boot_cpu_apic_id == id) { >> + /* >> + * boot_cpu_init() already hold bit 0 in cpu_present_mask >> + * for BSP, no need to allocte again. >> + */ >> + cpu = 0; >> + } else { >> + cpu = cpumask_next_zero(-1, cpu_present_mask); >> + } >> + >> + /* map the logic cpu id to APIC id */ >> + arm_cpu_to_apicid[cpu] = id; >> + >> + set_cpu_present(cpu, true); >> + set_cpu_possible(cpu, true); > This is getting nasty. Before adding this patch and previous ones we > need to put in place a method for the kernel to make a definite choice between > ACPI and DT and stick to that. We can't initialize the logical map twice > (which will happen if your DT has valid cpu nodes and a chosen node pointing > to proper ACPI tables) or even having some entries initialized from DT and > others by ACPI. It is a big fat no-no, please update the series accordingly. really good catch here :) so the problem here is that should we use both ACPI and DT in one system? > >> + >> + return cpu; >> +} >> + >> static int __init >> acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) >> { >> @@ -144,6 +201,16 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) >> >> acpi_table_print_madt_entry(header); >> >> + /* >> + * We need to register disabled CPU as well to permit >> + * counting disabled CPUs. This allows us to size >> + * cpus_possible_map more accurately, to permit >> + * to not preallocating memory for all NR_CPUS >> + * when we use CPU hotplug. >> + */ >> + acpi_register_gic_cpu_interface(processor->gic_id, >> + processor->flags & ACPI_MADT_ENABLED); >> + >> return 0; >> } >> >> @@ -186,6 +253,19 @@ static int __init acpi_parse_madt_gic_entries(void) >> return count; >> } >> >> +#ifdef CONFIG_SMP >> + if (available_cpus == 0) { >> + pr_info(PREFIX "Found 0 CPUs; assuming 1\n"); >> + arm_cpu_to_apicid[available_cpus] = >> + read_cpuid_mpidr() & MPIDR_HWID_BITMASK; >> + available_cpus = 1; /* We've got at least one of these */ >> + } > I'd rather check the MADT for at least the boot cpu to present, if it is > not ACPI tables are horribly buggy and the kernel should barf on that. > >> +#endif >> + >> + /* Make boot-up look pretty */ >> + pr_info("%d CPUs available, %d CPUs total\n", available_cpus, >> + total_cpus); > Ok, now, how can we use the "disabled" CPUs == (total_cpus - available_cpus) ? For cpus can be hot-added later when system is running. > > Are we keeping track of them in the kernel at all ? It does not look > like, so I wonder whether we need this bit of info. I do not see why it > is pretty to know that there are disabled, aka unusable CPUs. > >> return 0; >> } >> >> @@ -321,6 +401,9 @@ int __init early_acpi_boot_init(void) >> if (acpi_disabled) >> return -ENODEV; >> >> + /* Get the boot CPU's GIC cpu interface id before MADT parsing */ >> + boot_cpu_apic_id = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; > See Sudeep's comment. I will rework this part to get the GIC cpu interface id, not the MPIDR here. Thanks Hanjun
On Fri, Jan 24, 2014 at 02:37:28PM +0000, Hanjun Guo wrote: > Hi Lorenzo, > > On 2014?01?22? 23:53, Lorenzo Pieralisi wrote: > > On Fri, Jan 17, 2014 at 12:25:04PM +0000, Hanjun Guo wrote: > > > > [...] > > > >> +/* map logic cpu id to physical GIC id */ > >> +extern int arm_cpu_to_apicid[NR_CPUS]; > >> +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu] > > Sudeep already commented on this, please update it accordingly. > > Actually after some careful review of the ACPI code, I can't > update it as MPIDR here. > > MPIDR can be the ACPI uid and NOT the GIC id, the mapping > of them are something like this in ACPI driver now: > > logic cpu id <---> APIC Id (GIC ID) <---> ACPI uid (MPIDR on ARM) > but not logic cpu id <---> ACPI uid directly, you can refer to > the code of processor_core.c > > So here I can only map GIC id to logical cpu id. On ARM platforms GIC CPU IF id is probeable, you do not need to parse it (ie it is not information that you will find in DT). Please have a look at drivers/irqchip/irq-gic.c. We have to understand what's really required and when in ACPI, or to put it differently, why cpu_physical_id(cpu) is required and at what time at boot, I will have a look on my side too. > > > >> + > >> #else /* !CONFIG_ACPI */ > >> #define acpi_disabled 1 /* ACPI sometimes enabled on ARM */ > >> #define acpi_noirq 1 /* ACPI sometimes enabled on ARM */ > >> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c > >> index 8ba3e6f..1d9b789 100644 > >> --- a/drivers/acpi/plat/arm-core.c > >> +++ b/drivers/acpi/plat/arm-core.c > >> @@ -31,6 +31,7 @@ > >> #include <linux/smp.h> > >> > >> #include <asm/pgtable.h> > >> +#include <asm/cputype.h> > >> > >> /* > >> * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this > >> @@ -52,6 +53,13 @@ EXPORT_SYMBOL(acpi_pci_disabled); > >> */ > >> static u64 acpi_lapic_addr __initdata; > > Is this variable actually needed ? > > Yes, needed for GIC initialization. > > > > >> > >> +/* available_cpus here means enabled cpu in MADT */ > >> +static int available_cpus; > > Ditto. > > > >> + > >> +/* Map logic cpu id to physical GIC id (physical CPU id). */ > >> +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; > >> +static int boot_cpu_apic_id = -1; > > Do we need all these variables ? I think we should reuse cpu_logical_map > > data structures for that, it looks suspiciously familiar. > > MPIDR is the different part. if we use MPIDR as GIC id, i think > we can reuse cpu_logical_map, but Sudeep suggested not > use MPIDR as GIC id. It is not about *reusing* cpu_logical_map, it is about setting it up properly. cpu_logical_map must be initialized by ACPI for the spin table method to work properly (and PSCI too). And yes, cpu_physical_id(cpu) is expected to be the GIC CPU IF id on ARM, at least it looks like, I had a look too. But this does not change anything as far as cpu_logical_map is concerned, it must contain a list of MPIDRs in the system and must be retrieved via ACPI, not DT CPU nodes when ACPI is used for booting. I will have a further look, since this discrepancy is annoying. [...] > >> + > >> + available_cpus++; > >> + > > Is available_cpus != num_possible_cpus() ? It does not look like hence > > available_cpus can go. > > No, possible cpus include available cpus and disabled cpus > this is useful for ACPI based CPU hot-plug features. > > > > >> + /* allocate a logic cpu id for the new comer */ > >> + if (boot_cpu_apic_id == id) { > >> + /* > >> + * boot_cpu_init() already hold bit 0 in cpu_present_mask > >> + * for BSP, no need to allocte again. > >> + */ > >> + cpu = 0; > >> + } else { > >> + cpu = cpumask_next_zero(-1, cpu_present_mask); > >> + } > >> + > >> + /* map the logic cpu id to APIC id */ > >> + arm_cpu_to_apicid[cpu] = id; > >> + > >> + set_cpu_present(cpu, true); > >> + set_cpu_possible(cpu, true); > > This is getting nasty. Before adding this patch and previous ones we > > need to put in place a method for the kernel to make a definite choice between > > ACPI and DT and stick to that. We can't initialize the logical map twice > > (which will happen if your DT has valid cpu nodes and a chosen node pointing > > to proper ACPI tables) or even having some entries initialized from DT and > > others by ACPI. It is a big fat no-no, please update the series accordingly. > > really good catch here :) > so the problem here is that should we use both ACPI and DT in one system? > > > > > >> + > >> + return cpu; > >> +} > >> + > >> static int __init > >> acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) > >> { > >> @@ -144,6 +201,16 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) > >> > >> acpi_table_print_madt_entry(header); > >> > >> + /* > >> + * We need to register disabled CPU as well to permit > >> + * counting disabled CPUs. This allows us to size > >> + * cpus_possible_map more accurately, to permit > >> + * to not preallocating memory for all NR_CPUS > >> + * when we use CPU hotplug. > >> + */ > >> + acpi_register_gic_cpu_interface(processor->gic_id, > >> + processor->flags & ACPI_MADT_ENABLED); > >> + > >> return 0; > >> } > >> > >> @@ -186,6 +253,19 @@ static int __init acpi_parse_madt_gic_entries(void) > >> return count; > >> } > >> > >> +#ifdef CONFIG_SMP > >> + if (available_cpus == 0) { > >> + pr_info(PREFIX "Found 0 CPUs; assuming 1\n"); > >> + arm_cpu_to_apicid[available_cpus] = > >> + read_cpuid_mpidr() & MPIDR_HWID_BITMASK; > >> + available_cpus = 1; /* We've got at least one of these */ > >> + } > > I'd rather check the MADT for at least the boot cpu to present, if it is > > not ACPI tables are horribly buggy and the kernel should barf on that. > > > >> +#endif > >> + > >> + /* Make boot-up look pretty */ > >> + pr_info("%d CPUs available, %d CPUs total\n", available_cpus, > >> + total_cpus); > > Ok, now, how can we use the "disabled" CPUs == (total_cpus - available_cpus) ? > > For cpus can be hot-added later when system is running. I do not see any usage in the patchset and certainly those variables are not used in this patch, apart from printing messages whose usefulness is debatable. If, as you say, you are using those variables for something else, please add code in the patch where they are introduced for it to be self-contained and to simplify the review. Thanks, Lorenzo
On 2014?01?24? 23:35, Lorenzo Pieralisi wrote: > On Fri, Jan 24, 2014 at 02:37:28PM +0000, Hanjun Guo wrote: >> Hi Lorenzo, >> >> On 2014?01?22? 23:53, Lorenzo Pieralisi wrote: >>> On Fri, Jan 17, 2014 at 12:25:04PM +0000, Hanjun Guo wrote: >>> >>> [...] >>> >>>> +/* map logic cpu id to physical GIC id */ >>>> +extern int arm_cpu_to_apicid[NR_CPUS]; >>>> +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu] >>> Sudeep already commented on this, please update it accordingly. >> Actually after some careful review of the ACPI code, I can't >> update it as MPIDR here. >> >> MPIDR can be the ACPI uid and NOT the GIC id, the mapping >> of them are something like this in ACPI driver now: >> >> logic cpu id <---> APIC Id (GIC ID) <---> ACPI uid (MPIDR on ARM) >> but not logic cpu id <---> ACPI uid directly, you can refer to >> the code of processor_core.c >> >> So here I can only map GIC id to logical cpu id. > On ARM platforms GIC CPU IF id is probeable, you do not need to parse > it (ie it is not information that you will find in DT). Please have a look > at drivers/irqchip/irq-gic.c. > > We have to understand what's really required and when in ACPI, or to put it > differently, why cpu_physical_id(cpu) is required and at what time at > boot, I will have a look on my side too. Me too :) > >>>> + >>>> #else /* !CONFIG_ACPI */ >>>> #define acpi_disabled 1 /* ACPI sometimes enabled on ARM */ >>>> #define acpi_noirq 1 /* ACPI sometimes enabled on ARM */ >>>> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c >>>> index 8ba3e6f..1d9b789 100644 >>>> --- a/drivers/acpi/plat/arm-core.c >>>> +++ b/drivers/acpi/plat/arm-core.c >>>> @@ -31,6 +31,7 @@ >>>> #include <linux/smp.h> >>>> >>>> #include <asm/pgtable.h> >>>> +#include <asm/cputype.h> >>>> >>>> /* >>>> * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this >>>> @@ -52,6 +53,13 @@ EXPORT_SYMBOL(acpi_pci_disabled); >>>> */ >>>> static u64 acpi_lapic_addr __initdata; >>> Is this variable actually needed ? >> Yes, needed for GIC initialization. >> >>>> >>>> +/* available_cpus here means enabled cpu in MADT */ >>>> +static int available_cpus; >>> Ditto. >>> >>>> + >>>> +/* Map logic cpu id to physical GIC id (physical CPU id). */ >>>> +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; >>>> +static int boot_cpu_apic_id = -1; >>> Do we need all these variables ? I think we should reuse cpu_logical_map >>> data structures for that, it looks suspiciously familiar. >> MPIDR is the different part. if we use MPIDR as GIC id, i think >> we can reuse cpu_logical_map, but Sudeep suggested not >> use MPIDR as GIC id. > It is not about *reusing* cpu_logical_map, it is about setting it up > properly. cpu_logical_map must be initialized by ACPI for the spin table > method to work properly (and PSCI too). > > And yes, cpu_physical_id(cpu) is expected to be the GIC CPU IF id on > ARM, at least it looks like, I had a look too. But this does not change > anything as far as cpu_logical_map is concerned, it must contain a list > of MPIDRs in the system and must be retrieved via ACPI, not DT CPU nodes > when ACPI is used for booting. > > I will have a further look, since this discrepancy is annoying. Thank you for doing this, I will look that too. > > [...] > >>>> + >>>> + available_cpus++; >>>> + >>> Is available_cpus != num_possible_cpus() ? It does not look like hence >>> available_cpus can go. >> No, possible cpus include available cpus and disabled cpus >> this is useful for ACPI based CPU hot-plug features. >> >>>> + /* allocate a logic cpu id for the new comer */ >>>> + if (boot_cpu_apic_id == id) { >>>> + /* >>>> + * boot_cpu_init() already hold bit 0 in cpu_present_mask >>>> + * for BSP, no need to allocte again. >>>> + */ >>>> + cpu = 0; >>>> + } else { >>>> + cpu = cpumask_next_zero(-1, cpu_present_mask); >>>> + } >>>> + >>>> + /* map the logic cpu id to APIC id */ >>>> + arm_cpu_to_apicid[cpu] = id; >>>> + >>>> + set_cpu_present(cpu, true); >>>> + set_cpu_possible(cpu, true); >>> This is getting nasty. Before adding this patch and previous ones we >>> need to put in place a method for the kernel to make a definite choice between >>> ACPI and DT and stick to that. We can't initialize the logical map twice >>> (which will happen if your DT has valid cpu nodes and a chosen node pointing >>> to proper ACPI tables) or even having some entries initialized from DT and >>> others by ACPI. It is a big fat no-no, please update the series accordingly. >> really good catch here :) >> so the problem here is that should we use both ACPI and DT in one system? I think Mark had a clear answer about this :) (Answer for my self) >> >> >>>> + >>>> + return cpu; >>>> +} >>>> + >>>> static int __init >>>> acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) >>>> { >>>> @@ -144,6 +201,16 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) >>>> >>>> acpi_table_print_madt_entry(header); >>>> >>>> + /* >>>> + * We need to register disabled CPU as well to permit >>>> + * counting disabled CPUs. This allows us to size >>>> + * cpus_possible_map more accurately, to permit >>>> + * to not preallocating memory for all NR_CPUS >>>> + * when we use CPU hotplug. >>>> + */ >>>> + acpi_register_gic_cpu_interface(processor->gic_id, >>>> + processor->flags & ACPI_MADT_ENABLED); >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -186,6 +253,19 @@ static int __init acpi_parse_madt_gic_entries(void) >>>> return count; >>>> } >>>> >>>> +#ifdef CONFIG_SMP >>>> + if (available_cpus == 0) { >>>> + pr_info(PREFIX "Found 0 CPUs; assuming 1\n"); >>>> + arm_cpu_to_apicid[available_cpus] = >>>> + read_cpuid_mpidr() & MPIDR_HWID_BITMASK; >>>> + available_cpus = 1; /* We've got at least one of these */ >>>> + } >>> I'd rather check the MADT for at least the boot cpu to present, if it is >>> not ACPI tables are horribly buggy and the kernel should barf on that. >>> >>>> +#endif >>>> + >>>> + /* Make boot-up look pretty */ >>>> + pr_info("%d CPUs available, %d CPUs total\n", available_cpus, >>>> + total_cpus); >>> Ok, now, how can we use the "disabled" CPUs == (total_cpus - available_cpus) ? >> For cpus can be hot-added later when system is running. > I do not see any usage in the patchset and certainly those variables are > not used in this patch, apart from printing messages whose usefulness is > debatable. If, as you say, you are using those variables for something > else, please add code in the patch where they are introduced for it to be > self-contained and to simplify the review. ah, yes. although my ACPI based CPU hot-plug patch is ready, but need this patch set goes in first and then send them out. I agree with you, will try to update this patch. Thanks Hanjun
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index c335c6d..7edd39e 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -76,9 +76,6 @@ static inline void acpi_disable_pci(void) /* FIXME: this function should be moved to topology.h when it's ready */ void arch_fix_phys_package_id(int num, u32 slot); -/* temperally define -1 to make acpi core compilerable */ -#define cpu_physical_id(cpu) -1 - /* Low-level suspend routine. */ extern int (*acpi_suspend_lowlevel)(void); #define acpi_wakeup_address (0) @@ -86,6 +83,10 @@ extern int (*acpi_suspend_lowlevel)(void); #define MAX_GIC_CPU_INTERFACE 256 #define MAX_GIC_DISTRIBUTOR 1 /* should be the same as MAX_GIC_NR */ +/* map logic cpu id to physical GIC id */ +extern int arm_cpu_to_apicid[NR_CPUS]; +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu] + #else /* !CONFIG_ACPI */ #define acpi_disabled 1 /* ACPI sometimes enabled on ARM */ #define acpi_noirq 1 /* ACPI sometimes enabled on ARM */ diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c index 8ba3e6f..1d9b789 100644 --- a/drivers/acpi/plat/arm-core.c +++ b/drivers/acpi/plat/arm-core.c @@ -31,6 +31,7 @@ #include <linux/smp.h> #include <asm/pgtable.h> +#include <asm/cputype.h> /* * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this @@ -52,6 +53,13 @@ EXPORT_SYMBOL(acpi_pci_disabled); */ static u64 acpi_lapic_addr __initdata; +/* available_cpus here means enabled cpu in MADT */ +static int available_cpus; + +/* Map logic cpu id to physical GIC id (physical CPU id). */ +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; +static int boot_cpu_apic_id = -1; + #define BAD_MADT_ENTRY(entry, end) ( \ (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) @@ -132,6 +140,55 @@ static int __init acpi_parse_madt(struct acpi_table_header *table) * Please refer to chapter5.2.12.14/15 of ACPI 5.0 */ +/** + * acpi_register_gic_cpu_interface - register a gic cpu interface and + * generates a logic cpu number + * @id: gic cpu interface id to register + * @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(int id, u8 enabled) +{ + int cpu; + + if (id >= MAX_GIC_CPU_INTERFACE) { + pr_info(PREFIX "skipped apicid that is too big\n"); + return -EINVAL; + } + + total_cpus++; + if (!enabled) + return -EINVAL; + + if (available_cpus >= NR_CPUS) { + pr_warn(PREFIX "NR_CPUS limit of %d reached," + " Processor %d/0x%x ignored.\n", NR_CPUS, total_cpus, id); + return -EINVAL; + } + + available_cpus++; + + /* allocate a logic cpu id for the new comer */ + if (boot_cpu_apic_id == id) { + /* + * boot_cpu_init() already hold bit 0 in cpu_present_mask + * for BSP, no need to allocte again. + */ + cpu = 0; + } else { + cpu = cpumask_next_zero(-1, cpu_present_mask); + } + + /* map the logic cpu id to APIC id */ + arm_cpu_to_apicid[cpu] = id; + + set_cpu_present(cpu, true); + set_cpu_possible(cpu, true); + + return cpu; +} + static int __init acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) { @@ -144,6 +201,16 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) acpi_table_print_madt_entry(header); + /* + * We need to register disabled CPU as well to permit + * counting disabled CPUs. This allows us to size + * cpus_possible_map more accurately, to permit + * to not preallocating memory for all NR_CPUS + * when we use CPU hotplug. + */ + acpi_register_gic_cpu_interface(processor->gic_id, + processor->flags & ACPI_MADT_ENABLED); + return 0; } @@ -186,6 +253,19 @@ static int __init acpi_parse_madt_gic_entries(void) return count; } +#ifdef CONFIG_SMP + if (available_cpus == 0) { + pr_info(PREFIX "Found 0 CPUs; assuming 1\n"); + arm_cpu_to_apicid[available_cpus] = + read_cpuid_mpidr() & MPIDR_HWID_BITMASK; + available_cpus = 1; /* We've got at least one of these */ + } +#endif + + /* Make boot-up look pretty */ + pr_info("%d CPUs available, %d CPUs total\n", available_cpus, + total_cpus); + return 0; } @@ -321,6 +401,9 @@ int __init early_acpi_boot_init(void) if (acpi_disabled) return -ENODEV; + /* Get the boot CPU's GIC cpu interface id before MADT parsing */ + boot_cpu_apic_id = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; + /* * Process the Multiple APIC Description Table (MADT), if present */
When boot the kernel with MADT, the cpu possible and present sets should be enumerated for later smp initialization. The logic cpu id maps to APIC id (GIC id) is also implemented, it is needed for acpi processor drivers. Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> --- arch/arm64/include/asm/acpi.h | 7 ++-- drivers/acpi/plat/arm-core.c | 83 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 3 deletions(-)