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

[PATCH v4 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 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()'s definition down inside the arch check
alongside the init function so that boot on 32bit arm is not broken.

V4 has moved the RISC-V boot hart's call to store_cpu_topology() later
into the boot process it is now right before SMP is brought up (or not
in the case of !SMP). This prevents calling detect_cache_attributes()
while we cannot allocate memory.

V4 is also rebased on next-20220715 to get Sudeep's most recent
arch_topology patchset.

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  |  3 ++-
 drivers/base/arch_topology.c | 19 +++++++++++++++++
 4 files changed, 22 insertions(+), 42 deletions(-)


base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
-- 
2.37.1


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

[PATCH v4 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>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.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 0424b59b695e..0e2c6b30dd69 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -841,4 +841,23 @@ void __init init_cpu_topology(void)
 		return;
 	}
 }
+
+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.1


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

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

[Catalin Marinas]

On Fri, Jul 15, 2022 at 06:51:55PM +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.
> 
> CC: stable@vger.kernel.org

I'd quantify how far back you want this to go. IIUC based on the Fixes
tag in the other patch, it should stop at 5.4. If you send a pull
request instead and have a fixed commit id, you could add it as a
prerequisite on the following patch without a cc stable here.

Either way:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

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

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

[Conor.Dooley]

On 19/07/2022 12:41, Catalin Marinas wrote:
> On Fri, Jul 15, 2022 at 06:51:55PM +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.
>>
>> CC: stable@vger.kernel.org
> 
> I'd quantify how far back you want this to go. IIUC based on the Fixes
> tag in the other patch, it should stop at 5.4. If you send a pull
> request instead and have a fixed commit id, you could add it as a
> prerequisite on the following patch without a cc stable here.

I guess a PR might be the easiest way for it anyway, so that both
yourself and Palmer could merge it?

> 
> Either way:
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

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

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

[Catalin Marinas]

On Tue, Jul 19, 2022 at 11:51:04AM +0000, Conor.Dooley@microchip.com wrote:
> On 19/07/2022 12:41, Catalin Marinas wrote:
> > On Fri, Jul 15, 2022 at 06:51:55PM +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.
> >>
> >> CC: stable@vger.kernel.org
> > 
> > I'd quantify how far back you want this to go. IIUC based on the Fixes
> > tag in the other patch, it should stop at 5.4. If you send a pull
> > request instead and have a fixed commit id, you could add it as a
> > prerequisite on the following patch without a cc stable here.
> 
> I guess a PR might be the easiest way for it anyway, so that both
> yourself and Palmer could merge it?

I guess so, a stable branch would do. Note that Will is handling the
upcoming merging window.

-- 
Catalin

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

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

[Atish Patra]

On Fri, Jul 15, 2022 at 10:53 AM Conor Dooley <mail@conchuod.ie> 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>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.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 0424b59b695e..0e2c6b30dd69 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -841,4 +841,23 @@ void __init init_cpu_topology(void)
>                 return;
>         }
>  }
> +
> +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.1
>

LGTM.
Reviewed-by: Atish Patra <atishp@rivosinc.com>



--
Regards,
Atish

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

