[PATCH v2 0/2] Fix RISC-V's arch-topology reporting

[PATCH v2 0/2] Fix RISC-V's arch-topology reporting

[Conor Dooley]

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 arm64, 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 :)

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 0/2] Fix RISC-V's arch-topology reporting

[Conor Dooley]

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 arm64, 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 :)

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

[PATCH v2 0/2] Fix RISC-V's arch-topology reporting

[Conor Dooley]

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 arm64, 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 :)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor Dooley]

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)
+{
+	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];
-- 
2.37.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor Dooley]

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)
+{
+	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];
-- 
2.37.0

[PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor Dooley]

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)
+{
+	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];
-- 
2.37.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] riscv: topology: fix default topology reporting

[Conor Dooley]

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

[PATCH v2 2/2] riscv: topology: fix default topology reporting

[Conor Dooley]

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 2/2] riscv: topology: fix default topology reporting

[Conor Dooley]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor.Dooley]

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];

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor.Dooley]

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];
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor.Dooley]

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];
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor.Dooley]

+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.

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor.Dooley]

+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.



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor.Dooley]

+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.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor.Dooley]

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.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor.Dooley]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] arm64: topology: move store_cpu_topology() to shared code

[Conor.Dooley]

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.