linux-kernel - Re: [PATCH 0/5] RISC-V: Add cpu-map topology information nodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9f26cd2a-b389-3f4b-b7d2-06fa5c2d7016@microchip.com>
Date:   Wed, 6 Jul 2022 09:43:02 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <sudeep.holla@....com>, <Conor.Dooley@...rochip.com>
CC:     <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <paul.walmsley@...ive.com>, <palmer@...belt.com>,
        <aou@...s.berkeley.edu>, <Daire.McNamara@...rochip.com>,
        <niklas.cassel@....com>, <damien.lemoal@...nsource.wdc.com>,
        <geert@...ux-m68k.org>, <zong.li@...ive.com>, <kernel@...il.dk>,
        <hahnjo@...njo.de>, <devicetree@...r.kernel.org>,
        <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <Brice.Goglin@...ia.fr>
Subject: Re: [PATCH 0/5] RISC-V: Add cpu-map topology information nodes



On 06/07/2022 10:21, Sudeep Holla wrote:
> On Tue, Jul 05, 2022 at 11:03:54PM +0000, Conor.Dooley@...rochip.com wrote:
>>
>>
>> On 05/07/2022 21:33, Conor.Dooley@...rochip.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@...rochip.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@....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@...rochip.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@...rochip.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.