[PATCH v4 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>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/riscv/Kconfig          | 2 +-
 arch/riscv/kernel/smpboot.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 205c1e2f539c..7ffac8818060 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..b4d5524b1077 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -49,6 +49,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	unsigned int curr_cpuid;
 
 	curr_cpuid = smp_processor_id();
+	store_cpu_topology(curr_cpuid);
 	numa_store_cpu_info(curr_cpuid);
 	numa_add_cpu(curr_cpuid);
 
@@ -161,9 +162,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.1


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

Re: [PATCH v4 2/2] riscv: topology: fix default topology reporting

[Atish Patra]

On Fri, Jul 15, 2022 at 10:53 AM Conor Dooley <mail@conchuod.ie> 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>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/riscv/Kconfig          | 2 +-
>  arch/riscv/kernel/smpboot.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 205c1e2f539c..7ffac8818060 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..b4d5524b1077 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -49,6 +49,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>         unsigned int curr_cpuid;
>
>         curr_cpuid = smp_processor_id();
> +       store_cpu_topology(curr_cpuid);
>         numa_store_cpu_info(curr_cpuid);
>         numa_add_cpu(curr_cpuid);
>
> @@ -161,9 +162,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.1
>

LGTM.
Reviewed-by: Atish Patra <atishp@rivosinc.com>
-- 
Regards,
Atish

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

Re: [PATCH v4 0/2] Fix RISC-V's arch-topology reporting

[Conor.Dooley]

On 15/07/2022 18:51, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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!

FWIW, I pushed it to a branch for the sake of LKP testing and all
archs + randconfigs built fine.
Thanks,
Conor.

> 
> 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()'s definition down inside the arch check
> alongside the init function so that boot on 32bit arm is not broken.
> 
> V4 has moved the RISC-V boot hart's call to store_cpu_topology() later
> into the boot process it is now right before SMP is brought up (or not
> in the case of !SMP). This prevents calling detect_cache_attributes()
> while we cannot allocate memory.
> 
> V4 is also rebased on next-20220715 to get Sudeep's most recent
> arch_topology patchset.
> 
> 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  |  3 ++-
>  drivers/base/arch_topology.c | 19 +++++++++++++++++
>  4 files changed, 22 insertions(+), 42 deletions(-)
> 
> 
> base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
> --
> 2.37.1
> 

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

Re: [PATCH v4 0/2] Fix RISC-V's arch-topology reporting

[Conor.Dooley]

On 15/07/2022 18:51, Conor Dooley wrote:
> 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

Hey,

Not got any feedback on the smpboot changes from the RISC-V side.
I tested it on polarfire, the d1 (with both SMP & !SMP set iirc)
& on the u540. It all looked good to me.

I'd like to have this fixed for v5.20, but there isn't too much
time left before the mw. Not too sure about the cross-tree changes,
does it need an immutable branch or could it go through driver-core?
Catalin suggested removing the CC stable from patch 1/2 & adding it
as a dependency for the 2/2 patch - but obviously that's up to the
committer to sort out.

I guess since it is a fix, it could also go into rc1<

Thanks,
Conor.

> 
> 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()'s definition down inside the arch check
> alongside the init function so that boot on 32bit arm is not broken.
> 
> V4 has moved the RISC-V boot hart's call to store_cpu_topology() later
> into the boot process it is now right before SMP is brought up (or not
> in the case of !SMP). This prevents calling detect_cache_attributes()
> while we cannot allocate memory.
> 
> V4 is also rebased on next-20220715 to get Sudeep's most recent
> arch_topology patchset.
> 
> 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  |  3 ++-
>  drivers/base/arch_topology.c | 19 +++++++++++++++++
>  4 files changed, 22 insertions(+), 42 deletions(-)
> 
> 
> base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 0/2] Fix RISC-V's arch-topology reporting

[Will Deacon]

On Sat, Jul 23, 2022 at 11:22:01AM +0000, Conor.Dooley@microchip.com wrote:
> On 15/07/2022 18:51, Conor Dooley wrote:
> > 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
> 
> Hey,
> 
> Not got any feedback on the smpboot changes from the RISC-V side.
> I tested it on polarfire, the d1 (with both SMP & !SMP set iirc)
> & on the u540. It all looked good to me.
> 
> I'd like to have this fixed for v5.20, but there isn't too much
> time left before the mw. Not too sure about the cross-tree changes,
> does it need an immutable branch or could it go through driver-core?
> Catalin suggested removing the CC stable from patch 1/2 & adding it
> as a dependency for the 2/2 patch - but obviously that's up to the
> committer to sort out.

I'm finalising the arm64 queue today, so I don't really want to pull in
additional changes beyond critical fixes at this point, I'm afraid. I was
half-expecting a pull request from the riscv side last week but I didn't
see anything.

FWIW, if there's still no movement by -rc1, then I'm happy to queue all
of this on its own branch in the arm64 tree for 5.21.

Let me know.

Will

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

Re: [PATCH v4 0/2] Fix RISC-V's arch-topology reporting

[Conor.Dooley]

On 25/07/2022 10:13, Will Deacon wrote:
> On Sat, Jul 23, 2022 at 11:22:01AM +0000, Conor.Dooley@microchip.com wrote:
>> On 15/07/2022 18:51, Conor Dooley wrote:
>>
>> Hey,
>>
>> Not got any feedback on the smpboot changes from the RISC-V side.
>> I tested it on polarfire, the d1 (with both SMP & !SMP set iirc)
>> & on the u540. It all looked good to me.
>>
>> I'd like to have this fixed for v5.20, but there isn't too much
>> time left before the mw. Not too sure about the cross-tree changes,
>> does it need an immutable branch or could it go through driver-core?
>> Catalin suggested removing the CC stable from patch 1/2 & adding it
>> as a dependency for the 2/2 patch - but obviously that's up to the
>> committer to sort out.
> 
> I'm finalising the arm64 queue today, so I don't really want to pull in
> additional changes beyond critical fixes at this point, I'm afraid. I was
> half-expecting a pull request from the riscv side last week but I didn't
> see anything.

Yeah, that's fair. It's late in the game for cross-tree messing.
I know Palmer has been p busy recently.

> FWIW, if there's still no movement by -rc1, then I'm happy to queue all
> of this on its own branch in the arm64 tree for 5.21.

Hopefully someone on the riscv side will have confirmed what I am doing
is sane by then.

> 
> Let me know.

I will, thanks!
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 0/2] Fix RISC-V's arch-topology reporting

[Atish Patra]

On Mon, Jul 25, 2022 at 2:20 AM <Conor.Dooley@microchip.com> wrote:
>
> On 25/07/2022 10:13, Will Deacon wrote:
> > On Sat, Jul 23, 2022 at 11:22:01AM +0000, Conor.Dooley@microchip.com wrote:
> >> On 15/07/2022 18:51, Conor Dooley wrote:
> >>
> >> Hey,
> >>
> >> Not got any feedback on the smpboot changes from the RISC-V side.
> >> I tested it on polarfire, the d1 (with both SMP & !SMP set iirc)
> >> & on the u540. It all looked good to me.
> >>
> >> I'd like to have this fixed for v5.20, but there isn't too much
> >> time left before the mw. Not too sure about the cross-tree changes,
> >> does it need an immutable branch or could it go through driver-core?
> >> Catalin suggested removing the CC stable from patch 1/2 & adding it
> >> as a dependency for the 2/2 patch - but obviously that's up to the
> >> committer to sort out.
> >
> > I'm finalising the arm64 queue today, so I don't really want to pull in
> > additional changes beyond critical fixes at this point, I'm afraid. I was
> > half-expecting a pull request from the riscv side last week but I didn't
> > see anything.
>
> Yeah, that's fair. It's late in the game for cross-tree messing.
> I know Palmer has been p busy recently.
>
> > FWIW, if there's still no movement by -rc1, then I'm happy to queue all
> > of this on its own branch in the arm64 tree for 5.21.
>
> Hopefully someone on the riscv side will have confirmed what I am doing
> is sane by then.
>
> >
> > Let me know.
>
> I will, thanks!

Sorry for the delayed response here. I was planning to test the series
last week itself
but got dragged into something else and a qemu bug for NUMA.

Thanks for the fixes. I have tested this on Qemu(removing the topology
node) for the following configurations.
SMP, !SMP, NUMA (2 sockets)

FWIW,
Tested-by: Atish Patra <atishp@rivosinc.com>

-- 
Regards,
Atish

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

Re: [PATCH v4 0/2] Fix RISC-V's arch-topology reporting

[Conor.Dooley]

On 26/07/2022 09:12, Atish Patra wrote:
> 
> Sorry for the delayed response here. I was planning to test the series
> last week itself
> but got dragged into something else and a qemu bug for NUMA.

No worries, thanks for testing/reviewing it.

> 
> Thanks for the fixes. I have tested this on Qemu(removing the topology
> node) for the following configurations.
> SMP, !SMP, NUMA (2 sockets)

A NUMA test, great! I do need to get qemu set up properly..

> 
> FWIW,
> Tested-by: Atish Patra <atishp@rivosinc.com>

Thanks,
Conor.


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