| Message ID | 20220708203342.256459-2-mail@conchuod.ie (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix RISC-V's arch-topology reporting | expand |
On 08/07/2022 21:33, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > arm64's method of defining a default cpu topology requires only minimal > changes to apply to RISC-V also. The current arm64 implementation exits > early in a uniprocessor configuration by reading MPIDR & claiming that > uniprocessor can rely on the default values. > > This is appears to be a hangover from prior to '3102bc0e6ac7 ("arm64: > topology: Stop using MPIDR for topology information")', because the > current code just assigns default values for multiprocessor systems. > > With the MPIDR references removed, store_cpu_topolgy() can be moved to > the common arch_topology code. > > CC: stable@vger.kernel.org > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > arch/arm64/kernel/topology.c | 40 ------------------------------------ > drivers/base/arch_topology.c | 19 +++++++++++++++++ > 2 files changed, 19 insertions(+), 40 deletions(-) > > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > index 869ffc4d4484..7889a00f5487 100644 > --- a/arch/arm64/kernel/topology.c > +++ b/arch/arm64/kernel/topology.c > @@ -22,46 +22,6 @@ > #include <asm/cputype.h> > #include <asm/topology.h> > > -void store_cpu_topology(unsigned int cpuid) > -{ > - struct cpu_topology *cpuid_topo = &cpu_topology[cpuid]; > - u64 mpidr; > - > - if (cpuid_topo->package_id != -1) > - goto topology_populated; > - > - mpidr = read_cpuid_mpidr(); > - > - /* Uniprocessor systems can rely on default topology values */ > - if (mpidr & MPIDR_UP_BITMASK) > - return; > - > - /* > - * This would be the place to create cpu topology based on MPIDR. > - * > - * However, it cannot be trusted to depict the actual topology; some > - * pieces of the architecture enforce an artificial cap on Aff0 values > - * (e.g. GICv3's ICC_SGI1R_EL1 limits it to 15), leading to an > - * artificial cycling of Aff1, Aff2 and Aff3 values. IOW, these end up > - * having absolutely no relationship to the actual underlying system > - * topology, and cannot be reasonably used as core / package ID. > - * > - * If the MT bit is set, Aff0 *could* be used to define a thread ID, but > - * we still wouldn't be able to obtain a sane core ID. This means we > - * need to entirely ignore MPIDR for any topology deduction. > - */ > - cpuid_topo->thread_id = -1; > - cpuid_topo->core_id = cpuid; > - cpuid_topo->package_id = cpu_to_node(cpuid); > - > - pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n", > - cpuid, cpuid_topo->package_id, cpuid_topo->core_id, > - cpuid_topo->thread_id, mpidr); > - > -topology_populated: > - update_siblings_masks(cpuid); > -} > - > #ifdef CONFIG_ACPI > static bool __init acpi_cpu_is_threaded(int cpu) > { > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index 441e14ac33a4..07e84c6ac5c2 100644 > --- a/drivers/base/arch_topology.c > +++ b/drivers/base/arch_topology.c > @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid) > } > } > > +void __weak store_cpu_topology(unsigned int cpuid) Ahh crap, I forgot to remove the __weak. I won't immediately respin since it is minor. I've pushed it (without the __weak) to https://git.kernel.org/conor/h/arch-topo so it'll get the lkp coverage. Thanks, Conor. > +{ > + struct cpu_topology *cpuid_topo = &cpu_topology[cpuid]; > + > + if (cpuid_topo->package_id != -1) > + goto topology_populated; > + > + cpuid_topo->thread_id = -1; > + cpuid_topo->core_id = cpuid; > + cpuid_topo->package_id = cpu_to_node(cpuid); > + > + pr_debug("CPU%u: package %d core %d thread %d\n", > + cpuid, cpuid_topo->package_id, cpuid_topo->core_id, > + cpuid_topo->thread_id); > + > +topology_populated: > + update_siblings_masks(cpuid); > +} > + > static void clear_cpu_topology(int cpu) > { > struct cpu_topology *cpu_topo = &cpu_topology[cpu];
+CC Russel, Arnd On 08/07/2022 21:45, Conor Dooley - M52691 wrote: > On 08/07/2022 21:33, Conor Dooley wrote: >> From: Conor Dooley <conor.dooley@microchip.com> >> >> arm64's method of defining a default cpu topology requires only minimal >> changes to apply to RISC-V also. The current arm64 implementation exits >> early in a uniprocessor configuration by reading MPIDR & claiming that >> uniprocessor can rely on the default values. >> >> This is appears to be a hangover from prior to '3102bc0e6ac7 ("arm64: >> topology: Stop using MPIDR for topology information")', because the >> current code just assigns default values for multiprocessor systems. >> >> With the MPIDR references removed, store_cpu_topolgy() can be moved to >> the common arch_topology code. >> >> CC: stable@vger.kernel.org >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> --- ---8<--- >> #ifdef CONFIG_ACPI >> static bool __init acpi_cpu_is_threaded(int cpu) >> { >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c >> index 441e14ac33a4..07e84c6ac5c2 100644 >> --- a/drivers/base/arch_topology.c >> +++ b/drivers/base/arch_topology.c >> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid) >> } >> } >> >> +void __weak store_cpu_topology(unsigned int cpuid) > > Ahh crap, I forgot to remove the __weak. > I won't immediately respin since it is minor. I've pushed it (without > the __weak) to https://git.kernel.org/conor/h/arch-topo so it'll get > the lkp coverage. And build failure for arm32: > tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git arch-topo > branch HEAD: df379c4b12f6b22fb8c07c2be16fd821a4fcbfc5 riscv: topology: fix default topology reporting > > Error/Warning: (recently discovered and may have been fixed) > > arch_topology.c:(.text+0xbac): multiple definition of `store_cpu_topology'; arch/arm/kernel/topology.o:topology.c:(.text+0x0): first defined here > > Error/Warning ids grouped by kconfigs: > > gcc_recent_errors > `-- arm-defconfig > `-- multiple-definition-of-store_cpu_topology-arch-arm-kernel-topology.o:topology.c:(.text):first-defined-here > > elapsed time: 721m Looking at the arm32 implementation - it appears to be mostly the sort of MPIDR stuff that was removed from the arm64 implementation in 3102bc0e6ac7 ("arm64: topology: Stop using MPIDR for topology information"). Could arm32 benefit from the same shared implemenation too, or is usage of MPIDR only invalid for arm64? The other difference is a call to update_cpu_capacity() in the arm32 implementation. Could that be moved to smp_store_cpu_info() which is the only callsite of store_cpu_topology()? Either way, will respin a v3 that doesn't break the arm32 build when CONFIG_GENERIC_ARCH_TOPOLOGY is enabled :) Thanks, Conor.
On Sat, Jul 09, 2022 at 12:58:57PM +0000, Conor.Dooley@microchip.com wrote: > +CC Russel, Arnd > > On 08/07/2022 21:45, Conor Dooley - M52691 wrote: > > On 08/07/2022 21:33, Conor Dooley wrote: > >> From: Conor Dooley <conor.dooley@microchip.com> > >> > >> arm64's method of defining a default cpu topology requires only minimal > >> changes to apply to RISC-V also. The current arm64 implementation exits > >> early in a uniprocessor configuration by reading MPIDR & claiming that > >> uniprocessor can rely on the default values. > >> > >> This is appears to be a hangover from prior to '3102bc0e6ac7 ("arm64: > >> topology: Stop using MPIDR for topology information")', because the > >> current code just assigns default values for multiprocessor systems. > >> > >> With the MPIDR references removed, store_cpu_topolgy() can be moved to > >> the common arch_topology code. > >> > >> CC: stable@vger.kernel.org > >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > >> --- > ---8<--- > >> #ifdef CONFIG_ACPI > >> static bool __init acpi_cpu_is_threaded(int cpu) > >> { > >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > >> index 441e14ac33a4..07e84c6ac5c2 100644 > >> --- a/drivers/base/arch_topology.c > >> +++ b/drivers/base/arch_topology.c > >> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid) > >> } > >> } > >> > >> +void __weak store_cpu_topology(unsigned int cpuid) > > > > Ahh crap, I forgot to remove the __weak. > > I won't immediately respin since it is minor. I've pushed it (without > > the __weak) to https://git.kernel.org/conor/h/arch-topo so it'll get > > the lkp coverage. > > And build failure for arm32: > > > tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git arch-topo > > branch HEAD: df379c4b12f6b22fb8c07c2be16fd821a4fcbfc5 riscv: topology: fix default topology reporting > > > > Error/Warning: (recently discovered and may have been fixed) > > > > arch_topology.c:(.text+0xbac): multiple definition of `store_cpu_topology'; arch/arm/kernel/topology.o:topology.c:(.text+0x0): first defined here > > > > Error/Warning ids grouped by kconfigs: > > > > gcc_recent_errors > > `-- arm-defconfig > > `-- multiple-definition-of-store_cpu_topology-arch-arm-kernel-topology.o:topology.c:(.text):first-defined-here > > > > elapsed time: 721m > > Looking at the arm32 implementation - it appears to be mostly the sort of MPIDR > stuff that was removed from the arm64 implementation in 3102bc0e6ac7 ("arm64: > topology: Stop using MPIDR for topology information"). Could arm32 benefit from > the same shared implemenation too, or is usage of MPIDR only invalid for arm64? Don't look at me... this code was contributed by Linaro, presumably for systems they have. I've never had anything that would require this so the code never interested me, so I never took much notice of it. Sorry, I can't be of more help.
On Sat, Jul 09, 2022 at 12:58:57PM +0000, Conor.Dooley@microchip.com wrote: > Looking at the arm32 implementation - it appears to be mostly the sort of MPIDR > stuff that was removed from the arm64 implementation in 3102bc0e6ac7 ("arm64: > topology: Stop using MPIDR for topology information"). Could arm32 benefit from > the same shared implemenation too, or is usage of MPIDR only invalid for arm64? I don't recall all the details but IIRC there are parts if arch_topology that are ARM64/RISC-V only. ARM32 doesn't use it as it may break old platforms. Some of the functions that still arm32 specific are retained in arch/arm > The other difference is a call to update_cpu_capacity() in the arm32 > implementation. Could that be moved to smp_store_cpu_info() which is the only > callsite of store_cpu_topology()? > No please, leave arm32 as is. It was done for a reason like that and it help to not break some of the old 32-by platforms. > Either way, will respin a v3 that doesn't break the arm32 build when > CONFIG_GENERIC_ARCH_TOPOLOGY is enabled :) > Thanks.
On 11/07/2022 11:02, Sudeep Holla wrote: > On Sat, Jul 09, 2022 at 12:58:57PM +0000, Conor.Dooley@microchip.com wrote: >> Looking at the arm32 implementation - it appears to be mostly the sort of MPIDR >> stuff that was removed from the arm64 implementation in 3102bc0e6ac7 ("arm64: >> topology: Stop using MPIDR for topology information"). Could arm32 benefit from >> the same shared implemenation too, or is usage of MPIDR only invalid for arm64? > > I don't recall all the details but IIRC there are parts if arch_topology > that are ARM64/RISC-V only. ARM32 doesn't use it as it may break old > platforms. Some of the functions that still arm32 specific are retained > in arch/arm > >> The other difference is a call to update_cpu_capacity() in the arm32 >> implementation. Could that be moved to smp_store_cpu_info() which is the only >> callsite of store_cpu_topology()? >> > > No please, leave arm32 as is. It was done for a reason like that and it > help to not break some of the old 32-by platforms. Thought that might be the case, I won't (and didn't) touch arm32! Thanks, Conor.
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c index 869ffc4d4484..7889a00f5487 100644 --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@ -22,46 +22,6 @@ #include <asm/cputype.h> #include <asm/topology.h> -void store_cpu_topology(unsigned int cpuid) -{ - struct cpu_topology *cpuid_topo = &cpu_topology[cpuid]; - u64 mpidr; - - if (cpuid_topo->package_id != -1) - goto topology_populated; - - mpidr = read_cpuid_mpidr(); - - /* Uniprocessor systems can rely on default topology values */ - if (mpidr & MPIDR_UP_BITMASK) - return; - - /* - * This would be the place to create cpu topology based on MPIDR. - * - * However, it cannot be trusted to depict the actual topology; some - * pieces of the architecture enforce an artificial cap on Aff0 values - * (e.g. GICv3's ICC_SGI1R_EL1 limits it to 15), leading to an - * artificial cycling of Aff1, Aff2 and Aff3 values. IOW, these end up - * having absolutely no relationship to the actual underlying system - * topology, and cannot be reasonably used as core / package ID. - * - * If the MT bit is set, Aff0 *could* be used to define a thread ID, but - * we still wouldn't be able to obtain a sane core ID. This means we - * need to entirely ignore MPIDR for any topology deduction. - */ - cpuid_topo->thread_id = -1; - cpuid_topo->core_id = cpuid; - cpuid_topo->package_id = cpu_to_node(cpuid); - - pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n", - cpuid, cpuid_topo->package_id, cpuid_topo->core_id, - cpuid_topo->thread_id, mpidr); - -topology_populated: - update_siblings_masks(cpuid); -} - #ifdef CONFIG_ACPI static bool __init acpi_cpu_is_threaded(int cpu) { diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index 441e14ac33a4..07e84c6ac5c2 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid) } } +void __weak store_cpu_topology(unsigned int cpuid) +{ + struct cpu_topology *cpuid_topo = &cpu_topology[cpuid]; + + if (cpuid_topo->package_id != -1) + goto topology_populated; + + cpuid_topo->thread_id = -1; + cpuid_topo->core_id = cpuid; + cpuid_topo->package_id = cpu_to_node(cpuid); + + pr_debug("CPU%u: package %d core %d thread %d\n", + cpuid, cpuid_topo->package_id, cpuid_topo->core_id, + cpuid_topo->thread_id); + +topology_populated: + update_siblings_masks(cpuid); +} + static void clear_cpu_topology(int cpu) { struct cpu_topology *cpu_topo = &cpu_topology[cpu];