public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] Fix RISC-V's arch-topology reporting
@ 2022-07-09 15:23 Conor Dooley
  2022-07-09 15:23 ` [PATCH v3 1/2] arm64: topology: move store_cpu_topology() to shared code Conor Dooley
  2022-07-09 15:23 ` [PATCH v3 2/2] riscv: topology: fix default topology reporting Conor Dooley
  0 siblings, 2 replies; 9+ messages in thread
From: Conor Dooley @ 2022-07-09 15:23 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou,
	Sudeep Holla, Catalin Marinas, Will Deacon, Greg Kroah-Hartman,
	Rafael J . Wysocki
  Cc: Daire McNamara, Conor Dooley, Niklas Cassel, Damien Le Moal,
	Geert Uytterhoeven, Zong Li, Emil Renner Berthing, Jonas Hahnfeld,
	Guo Ren, Anup Patel, Atish Patra, Heiko Stuebner, Philipp Tomsich,
	Rob Herring, Marc Zyngier, Viresh Kumar, linux-riscv,
	linux-kernel, linux-arm-kernel, Brice Goglin

From: Conor Dooley <conor.dooley@microchip.com>

Hey all,
It's my first time messing around with arch/ code at all, let alone
more than one arch, so forgive me if I have screwed up how to do a
migration like this.

The goal here is the fix the incorrectly reported arch topology on
RISC-V which seems to have been broken since it was added.
cpu, package and thread IDs are all currently reported as -1, so tools
like lstopo think systems have multiple threads on the same core when
this is not true:
https://github.com/open-mpi/hwloc/issues/536

arm64's topology code basically applies to RISC-V too, so it has been
made generic along with the removal of MPIDR related code, which
appears to be redudant code since '3102bc0e6ac7 ("arm64: topology: Stop
using MPIDR for topology information")' replaced the code that actually
interacted with MPIDR with default values.

I only built tested for arm{,64} , so hopefully it is not broken when
used. Testing on both arm64 & !SMP RISC-V would really be appreciated!

For V2, I dropped the idea of doing a RISC-V specific implementation
followed by a move to the generic code & just went for the more straight
forward method of moving to the shared version first. I also dropped the
RFC.

V3 moves store_cpu_topology() down inside the arch check alongside the
init function so that boot on 32bit arm is not broken.

Thanks,
Conor

Conor Dooley (2):
  arm64: topology: move store_cpu_topology() to shared code
  riscv: topology: fix default topology reporting

 arch/arm64/kernel/topology.c | 40 ------------------------------------
 arch/riscv/Kconfig           |  2 +-
 arch/riscv/kernel/smpboot.c  |  4 +++-
 drivers/base/arch_topology.c | 19 +++++++++++++++++
 4 files changed, 23 insertions(+), 42 deletions(-)


base-commit: b6f1f2fa2bddd69ff46a190b8120bd440fd50563
-- 
2.37.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/2] arm64: topology: move store_cpu_topology() to shared code
  2022-07-09 15:23 [PATCH v3 0/2] Fix RISC-V's arch-topology reporting Conor Dooley
@ 2022-07-09 15:23 ` Conor Dooley
  2022-07-11 14:35   ` Sudeep Holla
  2022-07-09 15:23 ` [PATCH v3 2/2] riscv: topology: fix default topology reporting Conor Dooley
  1 sibling, 1 reply; 9+ messages in thread
From: Conor Dooley @ 2022-07-09 15:23 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou,
	Sudeep Holla, Catalin Marinas, Will Deacon, Greg Kroah-Hartman,
	Rafael J . Wysocki
  Cc: Daire McNamara, Conor Dooley, Niklas Cassel, Damien Le Moal,
	Geert Uytterhoeven, Zong Li, Emil Renner Berthing, Jonas Hahnfeld,
	Guo Ren, Anup Patel, Atish Patra, Heiko Stuebner, Philipp Tomsich,
	Rob Herring, Marc Zyngier, Viresh Kumar, linux-riscv,
	linux-kernel, linux-arm-kernel, Brice Goglin

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..b7633bacbd31 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -845,4 +845,23 @@ void __init init_cpu_topology(void)
 		}
 	}
 }
+
+void 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);
+}
 #endif
-- 
2.37.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/2] riscv: topology: fix default topology reporting
  2022-07-09 15:23 [PATCH v3 0/2] Fix RISC-V's arch-topology reporting Conor Dooley
  2022-07-09 15:23 ` [PATCH v3 1/2] arm64: topology: move store_cpu_topology() to shared code Conor Dooley
