| Message ID | 20150203200936.GA21731@e104818-lin.cambridge.arm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 2015?02?04? 04:09, Catalin Marinas wrote: > On Tue, Feb 03, 2015 at 02:17:49PM +0000, Mark Rutland wrote: >> On Mon, Feb 02, 2015 at 12:45:42PM +0000, Hanjun Guo wrote: >>> Introduce a new function map_gicc_mpidr() to allow MPIDRs to be obtained >>> from the GICC Structure introduced by ACPI 5.1. >>> >>> MPIDR is the CPU hardware ID as local APIC ID on x86 platform, so we use >>> MPIDR not the GIC CPU interface ID to identify CPUs. >>> >>> Further steps would typedef a phys_id_t for in arch code(with >>> appropriate size and a corresponding invalid value, say ~0) and use that >>> instead of an int in drivers/acpi/processor_core.c to store phys_id, then >>> no need for mpidr packing. >> >> I don't understand why we don't fix this now, and I'm very worried that >> this patch leaves much potential for FW bugs due to potential Linux >> bugs. >> >> Having a function called cpu_physical_id which _does not_ return a >> physical ID makes no sense to me. Any time we really need a physical >> ID, we're still going to have to unpack it (in an architecture-specific >> manner). > > Do you mean something like this? Only briefly tested on Juno and I may > have missed other calls: Thanks, I think it is Mark's suggestion (and also Lorenzo's) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index ea4d2b35c57b..4fafd62b1b86 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -49,33 +49,12 @@ static inline void enable_acpi(void) > acpi_noirq = 0; > } > > -/* MPIDR value provided in GICC structure is 64 bits, but the > - * existing phys_id (CPU hardware ID) using in acpi processor > - * driver is 32-bit, to conform to the same datatype we need > - * to repack the GICC structure MPIDR. > - * > - * bits other than following 32 bits are defined as 0, so it > - * will be no information lost after repacked. > - * > - * Bits [0:7] Aff0; > - * Bits [8:15] Aff1; > - * Bits [16:23] Aff2; > - * Bits [32:39] Aff3; > - */ > -static inline u32 pack_mpidr(u64 mpidr) > -{ > - return (u32) ((mpidr & 0xff00000000) >> 8) | mpidr; > -} > - > /* > * The ACPI processor driver for ACPI core code needs this macro > * to find out this cpu was already mapped (mapping from CPU hardware > * ID to CPU logical ID) or not. > - * > - * cpu_logical_map(cpu) is the mapping of MPIDR and the logical cpu, > - * and MPIDR is the cpu hardware ID we needed to pack. > */ > -#define cpu_physical_id(cpu) pack_mpidr(cpu_logical_map(cpu)) > +#define cpu_physical_id(cpu) cpu_logical_map(cpu) > > /* > * It's used from ACPI core in kdump to boot UP system with SMP kernel, > diff --git a/arch/arm64/include/asm/smp_plat.h b/arch/arm64/include/asm/smp_plat.h > index 59e282311b58..a492276e008d 100644 > --- a/arch/arm64/include/asm/smp_plat.h > +++ b/arch/arm64/include/asm/smp_plat.h > @@ -40,4 +40,6 @@ static inline u32 mpidr_hash_size(void) > extern u64 __cpu_logical_map[NR_CPUS]; > #define cpu_logical_map(cpu) __cpu_logical_map[cpu] > > +typedef u64 cpuid_t; I think cpuid_t is a little confused because people may recognize it as cpu logical id, its original meaning is the physical cpu ID, so how about: typedef u64 phys_id_t; ? Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 04, 2015 at 09:48:05AM +0000, Hanjun Guo wrote: > On 2015?02?04? 04:09, Catalin Marinas wrote: > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > > index ea4d2b35c57b..4fafd62b1b86 100644 > > --- a/arch/arm64/include/asm/acpi.h > > +++ b/arch/arm64/include/asm/acpi.h > > @@ -49,33 +49,12 @@ static inline void enable_acpi(void) > > acpi_noirq = 0; > > } > > > > -/* MPIDR value provided in GICC structure is 64 bits, but the > > - * existing phys_id (CPU hardware ID) using in acpi processor > > - * driver is 32-bit, to conform to the same datatype we need > > - * to repack the GICC structure MPIDR. > > - * > > - * bits other than following 32 bits are defined as 0, so it > > - * will be no information lost after repacked. > > - * > > - * Bits [0:7] Aff0; > > - * Bits [8:15] Aff1; > > - * Bits [16:23] Aff2; > > - * Bits [32:39] Aff3; > > - */ > > -static inline u32 pack_mpidr(u64 mpidr) > > -{ > > - return (u32) ((mpidr & 0xff00000000) >> 8) | mpidr; > > -} > > - > > /* > > * The ACPI processor driver for ACPI core code needs this macro > > * to find out this cpu was already mapped (mapping from CPU hardware > > * ID to CPU logical ID) or not. > > - * > > - * cpu_logical_map(cpu) is the mapping of MPIDR and the logical cpu, > > - * and MPIDR is the cpu hardware ID we needed to pack. > > */ > > -#define cpu_physical_id(cpu) pack_mpidr(cpu_logical_map(cpu)) > > +#define cpu_physical_id(cpu) cpu_logical_map(cpu) > > > > /* > > * It's used from ACPI core in kdump to boot UP system with SMP kernel, > > diff --git a/arch/arm64/include/asm/smp_plat.h b/arch/arm64/include/asm/smp_plat.h > > index 59e282311b58..a492276e008d 100644 > > --- a/arch/arm64/include/asm/smp_plat.h > > +++ b/arch/arm64/include/asm/smp_plat.h > > @@ -40,4 +40,6 @@ static inline u32 mpidr_hash_size(void) > > extern u64 __cpu_logical_map[NR_CPUS]; > > #define cpu_logical_map(cpu) __cpu_logical_map[cpu] > > > > +typedef u64 cpuid_t; > > I think cpuid_t is a little confused because people may recognize > it as cpu logical id, its original meaning is the physical cpu ID, > so how about: > > typedef u64 phys_id_t; ? I would keep "cpu" somewhere in the name as "phys" is too generic, maybe phys_cpuid_t.
On 2015?02?04? 19:21, Catalin Marinas wrote: > On Wed, Feb 04, 2015 at 09:48:05AM +0000, Hanjun Guo wrote: >> On 2015?02?04? 04:09, Catalin Marinas wrote: >>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >>> index ea4d2b35c57b..4fafd62b1b86 100644 >>> --- a/arch/arm64/include/asm/acpi.h >>> +++ b/arch/arm64/include/asm/acpi.h >>> @@ -49,33 +49,12 @@ static inline void enable_acpi(void) >>> acpi_noirq = 0; >>> } >>> >>> -/* MPIDR value provided in GICC structure is 64 bits, but the >>> - * existing phys_id (CPU hardware ID) using in acpi processor >>> - * driver is 32-bit, to conform to the same datatype we need >>> - * to repack the GICC structure MPIDR. >>> - * >>> - * bits other than following 32 bits are defined as 0, so it >>> - * will be no information lost after repacked. >>> - * >>> - * Bits [0:7] Aff0; >>> - * Bits [8:15] Aff1; >>> - * Bits [16:23] Aff2; >>> - * Bits [32:39] Aff3; >>> - */ >>> -static inline u32 pack_mpidr(u64 mpidr) >>> -{ >>> - return (u32) ((mpidr & 0xff00000000) >> 8) | mpidr; >>> -} >>> - >>> /* >>> * The ACPI processor driver for ACPI core code needs this macro >>> * to find out this cpu was already mapped (mapping from CPU hardware >>> * ID to CPU logical ID) or not. >>> - * >>> - * cpu_logical_map(cpu) is the mapping of MPIDR and the logical cpu, >>> - * and MPIDR is the cpu hardware ID we needed to pack. >>> */ >>> -#define cpu_physical_id(cpu) pack_mpidr(cpu_logical_map(cpu)) >>> +#define cpu_physical_id(cpu) cpu_logical_map(cpu) >>> >>> /* >>> * It's used from ACPI core in kdump to boot UP system with SMP kernel, >>> diff --git a/arch/arm64/include/asm/smp_plat.h b/arch/arm64/include/asm/smp_plat.h >>> index 59e282311b58..a492276e008d 100644 >>> --- a/arch/arm64/include/asm/smp_plat.h >>> +++ b/arch/arm64/include/asm/smp_plat.h >>> @@ -40,4 +40,6 @@ static inline u32 mpidr_hash_size(void) >>> extern u64 __cpu_logical_map[NR_CPUS]; >>> #define cpu_logical_map(cpu) __cpu_logical_map[cpu] >>> >>> +typedef u64 cpuid_t; >> >> I think cpuid_t is a little confused because people may recognize >> it as cpu logical id, its original meaning is the physical cpu ID, >> so how about: >> >> typedef u64 phys_id_t; ? > > I would keep "cpu" somewhere in the name as "phys" is too generic, maybe > phys_cpuid_t. This is pretty fine to me too :) x86 and IA64 use 32 bit cpu phys_id (apic_id) everywhere in the arch code, but I think I don't need to touch them in this patch. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 05, 2015 at 09:27:15AM +0000, Hanjun Guo wrote: > On 2015?02?04? 19:21, Catalin Marinas wrote: > > On Wed, Feb 04, 2015 at 09:48:05AM +0000, Hanjun Guo wrote: > >> On 2015?02?04? 04:09, Catalin Marinas wrote: > >>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > >>> index ea4d2b35c57b..4fafd62b1b86 100644 > >>> --- a/arch/arm64/include/asm/acpi.h > >>> +++ b/arch/arm64/include/asm/acpi.h > >>> @@ -49,33 +49,12 @@ static inline void enable_acpi(void) > >>> acpi_noirq = 0; > >>> } > >>> > >>> -/* MPIDR value provided in GICC structure is 64 bits, but the > >>> - * existing phys_id (CPU hardware ID) using in acpi processor > >>> - * driver is 32-bit, to conform to the same datatype we need > >>> - * to repack the GICC structure MPIDR. > >>> - * > >>> - * bits other than following 32 bits are defined as 0, so it > >>> - * will be no information lost after repacked. > >>> - * > >>> - * Bits [0:7] Aff0; > >>> - * Bits [8:15] Aff1; > >>> - * Bits [16:23] Aff2; > >>> - * Bits [32:39] Aff3; > >>> - */ > >>> -static inline u32 pack_mpidr(u64 mpidr) > >>> -{ > >>> - return (u32) ((mpidr & 0xff00000000) >> 8) | mpidr; > >>> -} > >>> - > >>> /* > >>> * The ACPI processor driver for ACPI core code needs this macro > >>> * to find out this cpu was already mapped (mapping from CPU hardware > >>> * ID to CPU logical ID) or not. > >>> - * > >>> - * cpu_logical_map(cpu) is the mapping of MPIDR and the logical cpu, > >>> - * and MPIDR is the cpu hardware ID we needed to pack. > >>> */ > >>> -#define cpu_physical_id(cpu) pack_mpidr(cpu_logical_map(cpu)) > >>> +#define cpu_physical_id(cpu) cpu_logical_map(cpu) > >>> > >>> /* > >>> * It's used from ACPI core in kdump to boot UP system with SMP kernel, > >>> diff --git a/arch/arm64/include/asm/smp_plat.h b/arch/arm64/include/asm/smp_plat.h > >>> index 59e282311b58..a492276e008d 100644 > >>> --- a/arch/arm64/include/asm/smp_plat.h > >>> +++ b/arch/arm64/include/asm/smp_plat.h > >>> @@ -40,4 +40,6 @@ static inline u32 mpidr_hash_size(void) > >>> extern u64 __cpu_logical_map[NR_CPUS]; > >>> #define cpu_logical_map(cpu) __cpu_logical_map[cpu] > >>> > >>> +typedef u64 cpuid_t; > >> > >> I think cpuid_t is a little confused because people may recognize > >> it as cpu logical id, its original meaning is the physical cpu ID, > >> so how about: > >> > >> typedef u64 phys_id_t; ? > > > > I would keep "cpu" somewhere in the name as "phys" is too generic, maybe > > phys_cpuid_t. > > This is pretty fine to me too :) > > x86 and IA64 use 32 bit cpu phys_id (apic_id) everywhere in the arch > code, but I think I don't need to touch them in this patch. No, as long as you define phys_cpuid_t to be 32-bit (I guess an int) on these architectures.
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index ea4d2b35c57b..4fafd62b1b86 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -49,33 +49,12 @@ static inline void enable_acpi(void) acpi_noirq = 0; } -/* MPIDR value provided in GICC structure is 64 bits, but the - * existing phys_id (CPU hardware ID) using in acpi processor - * driver is 32-bit, to conform to the same datatype we need - * to repack the GICC structure MPIDR. - * - * bits other than following 32 bits are defined as 0, so it - * will be no information lost after repacked. - * - * Bits [0:7] Aff0; - * Bits [8:15] Aff1; - * Bits [16:23] Aff2; - * Bits [32:39] Aff3; - */ -static inline u32 pack_mpidr(u64 mpidr) -{ - return (u32) ((mpidr & 0xff00000000) >> 8) | mpidr; -} - /* * The ACPI processor driver for ACPI core code needs this macro * to find out this cpu was already mapped (mapping from CPU hardware * ID to CPU logical ID) or not. - * - * cpu_logical_map(cpu) is the mapping of MPIDR and the logical cpu, - * and MPIDR is the cpu hardware ID we needed to pack. */ -#define cpu_physical_id(cpu) pack_mpidr(cpu_logical_map(cpu)) +#define cpu_physical_id(cpu) cpu_logical_map(cpu) /* * It's used from ACPI core in kdump to boot UP system with SMP kernel, diff --git a/arch/arm64/include/asm/smp_plat.h b/arch/arm64/include/asm/smp_plat.h index 59e282311b58..a492276e008d 100644 --- a/arch/arm64/include/asm/smp_plat.h +++ b/arch/arm64/include/asm/smp_plat.h @@ -40,4 +40,6 @@ static inline u32 mpidr_hash_size(void) extern u64 __cpu_logical_map[NR_CPUS]; #define cpu_logical_map(cpu) __cpu_logical_map[cpu] +typedef u64 cpuid_t; + #endif /* __ASM_SMP_PLAT_H */ diff --git a/arch/ia64/include/asm/smp.h b/arch/ia64/include/asm/smp.h index fea21e986022..251c6af899d6 100644 --- a/arch/ia64/include/asm/smp.h +++ b/arch/ia64/include/asm/smp.h @@ -41,6 +41,8 @@ ia64_get_lid (void) #define hard_smp_processor_id() ia64_get_lid() +typedef int cpuid_t; + #ifdef CONFIG_SMP #define XTP_OFFSET 0x1e0008 diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h index 8cd1cc3bc835..638f7562ba99 100644 --- a/arch/x86/include/asm/smp.h +++ b/arch/x86/include/asm/smp.h @@ -83,6 +83,8 @@ struct smp_ops { /* Globals due to paravirt */ extern void set_cpu_sibling_map(int cpu); +typedef int cpuid_t; + #ifdef CONFIG_SMP #ifndef CONFIG_PARAVIRT #define startup_ipi_hook(phys_apicid, start_eip, start_esp) do { } while (0) diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 1020b1b53a17..3e1a6b3ee3b8 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -215,7 +215,8 @@ static int acpi_processor_get_info(struct acpi_device *device) union acpi_object object = { 0 }; struct acpi_buffer buffer = { sizeof(union acpi_object), &object }; struct acpi_processor *pr = acpi_driver_data(device); - int phys_id, cpu_index, device_declaration = 0; + cpuid_t phys_id; + int cpu_index, device_declaration = 0; acpi_status status = AE_OK; static int cpu0_initialized; unsigned long long value; diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c index 5ac12e40fedd..a2735c315ef7 100644 --- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -13,7 +13,7 @@ ACPI_MODULE_NAME("processor_core"); static int map_lapic_id(struct acpi_subtable_header *entry, - u32 acpi_id, int *apic_id) + u32 acpi_id, cpuid_t *apic_id) { struct acpi_madt_local_apic *lapic = container_of(entry, struct acpi_madt_local_apic, header); @@ -29,7 +29,7 @@ static int map_lapic_id(struct acpi_subtable_header *entry, } static int map_x2apic_id(struct acpi_subtable_header *entry, - int device_declaration, u32 acpi_id, int *apic_id) + int device_declaration, u32 acpi_id, cpuid_t *apic_id) { struct acpi_madt_local_x2apic *apic = container_of(entry, struct acpi_madt_local_x2apic, header); @@ -46,7 +46,7 @@ static int map_x2apic_id(struct acpi_subtable_header *entry, } static int map_lsapic_id(struct acpi_subtable_header *entry, - int device_declaration, u32 acpi_id, int *apic_id) + int device_declaration, u32 acpi_id, cpuid_t *apic_id) { struct acpi_madt_local_sapic *lsapic = container_of(entry, struct acpi_madt_local_sapic, header); @@ -69,7 +69,7 @@ static int map_lsapic_id(struct acpi_subtable_header *entry, * on Intel platforms */ static int map_gicc_mpidr(struct acpi_subtable_header *entry, - int device_declaration, u32 acpi_id, int *mpidr) + int device_declaration, u32 acpi_id, cpuid_t *mpidr) { struct acpi_madt_generic_interrupt *gicc = container_of(entry, struct acpi_madt_generic_interrupt, header); @@ -84,24 +84,21 @@ static int map_gicc_mpidr(struct acpi_subtable_header *entry, if (device_declaration && (gicc->uid == acpi_id)) { /* * bits other than [0:7] Aff0, [8:15] Aff1, [16:23] Aff2 and - * [32:39] Aff3 must be 0 which is defined in ACPI 5.1, so pack - * the Affx fields into a single 32 bit identifier to accommodate - * the acpi processor drivers. + * [32:39] Aff3 must be 0 which is defined in ACPI 5.1 */ - *mpidr = ((gicc->arm_mpidr & 0xff00000000) >> 8) - | gicc->arm_mpidr; + *mpidr = gicc->arm_mpidr & 0xff00ffffffUL; return 0; } return -EINVAL; } -static int map_madt_entry(int type, u32 acpi_id) +static cpuid_t map_madt_entry(int type, u32 acpi_id) { unsigned long madt_end, entry; static struct acpi_table_madt *madt; static int read_madt; - int phys_id = -1; /* CPU hardware ID */ + cpuid_t phys_id = -1; /* CPU hardware ID */ if (!read_madt) { if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MADT, 0, @@ -145,7 +142,7 @@ static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id) struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; union acpi_object *obj; struct acpi_subtable_header *header; - int phys_id = -1; + cpuid_t phys_id = -1; if (ACPI_FAILURE(acpi_evaluate_object(handle, "_MAT", NULL, &buffer))) goto exit; @@ -174,7 +171,7 @@ exit: return phys_id; } -int acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id) +cpuid_t acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id) { int phys_id; @@ -185,7 +182,7 @@ int acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id) return phys_id; } -int acpi_map_cpuid(int phys_id, u32 acpi_id) +int acpi_map_cpuid(cpuid_t phys_id, u32 acpi_id) { #ifdef CONFIG_SMP int i; @@ -213,9 +210,9 @@ int acpi_map_cpuid(int phys_id, u32 acpi_id) * Return -1 for other CPU's handle. */ if (nr_cpu_ids <= 1 && acpi_id == 0) - return acpi_id; + return 0; else - return phys_id; + return -1; } #ifdef CONFIG_SMP @@ -233,7 +230,7 @@ int acpi_map_cpuid(int phys_id, u32 acpi_id) int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id) { - int phys_id; + cpuid_t phys_id; phys_id = acpi_get_phys_id(handle, type, acpi_id); diff --git a/include/acpi/processor.h b/include/acpi/processor.h index b95dc32a6e6b..30ebdbf0d961 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -196,7 +196,7 @@ struct acpi_processor_flags { struct acpi_processor { acpi_handle handle; u32 acpi_id; - u32 phys_id; /* CPU hardware ID such as APIC ID for x86 */ + cpuid_t phys_id; /* CPU hardware ID such as APIC ID for x86 */ u32 id; /* CPU logical ID allocated by OS */ u32 pblk; int performance_platform_limit; @@ -310,8 +310,8 @@ static inline int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) #endif /* CONFIG_CPU_FREQ */ /* in processor_core.c */ -int acpi_get_phys_id(acpi_handle, int type, u32 acpi_id); -int acpi_map_cpuid(int phys_id, u32 acpi_id); +cpuid_t acpi_get_phys_id(acpi_handle, int type, u32 acpi_id); +int acpi_map_cpuid(cpuid_t phys_id, u32 acpi_id); int acpi_get_cpuid(acpi_handle, int type, u32 acpi_id); /* in processor_pdc.c */