| Message ID | 20220705190435.1790466-1-mail@conchuod.ie (mailing list archive) |
|---|---|
| Headers | show |
| Series | RISC-V: Add cpu-map topology information nodes | expand |
On Tue, Jul 05, 2022 at 08:04:31PM +0100, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > It was reported to me that the Hive Unmatched incorrectly reports > its topology to hwloc, but the StarFive VisionFive did in [0] & > a subsequent off-list email from Brice (the hwloc maintainer). > This turned out not to be entirely true, the /downstream/ version > of the VisionFive does work correctly but not upstream, as the > downstream devicetree has a cpu-map node that was added recently. > > This series adds a cpu-map node to all upstream devicetrees, which > I have tested on mpfs & fu540. The first patch is lifted directly > from the downstream StarFive devicetree. > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> I would recommend to have sane defaults in core risc-v code in case of absence of /cpu-map node as it is optional. The reason I mentioned is that Conor mentioned how the default values in absence of the node looked quite wrong. I don't know if it is possible on RISC-V but on ARM64 we do have default values if arch_topology fails to set based on DT/ACPI.
On 05/07/2022 21:19, Sudeep Holla wrote: > On Tue, Jul 05, 2022 at 08:04:31PM +0100, Conor Dooley wrote: >> From: Conor Dooley <conor.dooley@microchip.com> >> >> It was reported to me that the Hive Unmatched incorrectly reports >> its topology to hwloc, but the StarFive VisionFive did in [0] & >> a subsequent off-list email from Brice (the hwloc maintainer). >> This turned out not to be entirely true, the /downstream/ version >> of the VisionFive does work correctly but not upstream, as the >> downstream devicetree has a cpu-map node that was added recently. >> >> This series adds a cpu-map node to all upstream devicetrees, which >> I have tested on mpfs & fu540. The first patch is lifted directly >> from the downstream StarFive devicetree. >> > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > > I would recommend to have sane defaults in core risc-v code in case of > absence of /cpu-map node as it is optional. The reason I mentioned is that > Conor mentioned how the default values in absence of the node looked quite > wrong. I don't know if it is possible on RISC-V but on ARM64 we do have > default values if arch_topology fails to set based on DT/ACPI. > Yeah the defaults are all -1. I'll add some sane defaults for a v2. Thanks, Conor.
On 05/07/2022 21:33, Conor.Dooley@microchip.com wrote: > > > On 05/07/2022 21:19, Sudeep Holla wrote: >> On Tue, Jul 05, 2022 at 08:04:31PM +0100, Conor Dooley wrote: >>> From: Conor Dooley <conor.dooley@microchip.com> >>> >>> It was reported to me that the Hive Unmatched incorrectly reports >>> its topology to hwloc, but the StarFive VisionFive did in [0] & >>> a subsequent off-list email from Brice (the hwloc maintainer). >>> This turned out not to be entirely true, the /downstream/ version >>> of the VisionFive does work correctly but not upstream, as the >>> downstream devicetree has a cpu-map node that was added recently. >>> >>> This series adds a cpu-map node to all upstream devicetrees, which >>> I have tested on mpfs & fu540. The first patch is lifted directly >>> from the downstream StarFive devicetree. >>> >> >> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> >> >> I would recommend to have sane defaults in core risc-v code in case of >> absence of /cpu-map node as it is optional. The reason I mentioned is that >> Conor mentioned how the default values in absence of the node looked quite >> wrong. I don't know if it is possible on RISC-V but on ARM64 we do have >> default values if arch_topology fails to set based on DT/ACPI. >> > > Yeah the defaults are all -1. I'll add some sane defaults for a v2. > Thanks, > Conor. I shamelessly stole from arm64... Seems to work, but have done minimal testing (only PolarFire SoC). Author: Conor Dooley <conor.dooley@microchip.com> Date: Wed Jul 6 00:00:34 2022 +0100 riscv: arch-topology: add sane defaults RISC-V has no sane defaults to fall back on where there is no cpu-map in the devicetree. Add sane defaults in ~the exact same way as ARM64. Signed-off-by: Conor Dooley <conor.dooley@microchip.com> diff --git a/arch/riscv/include/asm/topology.h b/arch/riscv/include/asm/topology.h new file mode 100644 index 000000000000..71c80710f00e --- /dev/null +++ b/arch/riscv/include/asm/topology.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2022 Microchip Technology Inc. and its subsidiaries + */ + +#ifndef _ASM_RISCV_TOPOLOGY_H +#define _ASM_RISCV_TOPOLOGY_H + +#include <asm-generic/topology.h> + +void store_cpu_topology(unsigned int cpuid); + +#endif /* _ASM_RISCV_TOPOLOGY_H */ \ No newline at end of file diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile index c71d6591d539..9518882ba6f9 100644 --- a/arch/riscv/kernel/Makefile +++ b/arch/riscv/kernel/Makefile @@ -50,6 +50,7 @@ obj-y += riscv_ksyms.o obj-y += stacktrace.o obj-y += cacheinfo.o obj-y += patch.o +obj-y += topology.o obj-y += probes/ obj-$(CONFIG_MMU) += vdso.o vdso/ diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c index f1e4948a4b52..d551c7f452d4 100644 --- a/arch/riscv/kernel/smpboot.c +++ b/arch/riscv/kernel/smpboot.c @@ -32,6 +32,7 @@ #include <asm/sections.h> #include <asm/sbi.h> #include <asm/smp.h> +#include <asm/topology.h> #include "head.h" @@ -40,6 +41,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,6 +164,7 @@ 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); diff --git a/arch/riscv/kernel/topology.c b/arch/riscv/kernel/topology.c new file mode 100644 index 000000000000..799b3423e0bc --- /dev/null +++ b/arch/riscv/kernel/topology.c @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Based on the arm64 version, which was in turn based on arm32, which was + * ultimately based on sh's. + * The arm64 version was listed as: + * Copyright (C) 2011,2013,2014 Linaro Limited. + * + */ +#include <linux/arch_topology.h> +#include <linux/topology.h> +#include <asm/topology.h> + +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_info("CPU%u: cluster %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); +}
On Tue, Jul 05, 2022 at 08:33:39PM +0000, Conor.Dooley@microchip.com wrote: > > > On 05/07/2022 21:19, Sudeep Holla wrote: > > On Tue, Jul 05, 2022 at 08:04:31PM +0100, Conor Dooley wrote: > >> From: Conor Dooley <conor.dooley@microchip.com> > >> > >> It was reported to me that the Hive Unmatched incorrectly reports > >> its topology to hwloc, but the StarFive VisionFive did in [0] & > >> a subsequent off-list email from Brice (the hwloc maintainer). > >> This turned out not to be entirely true, the /downstream/ version > >> of the VisionFive does work correctly but not upstream, as the > >> downstream devicetree has a cpu-map node that was added recently. > >> > >> This series adds a cpu-map node to all upstream devicetrees, which > >> I have tested on mpfs & fu540. The first patch is lifted directly > >> from the downstream StarFive devicetree. > >> > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > > > > I would recommend to have sane defaults in core risc-v code in case of > > absence of /cpu-map node as it is optional. The reason I mentioned is that > > Conor mentioned how the default values in absence of the node looked quite > > wrong. I don't know if it is possible on RISC-V but on ARM64 we do have > > default values if arch_topology fails to set based on DT/ACPI. > > > > Yeah the defaults are all -1. I'll add some sane defaults for a v2. Sorry I didn't mean it to be part of this series. This series of DT changes are just fine on their own.
On Tue, Jul 05, 2022 at 11:03:54PM +0000, Conor.Dooley@microchip.com wrote: > > > On 05/07/2022 21:33, Conor.Dooley@microchip.com wrote: > > > > > > On 05/07/2022 21:19, Sudeep Holla wrote: > >> On Tue, Jul 05, 2022 at 08:04:31PM +0100, Conor Dooley wrote: > >>> From: Conor Dooley <conor.dooley@microchip.com> > >>> > >>> It was reported to me that the Hive Unmatched incorrectly reports > >>> its topology to hwloc, but the StarFive VisionFive did in [0] & > >>> a subsequent off-list email from Brice (the hwloc maintainer). > >>> This turned out not to be entirely true, the /downstream/ version > >>> of the VisionFive does work correctly but not upstream, as the > >>> downstream devicetree has a cpu-map node that was added recently. > >>> > >>> This series adds a cpu-map node to all upstream devicetrees, which > >>> I have tested on mpfs & fu540. The first patch is lifted directly > >>> from the downstream StarFive devicetree. > >>> > >> > >> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > >> > >> I would recommend to have sane defaults in core risc-v code in case of > >> absence of /cpu-map node as it is optional. The reason I mentioned is that > >> Conor mentioned how the default values in absence of the node looked quite > >> wrong. I don't know if it is possible on RISC-V but on ARM64 we do have > >> default values if arch_topology fails to set based on DT/ACPI. > >> > > > > Yeah the defaults are all -1. I'll add some sane defaults for a v2. > > Thanks, > > Conor. > > I shamelessly stole from arm64... Seems to work, but have done minimal > testing (only PolarFire SoC). > > Author: Conor Dooley <conor.dooley@microchip.com> > Date: Wed Jul 6 00:00:34 2022 +0100 > > riscv: arch-topology: add sane defaults > > RISC-V has no sane defaults to fall back on where there is no cpu-map > in the devicetree. Add sane defaults in ~the exact same way as ARM64. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > diff --git a/arch/riscv/include/asm/topology.h b/arch/riscv/include/asm/topology.h > new file mode 100644 > index 000000000000..71c80710f00e > --- /dev/null > +++ b/arch/riscv/include/asm/topology.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2022 Microchip Technology Inc. and its subsidiaries > + */ > + > +#ifndef _ASM_RISCV_TOPOLOGY_H > +#define _ASM_RISCV_TOPOLOGY_H > + > +#include <asm-generic/topology.h> > + > +void store_cpu_topology(unsigned int cpuid); > + > +#endif /* _ASM_RISCV_TOPOLOGY_H */ > \ No newline at end of file > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile > index c71d6591d539..9518882ba6f9 100644 > --- a/arch/riscv/kernel/Makefile > +++ b/arch/riscv/kernel/Makefile > @@ -50,6 +50,7 @@ obj-y += riscv_ksyms.o > obj-y += stacktrace.o > obj-y += cacheinfo.o > obj-y += patch.o > +obj-y += topology.o > obj-y += probes/ > obj-$(CONFIG_MMU) += vdso.o vdso/ > > diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c > index f1e4948a4b52..d551c7f452d4 100644 > --- a/arch/riscv/kernel/smpboot.c > +++ b/arch/riscv/kernel/smpboot.c > @@ -32,6 +32,7 @@ > #include <asm/sections.h> > #include <asm/sbi.h> > #include <asm/smp.h> > +#include <asm/topology.h> > > #include "head.h" > > @@ -40,6 +41,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,6 +164,7 @@ 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); > diff --git a/arch/riscv/kernel/topology.c b/arch/riscv/kernel/topology.c > new file mode 100644 > index 000000000000..799b3423e0bc > --- /dev/null > +++ b/arch/riscv/kernel/topology.c > @@ -0,0 +1,30 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Based on the arm64 version, which was in turn based on arm32, which was > + * ultimately based on sh's. > + * The arm64 version was listed as: > + * Copyright (C) 2011,2013,2014 Linaro Limited. > + * > + */ > +#include <linux/arch_topology.h> > +#include <linux/topology.h> > +#include <asm/topology.h> > + > +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_info("CPU%u: cluster %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); > +} > Looks good. Again package id is not cluster. This is what my series is addressing. So update the log as Package instead of Cluster above. The cluster id will be -1 unless you can get that for DT.
On 06/07/2022 10:21, Sudeep Holla wrote: > On Tue, Jul 05, 2022 at 11:03:54PM +0000, Conor.Dooley@microchip.com wrote: >> >> >> On 05/07/2022 21:33, Conor.Dooley@microchip.com wrote: >>> >>> >>> On 05/07/2022 21:19, Sudeep Holla wrote: >>>> On Tue, Jul 05, 2022 at 08:04:31PM +0100, Conor Dooley wrote: >>>>> From: Conor Dooley <conor.dooley@microchip.com> >>>>> >>>>> It was reported to me that the Hive Unmatched incorrectly reports >>>>> its topology to hwloc, but the StarFive VisionFive did in [0] & >>>>> a subsequent off-list email from Brice (the hwloc maintainer). >>>>> This turned out not to be entirely true, the /downstream/ version >>>>> of the VisionFive does work correctly but not upstream, as the >>>>> downstream devicetree has a cpu-map node that was added recently. >>>>> >>>>> This series adds a cpu-map node to all upstream devicetrees, which >>>>> I have tested on mpfs & fu540. The first patch is lifted directly >>>>> from the downstream StarFive devicetree. >>>>> >>>> >>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> >>>> >>>> I would recommend to have sane defaults in core risc-v code in case of >>>> absence of /cpu-map node as it is optional. The reason I mentioned is that >>>> Conor mentioned how the default values in absence of the node looked quite >>>> wrong. I don't know if it is possible on RISC-V but on ARM64 we do have >>>> default values if arch_topology fails to set based on DT/ACPI. >>>> >>> >>> Yeah the defaults are all -1. I'll add some sane defaults for a v2. >>> Thanks, >>> Conor. >> >> I shamelessly stole from arm64... Seems to work, but have done minimal >> testing (only PolarFire SoC). >> >> Author: Conor Dooley <conor.dooley@microchip.com> >> Date: Wed Jul 6 00:00:34 2022 +0100 >> >> riscv: arch-topology: add sane defaults >> >> RISC-V has no sane defaults to fall back on where there is no cpu-map >> in the devicetree. Add sane defaults in ~the exact same way as ARM64. >> >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> >> diff --git a/arch/riscv/include/asm/topology.h b/arch/riscv/include/asm/topology.h >> new file mode 100644 >> index 000000000000..71c80710f00e >> --- /dev/null >> +++ b/arch/riscv/include/asm/topology.h >> @@ -0,0 +1,13 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (c) 2022 Microchip Technology Inc. and its subsidiaries >> + */ >> + >> +#ifndef _ASM_RISCV_TOPOLOGY_H >> +#define _ASM_RISCV_TOPOLOGY_H >> + >> +#include <asm-generic/topology.h> >> + >> +void store_cpu_topology(unsigned int cpuid); >> + >> +#endif /* _ASM_RISCV_TOPOLOGY_H */ >> \ No newline at end of file >> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile >> index c71d6591d539..9518882ba6f9 100644 >> --- a/arch/riscv/kernel/Makefile >> +++ b/arch/riscv/kernel/Makefile >> @@ -50,6 +50,7 @@ obj-y += riscv_ksyms.o >> obj-y += stacktrace.o >> obj-y += cacheinfo.o >> obj-y += patch.o >> +obj-y += topology.o >> obj-y += probes/ >> obj-$(CONFIG_MMU) += vdso.o vdso/ >> >> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c >> index f1e4948a4b52..d551c7f452d4 100644 >> --- a/arch/riscv/kernel/smpboot.c >> +++ b/arch/riscv/kernel/smpboot.c >> @@ -32,6 +32,7 @@ >> #include <asm/sections.h> >> #include <asm/sbi.h> >> #include <asm/smp.h> >> +#include <asm/topology.h> >> >> #include "head.h" >> >> @@ -40,6 +41,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,6 +164,7 @@ 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); >> diff --git a/arch/riscv/kernel/topology.c b/arch/riscv/kernel/topology.c >> new file mode 100644 >> index 000000000000..799b3423e0bc >> --- /dev/null >> +++ b/arch/riscv/kernel/topology.c >> @@ -0,0 +1,30 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Based on the arm64 version, which was in turn based on arm32, which was >> + * ultimately based on sh's. >> + * The arm64 version was listed as: >> + * Copyright (C) 2011,2013,2014 Linaro Limited. >> + * >> + */ >> +#include <linux/arch_topology.h> >> +#include <linux/topology.h> >> +#include <asm/topology.h> >> + >> +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_info("CPU%u: cluster %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); >> +} >> > > Looks good. Again package id is not cluster. This is what my series is > addressing. So update the log as Package instead of Cluster above. The > cluster id will be -1 unless you can get that for DT. Cool, I'll respin a v2 without this included then & get the series backported since this is user visible & will fix all existing supported platforms. I'll send the topology code changes separately to avoid it going forwards. Thanks! Conor.
On Wed, Jul 06, 2022 at 09:43:02AM +0000, Conor.Dooley@microchip.com wrote: > > > On 06/07/2022 10:21, Sudeep Holla wrote: > > On Tue, Jul 05, 2022 at 11:03:54PM +0000, Conor.Dooley@microchip.com wrote: > >> > >> > >> On 05/07/2022 21:33, Conor.Dooley@microchip.com wrote: > >>> > >>> > >>> On 05/07/2022 21:19, Sudeep Holla wrote: > >>>> On Tue, Jul 05, 2022 at 08:04:31PM +0100, Conor Dooley wrote: > >>>>> From: Conor Dooley <conor.dooley@microchip.com> > >>>>> > >>>>> It was reported to me that the Hive Unmatched incorrectly reports > >>>>> its topology to hwloc, but the StarFive VisionFive did in [0] & > >>>>> a subsequent off-list email from Brice (the hwloc maintainer). > >>>>> This turned out not to be entirely true, the /downstream/ version > >>>>> of the VisionFive does work correctly but not upstream, as the > >>>>> downstream devicetree has a cpu-map node that was added recently. > >>>>> > >>>>> This series adds a cpu-map node to all upstream devicetrees, which > >>>>> I have tested on mpfs & fu540. The first patch is lifted directly > >>>>> from the downstream StarFive devicetree. > >>>>> > >>>> > >>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > >>>> > >>>> I would recommend to have sane defaults in core risc-v code in case of > >>>> absence of /cpu-map node as it is optional. The reason I mentioned is that > >>>> Conor mentioned how the default values in absence of the node looked quite > >>>> wrong. I don't know if it is possible on RISC-V but on ARM64 we do have > >>>> default values if arch_topology fails to set based on DT/ACPI. > >>>> > >>> > >>> Yeah the defaults are all -1. I'll add some sane defaults for a v2. > >>> Thanks, > >>> Conor. > >> > >> I shamelessly stole from arm64... Seems to work, but have done minimal > >> testing (only PolarFire SoC). > >> > >> Author: Conor Dooley <conor.dooley@microchip.com> > >> Date: Wed Jul 6 00:00:34 2022 +0100 > >> > >> riscv: arch-topology: add sane defaults > >> > >> RISC-V has no sane defaults to fall back on where there is no cpu-map > >> in the devicetree. Add sane defaults in ~the exact same way as ARM64. > >> > >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > >> > >> diff --git a/arch/riscv/include/asm/topology.h b/arch/riscv/include/asm/topology.h > >> new file mode 100644 > >> index 000000000000..71c80710f00e > >> --- /dev/null > >> +++ b/arch/riscv/include/asm/topology.h > >> @@ -0,0 +1,13 @@ > >> +/* SPDX-License-Identifier: GPL-2.0-only */ > >> +/* > >> + * Copyright (c) 2022 Microchip Technology Inc. and its subsidiaries > >> + */ > >> + > >> +#ifndef _ASM_RISCV_TOPOLOGY_H > >> +#define _ASM_RISCV_TOPOLOGY_H > >> + > >> +#include <asm-generic/topology.h> > >> + > >> +void store_cpu_topology(unsigned int cpuid); > >> + > >> +#endif /* _ASM_RISCV_TOPOLOGY_H */ > >> \ No newline at end of file > >> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile > >> index c71d6591d539..9518882ba6f9 100644 > >> --- a/arch/riscv/kernel/Makefile > >> +++ b/arch/riscv/kernel/Makefile > >> @@ -50,6 +50,7 @@ obj-y += riscv_ksyms.o > >> obj-y += stacktrace.o > >> obj-y += cacheinfo.o > >> obj-y += patch.o > >> +obj-y += topology.o > >> obj-y += probes/ > >> obj-$(CONFIG_MMU) += vdso.o vdso/ > >> > >> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c > >> index f1e4948a4b52..d551c7f452d4 100644 > >> --- a/arch/riscv/kernel/smpboot.c > >> +++ b/arch/riscv/kernel/smpboot.c > >> @@ -32,6 +32,7 @@ > >> #include <asm/sections.h> > >> #include <asm/sbi.h> > >> #include <asm/smp.h> > >> +#include <asm/topology.h> > >> > >> #include "head.h" > >> > >> @@ -40,6 +41,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,6 +164,7 @@ 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); > >> diff --git a/arch/riscv/kernel/topology.c b/arch/riscv/kernel/topology.c > >> new file mode 100644 > >> index 000000000000..799b3423e0bc > >> --- /dev/null > >> +++ b/arch/riscv/kernel/topology.c > >> @@ -0,0 +1,30 @@ > >> +// SPDX-License-Identifier: GPL-2.0-only > >> +/* > >> + * Based on the arm64 version, which was in turn based on arm32, which was > >> + * ultimately based on sh's. > >> + * The arm64 version was listed as: > >> + * Copyright (C) 2011,2013,2014 Linaro Limited. > >> + * > >> + */ > >> +#include <linux/arch_topology.h> > >> +#include <linux/topology.h> > >> +#include <asm/topology.h> > >> + > >> +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_info("CPU%u: cluster %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); > >> +} > >> > > > > Looks good. Again package id is not cluster. This is what my series is > > addressing. So update the log as Package instead of Cluster above. The > > cluster id will be -1 unless you can get that for DT. > > > Cool, I'll respin a v2 without this included then & get the series > backported since this is user visible & will fix all existing supported > platforms. > The DT /cpu-map is optional, so I don't think it needs to be backported. > > I'll send the topology code changes separately to avoid it going > forwards. This on the other hand can be backported to fix userspace.
On 06/07/2022 11:03, Sudeep Holla wrote: > On Wed, Jul 06, 2022 at 09:43:02AM +0000, Conor.Dooley@microchip.com wrote: >> Cool, I'll respin a v2 without this included then & get the series >> backported since this is user visible & will fix all existing supported >> platforms. >> > > The DT /cpu-map is optional, so I don't think it needs to be backported. > >> >> I'll send the topology code changes separately to avoid it going >> forwards. > > This on the other hand can be backported to fix userspace. > Uh sure, that sounds like a better idea (but more work lol). No need for a v2 of this series then. Thanks again :)
On 06/07/2022 10:21, Sudeep Holla wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Tue, Jul 05, 2022 at 11:03:54PM +0000, Conor.Dooley@microchip.com wrote: >> >> >> On 05/07/2022 21:33, Conor.Dooley@microchip.com wrote: >>> >>> >>> On 05/07/2022 21:19, Sudeep Holla wrote: >>>> On Tue, Jul 05, 2022 at 08:04:31PM +0100, Conor Dooley wrote: >>>>> From: Conor Dooley <conor.dooley@microchip.com> >>>>> >>>>> It was reported to me that the Hive Unmatched incorrectly reports >>>>> its topology to hwloc, but the StarFive VisionFive did in [0] & >>>>> a subsequent off-list email from Brice (the hwloc maintainer). >>>>> This turned out not to be entirely true, the /downstream/ version >>>>> of the VisionFive does work correctly but not upstream, as the >>>>> downstream devicetree has a cpu-map node that was added recently. >>>>> >>>>> This series adds a cpu-map node to all upstream devicetrees, which >>>>> I have tested on mpfs & fu540. The first patch is lifted directly >>>>> from the downstream StarFive devicetree. >>>>> >>>> >>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> >>>> >>>> I would recommend to have sane defaults in core risc-v code in case of >>>> absence of /cpu-map node as it is optional. The reason I mentioned is that >>>> Conor mentioned how the default values in absence of the node looked quite >>>> wrong. I don't know if it is possible on RISC-V but on ARM64 we do have >>>> default values if arch_topology fails to set based on DT/ACPI. >>>> >>> >>> Yeah the defaults are all -1. I'll add some sane defaults for a v2. >>> Thanks, >>> Conor. >> >> I shamelessly stole from arm64... Seems to work, but have done minimal >> testing (only PolarFire SoC). >> >> Author: Conor Dooley <conor.dooley@microchip.com> >> Date: Wed Jul 6 00:00:34 2022 +0100 >> >> riscv: arch-topology: add sane defaults >> >> RISC-V has no sane defaults to fall back on where there is no cpu-map >> in the devicetree. Add sane defaults in ~the exact same way as ARM64. >> >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> >> diff --git a/arch/riscv/include/asm/topology.h b/arch/riscv/include/asm/topology.h >> new file mode 100644 >> index 000000000000..71c80710f00e >> --- /dev/null >> +++ b/arch/riscv/include/asm/topology.h >> @@ -0,0 +1,13 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (c) 2022 Microchip Technology Inc. and its subsidiaries >> + */ >> + >> +#ifndef _ASM_RISCV_TOPOLOGY_H >> +#define _ASM_RISCV_TOPOLOGY_H >> + >> +#include <asm-generic/topology.h> >> + >> +void store_cpu_topology(unsigned int cpuid); >> + >> +#endif /* _ASM_RISCV_TOPOLOGY_H */ >> \ No newline at end of file >> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile >> index c71d6591d539..9518882ba6f9 100644 >> --- a/arch/riscv/kernel/Makefile >> +++ b/arch/riscv/kernel/Makefile >> @@ -50,6 +50,7 @@ obj-y += riscv_ksyms.o >> obj-y += stacktrace.o >> obj-y += cacheinfo.o >> obj-y += patch.o >> +obj-y += topology.o >> obj-y += probes/ >> obj-$(CONFIG_MMU) += vdso.o vdso/ >> >> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c >> index f1e4948a4b52..d551c7f452d4 100644 >> --- a/arch/riscv/kernel/smpboot.c >> +++ b/arch/riscv/kernel/smpboot.c >> @@ -32,6 +32,7 @@ >> #include <asm/sections.h> >> #include <asm/sbi.h> >> #include <asm/smp.h> >> +#include <asm/topology.h> >> >> #include "head.h" >> >> @@ -40,6 +41,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,6 +164,7 @@ 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); >> diff --git a/arch/riscv/kernel/topology.c b/arch/riscv/kernel/topology.c >> new file mode 100644 >> index 000000000000..799b3423e0bc >> --- /dev/null >> +++ b/arch/riscv/kernel/topology.c >> @@ -0,0 +1,30 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Based on the arm64 version, which was in turn based on arm32, which was >> + * ultimately based on sh's. >> + * The arm64 version was listed as: >> + * Copyright (C) 2011,2013,2014 Linaro Limited. >> + * >> + */ >> +#include <linux/arch_topology.h> >> +#include <linux/topology.h> >> +#include <asm/topology.h> >> + >> +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_info("CPU%u: cluster %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); >> +} >> > > Looks good. Again package id is not cluster. This is what my series is > addressing. So update the log as Package instead of Cluster above. The > cluster id will be -1 unless you can get that for DT. > FYI I took that directly from arm64: arch/arm64/kernel/topology.c:L57 (next-20220706) 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);
On Wed, Jul 06, 2022 at 01:04:24PM +0000, Conor.Dooley@microchip.com wrote: > On 06/07/2022 10:21, Sudeep Holla wrote: [...] > > Looks good. Again package id is not cluster. This is what my series is > > addressing. So update the log as Package instead of Cluster above. The > > cluster id will be -1 unless you can get that for DT. > > > > FYI I took that directly from arm64: > arch/arm64/kernel/topology.c:L57 (next-20220706) > > 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); > Yikes, that needs to change. I will get that updates. Thanks for pointing that out.
From: Conor Dooley <conor.dooley@microchip.com> On Tue, 5 Jul 2022 20:04:31 +0100, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > It was reported to me that the Hive Unmatched incorrectly reports > its topology to hwloc, but the StarFive VisionFive did in [0] & > a subsequent off-list email from Brice (the hwloc maintainer). > This turned out not to be entirely true, the /downstream/ version > of the VisionFive does work correctly but not upstream, as the > downstream devicetree has a cpu-map node that was added recently. > > [...] Applied to dt-for-next, thanks! [4/5] riscv: dts: microchip: Add mpfs' topology information https://git.kernel.org/conor/c/88d319c6abae The rest is yours Palmer once reviewed :) Thanks, Conor.
From: Conor Dooley <conor.dooley@microchip.com> It was reported to me that the Hive Unmatched incorrectly reports its topology to hwloc, but the StarFive VisionFive did in [0] & a subsequent off-list email from Brice (the hwloc maintainer). This turned out not to be entirely true, the /downstream/ version of the VisionFive does work correctly but not upstream, as the downstream devicetree has a cpu-map node that was added recently. This series adds a cpu-map node to all upstream devicetrees, which I have tested on mpfs & fu540. The first patch is lifted directly from the downstream StarFive devicetree. Thanks, Conor. 0: https://github.com/open-mpi/hwloc/issues/536 Conor Dooley (4): riscv: dts: sifive: Add fu540 topology information riscv: dts: sifive: Add fu740 topology information riscv: dts: microchip: Add mpfs' topology information riscv: dts: canaan: Add k210 topology information Jonas Hahnfeld (1): riscv: dts: starfive: Add JH7100 CPU topology arch/riscv/boot/dts/canaan/k210.dtsi | 12 +++++++++++ arch/riscv/boot/dts/microchip/mpfs.dtsi | 24 ++++++++++++++++++++++ arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 24 ++++++++++++++++++++++ arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 24 ++++++++++++++++++++++ arch/riscv/boot/dts/starfive/jh7100.dtsi | 16 +++++++++++++-- 5 files changed, 98 insertions(+), 2 deletions(-) base-commit: b6f1f2fa2bddd69ff46a190b8120bd440fd50563