@ 2022-07-09 15:23 ` Conor Dooley
  2022-07-11 14:59   ` Sudeep Holla
  1 sibling, 1 reply; 9+ messages in thread
From: Conor Dooley @ 2022-07-09 15:23 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou,
	Sudeep Holla, Catalin Marinas, Will Deacon, Greg Kroah-Hartman,
	Rafael J . Wysocki
  Cc: Daire McNamara, Conor Dooley, Niklas Cassel, Damien Le Moal,
	Geert Uytterhoeven, Zong Li, Emil Renner Berthing, Jonas Hahnfeld,
	Guo Ren, Anup Patel, Atish Patra, Heiko Stuebner, Philipp Tomsich,
	Rob Herring, Marc Zyngier, Viresh Kumar, linux-riscv,
	linux-kernel, linux-arm-kernel, Brice Goglin

From: Conor Dooley <conor.dooley@microchip.com>

RISC-V has no sane defaults to fall back on where there is no cpu-map
in the devicetree.
Without sane defaults, the package, core and thread IDs are all set to
-1. This causes user-visible inaccuracies for tools like hwloc/lstopo
which rely on the sysfs cpu topology files to detect a system's
topology.

On a PolarFire SoC, which should have 4 harts with a thread each,
lstopo currently reports:

Machine (793MB total)
  Package L#0
    NUMANode L#0 (P#0 793MB)
    Core L#0
      L1d L#0 (32KB) + L1i L#0 (32KB) + PU L#0 (P#0)
      L1d L#1 (32KB) + L1i L#1 (32KB) + PU L#1 (P#1)
      L1d L#2 (32KB) + L1i L#2 (32KB) + PU L#2 (P#2)
      L1d L#3 (32KB) + L1i L#3 (32KB) + PU L#3 (P#3)

Adding calls to store_cpu_topology() in {boot,smp} hart bringup code
results in the correct topolgy being reported:

Machine (793MB total)
  Package L#0
    NUMANode L#0 (P#0 793MB)
    L1d L#0 (32KB) + L1i L#0 (32KB) + Core L#0 + PU L#0 (P#0)
    L1d L#1 (32KB) + L1i L#1 (32KB) + Core L#1 + PU L#1 (P#1)
    L1d L#2 (32KB) + L1i L#2 (32KB) + Core L#2 + PU L#2 (P#2)
    L1d L#3 (32KB) + L1i L#3 (32KB) + Core L#3 + PU L#3 (P#3)

CC: stable@vger.kernel.org
Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
Reported-by: Brice Goglin <Brice.Goglin@inria.fr>
Link: https://github.com/open-mpi/hwloc/issues/536
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
---
 arch/riscv/Kconfig          | 2 +-
 arch/riscv/kernel/smpboot.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 2af0701b7518..4b6c2fdbb57c 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -52,7 +52,7 @@ config RISCV
 	select COMMON_CLK
 	select CPU_PM if CPU_IDLE
 	select EDAC_SUPPORT
-	select GENERIC_ARCH_TOPOLOGY if SMP
+	select GENERIC_ARCH_TOPOLOGY
 	select GENERIC_ATOMIC64 if !64BIT
 	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
 	select GENERIC_EARLY_IOREMAP
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index f1e4948a4b52..a1c861f84fe2 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -40,6 +40,8 @@ static DECLARE_COMPLETION(cpu_running);
 void __init smp_prepare_boot_cpu(void)
 {
 	init_cpu_topology();
+
+	store_cpu_topology(smp_processor_id());
 }
 
 void __init smp_prepare_cpus(unsigned int max_cpus)
@@ -161,9 +163,9 @@ asmlinkage __visible void smp_callin(void)
 	mmgrab(mm);
 	current->active_mm = mm;
 
+	store_cpu_topology(curr_cpuid);
 	notify_cpu_starting(curr_cpuid);
 	numa_add_cpu(curr_cpuid);
-	update_siblings_masks(curr_cpuid);
 	set_cpu_online(curr_cpuid, 1);
 
 	/*
-- 
2.37.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] arm64: topology: move store_cpu_topology() to shared code
  2022-07-09 15:23 ` [PATCH v3 1/2] arm64: topology: move store_cpu_topology() to shared code Conor Dooley
@ 2022-07-11 14:35   ` Sudeep Holla
  2022-07-11 14:50     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 9+ messages in thread
From: Sudeep Holla @ 2022-07-11 14:35 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou,
	Catalin Marinas, Will Deacon, Sudeep Holla, Greg Kroah-Hartman,
	Rafael J . Wysocki, Daire McNamara, Conor Dooley, Niklas Cassel,
	Damien Le Moal, Geert Uytterhoeven, Zong Li, Emil Renner Berthing,
	Jonas Hahnfeld, Guo Ren, Anup Patel, Atish Patra, Heiko Stuebner,
	Philipp Tomsich, Rob Herring, Marc Zyngier, Viresh Kumar,
	linux-riscv, linux-kernel, linux-arm-kernel, Brice Goglin

On Sat, Jul 09, 2022 at 04:23:54PM +0100, 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.
>

Looks good. FWIW,

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

> CC: stable@vger.kernel.org

However, while I understand the reason why this is needed in stable trees
for RISC-V, I am not sure if we want this for stable tree at-least on arm64.
I leave that part to Greg and Will.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] arm64: topology: move store_cpu_topology() to shared code
  2022-07-11 14:35   ` Sudeep Holla
@ 2022-07-11 14:50     ` Greg Kroah-Hartman
  2022-07-11 15:24       ` Sudeep Holla
  0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2022-07-11 14:50 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Conor Dooley, Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt,
	Albert Ou, Catalin Marinas, Will Deacon, Rafael J . Wysocki,
	Daire McNamara, Conor Dooley, Niklas Cassel, Damien Le Moal,
	Geert Uytterhoeven, Zong Li, Emil Renner Berthing, Jonas Hahnfeld,
	Guo Ren, Anup Patel, Atish Patra, Heiko Stuebner, Philipp Tomsich,
	Rob Herring, Marc Zyngier, Viresh Kumar, linux-riscv,
	linux-kernel, linux-arm-kernel, Brice Goglin

On Mon, Jul 11, 2022 at 03:35:42PM +0100, Sudeep Holla wrote:
> On Sat, Jul 09, 2022 at 04:23:54PM +0100, 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.
> >
> 
> Looks good. FWIW,
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> > CC: stable@vger.kernel.org
> 
> However, while I understand the reason why this is needed in stable trees
> for RISC-V, I am not sure if we want this for stable tree at-least on arm64.
> I leave that part to Greg and Will.

Why would it be good for one arch but bad for another?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/2] riscv: topology: fix default topology reporting
  2022-07-09 15:23 ` [PATCH v3 2/2] riscv: topology: fix default topology reporting Conor Dooley
@ 2022-07-11 14:59   ` Sudeep Holla
  2022-07-11 16:28     ` Conor.Dooley
  0 siblings, 1 reply; 9+ messages in thread
From: Sudeep Holla @ 2022-07-11 14:59 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Paul Walmsley, Palmer Dabbelt, Sudeep Holla, Palmer Dabbelt,
	Albert Ou, Catalin Marinas, Will Deacon, Greg Kroah-Hartman,
	Rafael J . Wysocki, Daire McNamara, Conor Dooley, Niklas Cassel,
	Damien Le Moal, Geert Uytterhoeven, Zong Li, Emil Renner Berthing,
	Jonas Hahnfeld, Guo Ren, Anup Patel, Atish Patra, Heiko Stuebner,
	Philipp Tomsich, Rob Herring, Marc Zyngier, Viresh Kumar,
	linux-riscv, linux-kernel, linux-arm-kernel, Brice Goglin

On Sat, Jul 09, 2022 at 04:23:55PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> RISC-V has no sane defaults to fall back on where there is no cpu-map
> in the devicetree.
> Without sane defaults, the package, core and thread IDs are all set to
> -1. This causes user-visible inaccuracies for tools like hwloc/lstopo
> which rely on the sysfs cpu topology files to detect a system's
> topology.
> 
> On a PolarFire SoC, which should have 4 harts with a thread each,
> lstopo currently reports:
> 
> Machine (793MB total)
>   Package L#0
>     NUMANode L#0 (P#0 793MB)
>     Core L#0
>       L1d L#0 (32KB) + L1i L#0 (32KB) + PU L#0 (P#0)
>       L1d L#1 (32KB) + L1i L#1 (32KB) + PU L#1 (P#1)
>       L1d L#2 (32KB) + L1i L#2 (32KB) + PU L#2 (P#2)
>       L1d L#3 (32KB) + L1i L#3 (32KB) + PU L#3 (P#3)
> 
> Adding calls to store_cpu_topology() in {boot,smp} hart bringup code
> results in the correct topolgy being reported:
> 
> Machine (793MB total)
>   Package L#0
>     NUMANode L#0 (P#0 793MB)
>     L1d L#0 (32KB) + L1i L#0 (32KB) + Core L#0 + PU L#0 (P#0)
>     L1d L#1 (32KB) + L1i L#1 (32KB) + Core L#1 + PU L#1 (P#1)
>     L1d L#2 (32KB) + L1i L#2 (32KB) + Core L#2 + PU L#2 (P#2)
>     L1d L#3 (32KB) + L1i L#3 (32KB) + Core L#3 + PU L#3 (P#3)
> 
> CC: stable@vger.kernel.org
> Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
> Reported-by: Brice Goglin <Brice.Goglin@inria.fr>
> Link: https://github.com/open-mpi/hwloc/issues/536
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> ---
>  arch/riscv/Kconfig          | 2 +-
>  arch/riscv/kernel/smpboot.c | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 2af0701b7518..4b6c2fdbb57c 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -52,7 +52,7 @@ config RISCV
>  	select COMMON_CLK
>  	select CPU_PM if CPU_IDLE
>  	select EDAC_SUPPORT
> -	select GENERIC_ARCH_TOPOLOGY if SMP
> +	select GENERIC_ARCH_TOPOLOGY

I am not sure of !SMP as ARM64 is default SMP only. I have never reviewed
the arch topology code with !SMP considered. I will leave that part to
RISC-V developers.

>  	select GENERIC_ATOMIC64 if !64BIT
>  	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
>  	select GENERIC_EARLY_IOREMAP
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index f1e4948a4b52..a1c861f84fe2 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -40,6 +40,8 @@ static DECLARE_COMPLETION(cpu_running);
>  void __init smp_prepare_boot_cpu(void)
>  {
>  	init_cpu_topology();
> +
> +	store_cpu_topology(smp_processor_id());
>  }
>  
>  void __init smp_prepare_cpus(unsigned int max_cpus)
> @@ -161,9 +163,9 @@ asmlinkage __visible void smp_callin(void)
>  	mmgrab(mm);
>  	current->active_mm = mm;
>  
> +	store_cpu_topology(curr_cpuid);
>  	notify_cpu_starting(curr_cpuid);
>  	numa_add_cpu(curr_cpuid);
> -	update_siblings_masks(curr_cpuid);
>  	set_cpu_online(curr_cpuid, 1);
>  
>  	/*

Other than that, this looks good. FWIW:
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] arm64: topology: move store_cpu_topology() to shared code
  2022-07-11 14:50     ` Greg Kroah-Hartman
@ 2022-07-11 15:24       ` Sudeep Holla
  2022-07-11 16:39         ` Conor.Dooley
  0 siblings, 1 reply; 9+ messages in thread
From: Sudeep Holla @ 2022-07-11 15:24 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Conor Dooley, Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt,
	Sudeep Holla, Albert Ou, Catalin Marinas, Will Deacon,
	Rafael J . Wysocki, Daire McNamara, Conor Dooley, Niklas Cassel,
	Damien Le Moal, Geert Uytterhoeven, Zong Li, Emil Renner Berthing,
	Jonas Hahnfeld, Guo Ren, Anup Patel, Atish Patra, Heiko Stuebner,
	Philipp Tomsich, Rob Herring, Marc Zyngier, Viresh Kumar,
	linux-riscv, linux-kernel, linux-arm-kernel, Brice Goglin

On Mon, Jul 11, 2022 at 04:50:38PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 11, 2022 at 03:35:42PM +0100, Sudeep Holla wrote:
> > On Sat, Jul 09, 2022 at 04:23:54PM +0100, 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.
> > >
> > 
> > Looks good. FWIW,
> > 
> > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > 
> > > CC: stable@vger.kernel.org
> > 
> > However, while I understand the reason why this is needed in stable trees
> > for RISC-V, I am not sure if we want this for stable tree at-least on arm64.
> > I leave that part to Greg and Will.
> 
> Why would it be good for one arch but bad for another?

Not really bad as such. Just needs testing and must not change much ideally,
but it really depends on which stable trees we will target and what is the
original state there. As mentioned in the commit, this changed a bit around
v5.8/9 on arm64 and not sure what kernels RISC-V needs this. There could
be some surprises on some Andriod platforms but that is something we can
look at when if and when there are complaints.

I am in general not sure what is the -stable tree rules is such situation and
hence made the noise so that we are aware that we may need more work than just
backporting this patch. Also this is just my opinion. If we decide to backport
esp. to kernels older than the one containing 3102bc0e6ac7, then arm64 may need
more changes or probably we can pull that commit if that makes it easier. Based
on what is decided and what are the targeted -stable trees, we can dig deeper.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/2] riscv: topology: fix default topology reporting
  2022-07-11 14:59   ` Sudeep Holla
@ 2022-07-11 16:28     ` Conor.Dooley
  0 siblings, 0 replies; 9+ messages in thread
From: Conor.Dooley @ 2022-07-11 16:28 UTC (permalink / raw)
  To: sudeep.holla, mail
  Cc: paul.walmsley, palmer, palmer, aou, catalin.marinas, will, gregkh,
	rafael, Daire.McNamara, niklas.cassel, damien.lemoal, geert,
	zong.li, kernel, hahnjo, guoren, anup, atishp, heiko,
	philipp.tomsich, robh, maz, viresh.kumar, linux-riscv,
	linux-kernel, linux-arm-kernel, Brice.Goglin

On 11/07/2022 15:59, Sudeep Holla wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Sat, Jul 09, 2022 at 04:23:55PM +0100, Conor Dooley wrote:
>> From: Conor Dooley <conor.dooley@microchip.com>
>>
>> RISC-V has no sane defaults to fall back on where there is no cpu-map
>> in the devicetree.
>> Without sane defaults, the package, core and thread IDs are all set to
>> -1. This causes user-visible inaccuracies for tools like hwloc/lstopo
>> which rely on the sysfs cpu topology files to detect a system's
>> topology.
>>
>> On a PolarFire SoC, which should have 4 harts with a thread each,
>> lstopo currently reports:
>>
>> Machine (793MB total)
>>   Package L#0
>>     NUMANode L#0 (P#0 793MB)
>>     Core L#0
>>       L1d L#0 (32KB) + L1i L#0 (32KB) + PU L#0 (P#0)
>>       L1d L#1 (32KB) + L1i L#1 (32KB) + PU L#1 (P#1)
>>       L1d L#2 (32KB) + L1i L#2 (32KB) + PU L#2 (P#2)
>>       L1d L#3 (32KB) + L1i L#3 (32KB) + PU L#3 (P#3)
>>
>> Adding calls to store_cpu_topology() in {boot,smp} hart bringup code
>> results in the correct topolgy being reported:
>>
>> Machine (793MB total)
>>   Package L#0
>>     NUMANode L#0 (P#0 793MB)
>>     L1d L#0 (32KB) + L1i L#0 (32KB) + Core L#0 + PU L#0 (P#0)
>>     L1d L#1 (32KB) + L1i L#1 (32KB) + Core L#1 + PU L#1 (P#1)
>>     L1d L#2 (32KB) + L1i L#2 (32KB) + Core L#2 + PU L#2 (P#2)
>>     L1d L#3 (32KB) + L1i L#3 (32KB) + Core L#3 + PU L#3 (P#3)
>>
>> CC: stable@vger.kernel.org
>> Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
>> Reported-by: Brice Goglin <Brice.Goglin@inria.fr>
>> Link: https://github.com/open-mpi/hwloc/issues/536
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>> ---
>>  arch/riscv/Kconfig          | 2 +-
>>  arch/riscv/kernel/smpboot.c | 4 +++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 2af0701b7518..4b6c2fdbb57c 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -52,7 +52,7 @@ config RISCV
>>       select COMMON_CLK
>>       select CPU_PM if CPU_IDLE
>>       select EDAC_SUPPORT
>> -     select GENERIC_ARCH_TOPOLOGY if SMP
>> +     select GENERIC_ARCH_TOPOLOGY
> 
> I am not sure of !SMP as ARM64 is default SMP only. I have never reviewed
> the arch topology code with !SMP considered. I will leave that part to
> RISC-V developers.
> 

I checked it on a D1 which is !SMP - no trouble booting and
the topology reporting seemed fine.

Thanks for the reviews,
Conor.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] arm64: topology: move store_cpu_topology() to shared code
  2022-07-11 15:24       ` Sudeep Holla
@ 2022-07-11 16:39         ` Conor.Dooley
  0 siblings, 0 replies; 9+ messages in thread
From: Conor.Dooley @ 2022-07-11 16:39 UTC (permalink / raw)
  To: sudeep.holla, gregkh
  Cc: mail, paul.walmsley, palmer, palmer, aou, catalin.marinas, will,
	rafael, Daire.McNamara, niklas.cassel, damien.lemoal, geert,
	zong.li, kernel, hahnjo, guoren, anup, atishp, heiko,
	philipp.tomsich, robh, maz, viresh.kumar, linux-riscv,
	linux-kernel, linux-arm-kernel, Brice.Goglin

On 11/07/2022 16:24, Sudeep Holla wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Jul 11, 2022 at 04:50:38PM +0200, Greg Kroah-Hartman wrote:
>> On Mon, Jul 11, 2022 at 03:35:42PM +0100, Sudeep Holla wrote:
>>> On Sat, Jul 09, 2022 at 04:23:54PM +0100, 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.
>>>>
>>>
>>> Looks good. FWIW,
>>>
>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>>>
>>>> CC: stable@vger.kernel.org
>>>
>>> However, while I understand the reason why this is needed in stable trees
>>> for RISC-V, I am not sure if we want this for stable tree at-least on arm64.
>>> I leave that part to Greg and Will.
>>
>> Why would it be good for one arch but bad for another?
> 
> Not really bad as such. Just needs testing and must not change much ideally,
> but it really depends on which stable trees we will target and what is the
> original state there. As mentioned in the commit, this changed a bit around
> v5.8/9 on arm64 and not sure what kernels RISC-V needs this. There could
> be some surprises on some Andriod platforms but that is something we can
> look at when if and when there are complaints.
> 
> I am in general not sure what is the -stable tree rules is such situation and
> hence made the noise so that we are aware that we may need more work than just
> backporting this patch. Also this is just my opinion. If we decide to backport
> esp. to kernels older than the one containing 3102bc0e6ac7, then arm64 may need
> more changes or probably we can pull that commit if that makes it easier. Based
> on what is decided and what are the targeted -stable trees, we can dig deeper.

There's always the option of, for the older kernels, not migrating arm64 at all
and just wrap store_cpu_topo with "if RISCV" rather than "if RISCV || ARM64".


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-07-11 16:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-09 15:23 [PATCH v3 0/2] Fix RISC-V's arch-topology reporting Conor Dooley
2022-07-09 15:23 ` [PATCH v3 1/2] arm64: topology: move store_cpu_topology() to shared code Conor Dooley
2022-07-11 14:35   ` Sudeep Holla
2022-07-11 14:50     ` Greg Kroah-Hartman
2022-07-11 15:24       ` Sudeep Holla
2022-07-11 16:39         ` Conor.Dooley
2022-07-09 15:23 ` [PATCH v3 2/2] riscv: topology: fix default topology reporting Conor Dooley
2022-07-11 14:59   ` Sudeep Holla
2022-07-11 16:28     ` Conor.Dooley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox