Message ID | 20230802101934.258937135@linutronix.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f41:0:b0:3e4:2afc:c1 with SMTP id v1csp409351vqx; Wed, 2 Aug 2023 05:13:08 -0700 (PDT) X-Google-Smtp-Source: APBJJlFA8CDIjf4yOyqhmXctSB8HxWRMWmWxAAfy8GUmuVFNk+7fl+4/hi6KBPROJvet0HiF1fvO X-Received: by 2002:a05:6a20:938b:b0:13e:f5b5:48ee with SMTP id x11-20020a056a20938b00b0013ef5b548eemr2243557pzh.59.1690978388355; Wed, 02 Aug 2023 05:13:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690978388; cv=none; d=google.com; s=arc-20160816; b=0Xw78PW4JDfmpCa1UdpxMKpwzrPsMAfw1OrPe7xsirhvtFqIJmmzGeBzDXck1OdzfI I7TI9H1uG6yKlj5116cwJbRhXTdW/hk5UUYxw8Esg3nkwJDJHCj46FgPxlgVnFtmHFDR RVBAbi0zxsTaXxr4nVuOC8r79d370UV0tXNNbhDlWJ7tMUGMR+hR1K7jMw3Iyz19aS9a Zpv+hgfWwxm3ypM/chbKShxNb65iEZI4ZucyITviIGDWNKgAMoxusVj3mk5ks4v7Tp2u ycCvAPDhS5lyINP+VA7bjY1tzROQDsMY8zg8UVI9zua+Hb5aKBgS+/VywvHXVzswXi9f gycA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:date:mime-version:references:subject:cc:to:from :dkim-signature:dkim-signature:message-id; bh=K8G3BegNCqpNmXITgAr+wce0GBx04jSPNL1AU9AAQFg=; fh=O9JPtEgoXN20WvvffwC9vht4C9mlV4cjm0SKbEbqoG0=; b=bBJQlQzjQNNsRaEl0LZF5xpT3VW4TbHYYSmWcGMrLgnsKyhZels6mEuWb6FIZ/7Bc+ 8Clpj4UnVbGffnUtEwuElzfZUbYvbbvbF2KmyOgtKV4Kin50nyBgaWB1QphL1Rgvk4K7 554Fdf9ssopk96xaXHWAmCFH3/GglrkZPu3Yz2P2R9PYJXHRq33X24BPTxufO0DR+JuK g1M3GM/smHe9xfR6Xu9T2MiaTesPAUR0CDVYYK2Qhdf4DzNeAUIi2nCYP4g7QeCdYa5h Heo9cMZLdqwRWZN3zFPej4eJea3xpwVJ0SrJHgl/ZVs6Ju6gpfy0hpoGCDDnT+REZuby VIDQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linutronix.de header.s=2020 header.b=w9vxYKjt; dkim=neutral (no key) header.i=@linutronix.de header.s=2020e header.b=UJBJjCKc; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=linutronix.de Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id bs184-20020a6328c1000000b005633d19281bsi10563804pgb.401.2023.08.02.05.12.50; Wed, 02 Aug 2023 05:13:08 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linutronix.de header.s=2020 header.b=w9vxYKjt; dkim=neutral (no key) header.i=@linutronix.de header.s=2020e header.b=UJBJjCKc; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=linutronix.de Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233378AbjHBKYA (ORCPT <rfc822;maxi.paulin@gmail.com> + 99 others); Wed, 2 Aug 2023 06:24:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43166 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231612AbjHBKXK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 2 Aug 2023 06:23:10 -0400 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A83E626B6 for <linux-kernel@vger.kernel.org>; Wed, 2 Aug 2023 03:21:55 -0700 (PDT) Message-ID: <20230802101934.258937135@linutronix.de> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1690971700; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: references:references; bh=K8G3BegNCqpNmXITgAr+wce0GBx04jSPNL1AU9AAQFg=; b=w9vxYKjt5/a2EMtxkZ89rejhVCq4lEmr1Uqx3saIIyeVaEx6lADJRztUMpFiP+QSFVHZc9 xU3gg/W3sbt/fqErnHwjPBnrusHmcss9fo9MnMrMgFUJellh9E9DBLRWEyEr0sfC1HK+sb xwgUCnNASFa56bx4zXVscPOS/cBhurD/vxoDLrRjn7KjseFiVJZrKxSLCmSITTvNwLraez GFBPu0zGg/vC5Avoh/UyiSYxObMwqUNfV+YNyltRjAml2rKPzqXRj5J3Hq7KYP3bfrKZ3r Nr/fqNRmwnKoTEn0H6bOWs8JFRRVnE1InsU7eLmabpYOUci8zJcrfigocBBRHg== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1690971700; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: references:references; bh=K8G3BegNCqpNmXITgAr+wce0GBx04jSPNL1AU9AAQFg=; b=UJBJjCKc7jrzSAUdDDRKu85O6xzgoSIADLOth52EW0WhDR13hhHMx2Vp+Erc55z+TJ56mC KmXbVv5Oo2U3b1Bg== From: Thomas Gleixner <tglx@linutronix.de> To: LKML <linux-kernel@vger.kernel.org> Cc: x86@kernel.org, Tom Lendacky <thomas.lendacky@amd.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Arjan van de Ven <arjan@linux.intel.com>, Huang Rui <ray.huang@amd.com>, Juergen Gross <jgross@suse.com>, Dimitri Sivanich <dimitri.sivanich@hpe.com>, Michael Kelley <mikelley@microsoft.com>, Wei Liu <wei.liu@kernel.org> Subject: [patch V3 27/40] x86/cpu: Provide a sane leaf 0xb/0x1f parser References: <20230802101635.459108805@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Date: Wed, 2 Aug 2023 12:21:40 +0200 (CEST) X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1773119354595317267 X-GMAIL-MSGID: 1773119354595317267 |
Series |
x86/cpu: Rework the topology evaluation
|
|
Commit Message
Thomas Gleixner
Aug. 2, 2023, 10:21 a.m. UTC
detect_extended_topology() along with it's early() variant is a classic
example for duct tape engineering:
- It evaluates an array of subleafs with a boatload of local variables
for the relevant topology levels instead of using an array to save the
enumerated information and propagate it to the right level
- It has no boundary checks for subleafs
- It prevents updating the die_id with a crude workaround instead of
checking for leaf 0xb which does not provide die information.
- It's broken vs. the number of dies evaluation as it uses:
num_processors[DIE_LEVEL] / num_processors[CORE_LEVEL]
which "works" only correctly if there is none of the intermediate
topology levels (MODULE/TILE) enumerated.
There is zero value in trying to "fix" that code as the only proper fix is
to rewrite it from scratch.
Implement a sane parser with proper code documentation, which will be used
for the consolidated topology evaluation in the next step.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
V2: Fixed up the comment alignment for registers - Peterz
---
arch/x86/kernel/cpu/Makefile | 2
arch/x86/kernel/cpu/topology.h | 12 +++
arch/x86/kernel/cpu/topology_ext.c | 136 +++++++++++++++++++++++++++++++++++++
3 files changed, 149 insertions(+), 1 deletion(-)
Comments
Hi, Thomas, On Wed, 2023-08-02 at 12:21 +0200, Thomas Gleixner wrote: > detect_extended_topology() along with it's early() variant is a > classic > example for duct tape engineering: > > - It evaluates an array of subleafs with a boatload of local > variables > for the relevant topology levels instead of using an array to > save the > enumerated information and propagate it to the right level > > - It has no boundary checks for subleafs > > - It prevents updating the die_id with a crude workaround instead > of > checking for leaf 0xb which does not provide die information. > > - It's broken vs. the number of dies evaluation as it uses: > > num_processors[DIE_LEVEL] / num_processors[CORE_LEVEL] > > which "works" only correctly if there is none of the intermediate > topology levels (MODULE/TILE) enumerated. > > There is zero value in trying to "fix" that code as the only proper > fix is > to rewrite it from scratch. > > Implement a sane parser with proper code documentation, which will be > used > for the consolidated topology evaluation in the next step. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > V2: Fixed up the comment alignment for registers - Peterz > --- > arch/x86/kernel/cpu/Makefile | 2 > arch/x86/kernel/cpu/topology.h | 12 +++ > arch/x86/kernel/cpu/topology_ext.c | 136 > +++++++++++++++++++++++++++++++++++++ > 3 files changed, 149 insertions(+), 1 deletion(-) > > --- a/arch/x86/kernel/cpu/Makefile > +++ b/arch/x86/kernel/cpu/Makefile > @@ -18,7 +18,7 @@ KMSAN_SANITIZE_common.o := n > KCSAN_SANITIZE_common.o := n > > obj-y := cacheinfo.o scattered.o > -obj-y += topology_common.o topology.o > +obj-y += topology_common.o topology_ext.o > topology.o > obj-y += common.o > obj-y += rdrand.o > obj-y += match.o > --- a/arch/x86/kernel/cpu/topology.h > +++ b/arch/x86/kernel/cpu/topology.h > @@ -16,6 +16,7 @@ void cpu_init_topology(struct cpuinfo_x8 > void cpu_parse_topology(struct cpuinfo_x86 *c); > void topology_set_dom(struct topo_scan *tscan, enum > x86_topology_domains dom, > unsigned int shift, unsigned int ncpus); > +bool cpu_parse_topology_ext(struct topo_scan *tscan); > > static inline u32 topo_shift_apicid(u32 apicid, enum > x86_topology_domains dom) > { > @@ -31,4 +32,15 @@ static inline u32 topo_relative_domain_i > return apicid & (x86_topo_system.dom_size[dom] - 1); > } > > +/* > + * Update a domain level after the fact without propagating. Used to > fixup > + * broken CPUID enumerations. > + */ > +static inline void topology_update_dom(struct topo_scan *tscan, enum > x86_topology_domains dom, > + unsigned int shift, unsigned > int ncpus) > +{ > + tscan->dom_shifts[dom] = shift; > + tscan->dom_ncpus[dom] = ncpus; > +} > + > #endif /* ARCH_X86_TOPOLOGY_H */ > --- /dev/null > +++ b/arch/x86/kernel/cpu/topology_ext.c > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/cpu.h> > + > +#include <asm/apic.h> > +#include <asm/memtype.h> > +#include <asm/processor.h> > + > +#include "cpu.h" > + > +enum topo_types { > + INVALID_TYPE = 0, > + SMT_TYPE = 1, > + CORE_TYPE = 2, > + MODULE_TYPE = 3, > + TILE_TYPE = 4, > + DIE_TYPE = 5, > + DIEGRP_TYPE = 6, > + MAX_TYPE = 7, > +}; > + > +/* > + * Use a lookup table for the case that there are future types > 6 > which > + * describe an intermediate domain level which does not exist today. > + * > + * A table will also be handy to parse the new AMD 0x80000026 leaf > which > + * has defined different domain types, but otherwise uses the same > layout > + * with some of the reserved bits used for new information. > + */ > +static const unsigned int topo_domain_map[MAX_TYPE] = { > + [SMT_TYPE] = TOPO_SMT_DOMAIN, > + [CORE_TYPE] = TOPO_CORE_DOMAIN, > + [MODULE_TYPE] = TOPO_MODULE_DOMAIN, > + [TILE_TYPE] = TOPO_TILE_DOMAIN, > + [DIE_TYPE] = TOPO_DIE_DOMAIN, > + [DIEGRP_TYPE] = TOPO_PKG_DOMAIN, May I know why DIEGRP_TYPE is mapped to TOPO_PKG_DOMAIN? > +}; > + > +static inline bool topo_subleaf(struct topo_scan *tscan, u32 leaf, > u32 subleaf) > +{ > + unsigned int dom, maxtype = leaf == 0xb ? CORE_TYPE + 1 : > MAX_TYPE; > + struct { > + // eax > + u32 x2apic_shift : 5, // Number of bits to > shift APIC ID right > + // for the topology ID > at the next level > + __rsvd0 : 27; // Reserved > + // ebx > + u32 num_processors : 16, // Number of processors > at current level > + __rsvd1 : 16; // Reserved > + // ecx > + u32 level : 8, // Current topology > level. Same as sub leaf number > + type : 8, // Level type. If 0, > invalid > + __rsvd2 : 16; // Reserved > + // edx > + u32 x2apic_id : 32; // X2APIC ID of the > current logical processor > + } sl; > + > + cpuid_subleaf(leaf, subleaf, &sl); > + > + if (!sl.num_processors || sl.type == INVALID_TYPE) > + return false; > + > + if (sl.type >= maxtype) { It is still legal to have sparse type value in the future, and then this check will break. IMO, it is better to use a function to convert type to domain, and check for unknown domain here, say, something like diff --git a/arch/x86/kernel/cpu/topology_ext.c b/arch/x86/kernel/cpu/topology_ext.c index 5ddc5d24435e..7720a7bc7478 100644 --- a/arch/x86/kernel/cpu/topology_ext.c +++ b/arch/x86/kernel/cpu/topology_ext.c @@ -26,14 +26,27 @@ enum topo_types { * has defined different domain types, but otherwise uses the same layout * with some of the reserved bits used for new information. */ -static const unsigned int topo_domain_map[MAX_TYPE] = { - [SMT_TYPE] = TOPO_SMT_DOMAIN, - [CORE_TYPE] = TOPO_CORE_DOMAIN, - [MODULE_TYPE] = TOPO_MODULE_DOMAIN, - [TILE_TYPE] = TOPO_TILE_DOMAIN, - [DIE_TYPE] = TOPO_DIE_DOMAIN, - [DIEGRP_TYPE] = TOPO_PKG_DOMAIN, -}; + +static enum x86_topology_domains topo_type_to_domain(int type) +{ + switch (type) { + case SMT_TYPE: + return TOPO_SMT_DOMAIN; + case CORE_TYPE: + return TOPO_CORE_DOMAIN; + case MODULE_TYPE: + return TOPO_MODULE_DOMAIN; + case TILE_TYPE: + return TOPO_TILE_DOMAIN; + case DIE_TYPE: + return TOPO_DIE_DOMAIN; + case DIEGRP_TYPE: + return TOPO_PKG_DOMAIN; + default: + return TOPO_MAX_DOMAIN; + } + +} static inline bool topo_subleaf(struct topo_scan *tscan, u32 leaf, u32 subleaf) { @@ -59,7 +72,8 @@ static inline bool topo_subleaf(struct topo_scan *tscan, u32 leaf, u32 subleaf) if (!sl.num_processors || sl.type == INVALID_TYPE) return false; - if (sl.type >= maxtype) { + dom = topo_type_to_domain(sl.type); + if (dom == TOPO_MAX_DOMAIN) { /* * As the subleafs are ordered in domain level order, this * could be recovered in theory by propagating the @@ -84,7 +98,6 @@ static inline bool topo_subleaf(struct topo_scan *tscan, u32 leaf, u32 subleaf) return true; } - dom = topo_domain_map[sl.type]; if (!dom) { tscan->c->topo.initial_apicid = sl.x2apic_id; } else if (tscan->c->topo.initial_apicid != sl.x2apic_id) { > + /* > + * As the subleafs are ordered in domain level order, > this > + * could be recovered in theory by propagating the > + * information at the last parsed level. > + * > + * But if the infinite wisdom of hardware folks > decides to > + * create a new domain type between CORE and MODULE > or DIE > + * and DIEGRP, then that would overwrite the CORE or > DIE > + * information. Sorry that I'm confused here. Say, we have CORE, FOO, MODULE, then the subleave of FOO must be higher than CORE but lower than MODULE. so we parse CORE first and propagates the info to FOO/MODULE, and then parse FOO and propagate to MODULE, and parse MODULE in the end. How could we overwrite the info of a lower level? > + * > + * It really would have been too obvious to make the > domain > + * type space sparse and leave a few reserved types > between > + * the points which might change instead of forcing > + * software to either create a monstrosity of > workarounds > + * or just being up the creek without a paddle. Agreed. with sparse type space, we know the relationship between different types, without knowing what the type really means. > + * > + * Refuse to implement monstrosity, emit an error and try > + * to survive. > + */ > + pr_err_once("Topology: leaf 0x%x:%d Unknown domain type %u\n", > + leaf, subleaf, sl.type); > + return true; Don't want to be TLDR, I can think of a couple cases that breaks Linux in different ways if we ignore the cpu topology info of an unknown level. So I just want to understand the strategy here, does this mean that we're not looking for a future proof solution, and instead we are planning to take future updates (patch enum topo_types/enum x86_topology_domains/topo_domain_map) whenever a new level is invented? TBH, I'm still thinking of a future proof proposal here. currently, Linux only cares about pkg_id/core_id/die_id, and the relationship between these three levels. 1. for package id: pkg_id_low = FOO.x2apic_shift (FOO is the highest enumerated level, no matter its type is known or not) 2. for core_id: as SMT level is always enumerated, core_id_low = SMT.x2apic_shift, core_id_high = pkg_id_low - 1; 3. for die_id: Make Linux Die *OPTIONAL*. when DIE is enumerated via CPUID.1F, die_id_low = FOO.x2apic_shift (FOO is the next enumerated lower level of DIE, no matter its type is known or not), die_id_high = pkg_id_low - 1; when DIE is not enumerated via CPUID.1F, then Linux die does not exist, adjust the die related topology information, say, die_id = -1, topology_max_dies_per_package = 0, etc, and don't expose die sysfs I/F. With this, we can guarantee that all the available topology information are always valid, even when running on future platforms. what do you think? thanks, rui
On Sat, Aug 12 2023 at 08:21, Rui Zhang wrote: > On Wed, 2023-08-02 at 12:21 +0200, Thomas Gleixner wrote: >> + >> +/* >> + * Use a lookup table for the case that there are future types > 6 >> which >> + * describe an intermediate domain level which does not exist today. >> + * >> + * A table will also be handy to parse the new AMD 0x80000026 leaf >> which >> + * has defined different domain types, but otherwise uses the same >> layout >> + * with some of the reserved bits used for new information. >> + */ >> +static const unsigned int topo_domain_map[MAX_TYPE] = { >> + [SMT_TYPE] = TOPO_SMT_DOMAIN, >> + [CORE_TYPE] = TOPO_CORE_DOMAIN, >> + [MODULE_TYPE] = TOPO_MODULE_DOMAIN, >> + [TILE_TYPE] = TOPO_TILE_DOMAIN, >> + [DIE_TYPE] = TOPO_DIE_DOMAIN, >> + [DIEGRP_TYPE] = TOPO_PKG_DOMAIN, > > May I know why DIEGRP_TYPE is mapped to TOPO_PKG_DOMAIN? Where else should it go? It's the topmost, no? But diegrp is a terminoligy which is not used in the kernel >> + >> + if (sl.type >= maxtype) { > > It is still legal to have sparse type value in the future, and then > this check will break. > IMO, it is better to use a function to convert type to domain, and > check for unknown domain here, say, something like Why? If somewhere in the future Intel decides to add UBER_TILE_TYPE, then this will be a type larger than DIEGRP_TYPE. maxtype will then cover the whole thing and the table will map it to the right place. Even if in their infinite wisdom the HW folks decide to make a gap, then the table can handle it simply by putting an invalid value into the gap and checking for that. Serioulsy we don't need a switch case for that. >> + /* >> + * As the subleafs are ordered in domain level order, >> this >> + * could be recovered in theory by propagating the >> + * information at the last parsed level. >> + * >> + * But if the infinite wisdom of hardware folks >> decides to >> + * create a new domain type between CORE and MODULE >> or DIE >> + * and DIEGRP, then that would overwrite the CORE or >> DIE >> + * information. > > Sorry that I'm confused here. > > Say, we have CORE, FOO, MODULE, then the subleave of FOO must be higher > than CORE but lower than MODULE. > so we parse CORE first and propagates the info to FOO/MODULE, and then > parse FOO and propagate to MODULE, and parse MODULE in the end. > How could we overwrite the info of a lower level? We don't know about this new thing yet. So where should we propagate to? We could say, last was core so we stick the new thing into module, but do we know that's correct? Do we know there is a module actually. Let me rephrase that comment. >> + * >> + * Refuse to implement monstrosity, emit an error and try >> + * to survive. >> + */ >> + pr_err_once("Topology: leaf 0x%x:%d Unknown domain type %u\n", >> + leaf, subleaf, sl.type); >> + return true; > > Don't want to be TLDR, I can think of a couple cases that breaks Linux > in different ways if we ignore the cpu topology info of an unknown > level. Come on. If Intel manages it to create a new level then it's not rocket science to integrate support for that a long time before actual silicon ships. So what will break? The machines of people who use ancient kernels on modern hardware? They can keep the pieces. > So I just want to understand the strategy here, does this mean that > we're not looking for a future proof solution, and instead we are > planning to take future updates (patch enum topo_types/enum > x86_topology_domains/topo_domain_map) whenever a new level is > invented? You need that anyway, no? > With this, we can guarantee that all the available topology information > are always valid, even when running on future platforms. I know that it can be made work, but is it worth the extra effort? I don't think so. Thanks, tglx
On Sat, Aug 12 2023 at 22:04, Thomas Gleixner wrote: > On Sat, Aug 12 2023 at 08:21, Rui Zhang wrote: >> With this, we can guarantee that all the available topology information >> are always valid, even when running on future platforms. > > I know that it can be made work, but is it worth the extra effort? I > don't think so. So I thought more about it. For intermediate levels, i.e. something which is squeezed between two existing levels, this works by some definition of works. I.e. the example where we have UBER_TILE between TILE and DIE, then we'd set and propagate the UBER_TILE entry into the DIE slot and then overwrite it again, if there is a DIE entry too. Where it becomes interesting is when the unknown level is past DIEGRP, e.g. DIEGRP_CONGLOMORATE then we'd need to overwrite the DIEGRP level, right? It can be done, but I don't know whether it buys us much for the purely theoretical case of new levels added. Thanks, tglx
On Sun, 2023-08-13 at 17:04 +0200, Thomas Gleixner wrote: > On Sat, Aug 12 2023 at 22:04, Thomas Gleixner wrote: > > On Sat, Aug 12 2023 at 08:21, Rui Zhang wrote: > > > With this, we can guarantee that all the available topology > > > information > > > are always valid, even when running on future platforms. > > > > I know that it can be made work, but is it worth the extra effort? > > I > > don't think so. > > So I thought more about it. For intermediate levels, i.e. something > which is squeezed between two existing levels, this works by some > definition of works. this "some definition of works" includes parsing the unknown levels, right? > > I.e. the example where we have UBER_TILE between TILE and DIE, then > we'd > set and propagate the UBER_TILE entry into the DIE slot and then > overwrite it again, if there is a DIE entry too. Well, not really. If we have TILE/UBER_TILE/DIE in CPUID but only support TILE/DIE in kernel, the UBER_TILE information is overwritten. But, UBER_TILE tells us the starting bit in APIC ID for die_id. Say, level type eax.shifts 0 SMT 1 1 CORE 5 2 TILE 7 3 UBER_TILE 8 4 DIE 9 This is a 1 package system with 2 dies, each die has 2 uber_tiles and each uber_tile has 2 tiles. If we don't support uber_tile, what we want to see is a platform with 2 dies and each die has 4 tiles. But topo_shift_apicid() uses x86_topo_system.dom_shifts[TILE], so what we see is a platform with 4 dies, and each die has 2 tiles. And this is broken. IMO, what we really need for each domain in x86_topo_system is dom_size and dom_offset (id bit offset in APIC ID). and when parsing domain A, we can propagate its eax.shifts to the dom_offset of its upper level domains. With this, we set dom_offset[DIE] to 7 first when parsing TILE, and then overwrite it to 8 when parsing UBER_TILE, and set dom_offset[PACKAGE] to 9 when parsinig DIE. lossing TILE.eax.shifts is okay, because it is for UBER_TILE id. > > Where it becomes interesting is when the unknown level is past > DIEGRP, > e.g. DIEGRP_CONGLOMORATE then we'd need to overwrite the DIEGRP > level, > right? > > It can be done, but I don't know whether it buys us much for the > purely > theoretical case of new levels added. > > Similar to previous case, DIEGRP_CONGLOMORATE eax.shifts can be propagated to dom_offset[PACKAGE]. But, still, there is one case that we can not handle, (the reason I'm proposing optional die support in Linux) Say, we have new level FOO, and the CPUID is like this level type eax.shifts 0 SMT 1 1 CORE 5 2 FOO 8 This can be a system with 1. 1 die and 8 FOOs if DIE is the upper level of FOO or 2. 8 FOOs with 1 die in each FOO if DIE is the lower level of FOO Currently, die topology information is mandatory in Linux, we cannot make it right without patching enum topo_types/enum x86_topology_domains/topo_domain_map (which in fact tells the relationship between DIE and FOO). thanks, rui
> On Sun, 2023-08-13 at 17:04 +0200, Thomas Gleixner wrote: > > With this, we set dom_offset[DIE] to 7 first when parsing TILE, and > then overwrite it to 8 when parsing UBER_TILE, and set > dom_offset[PACKAGE] to 9 when parsinig DIE. > > lossing TILE.eax.shifts is okay, because it is for UBER_TILE id. No. That's just wrong. TILE is defined and potentially used in the kernel. How can you rightfully assume that UBER TILE is a valid substitution? You can't. > Currently, die topology information is mandatory in Linux, we cannot > make it right without patching enum topo_types/enum > x86_topology_domains/topo_domain_map (which in fact tells the > relationship between DIE and FOO). You cannot just nilly willy assume at which domain level FOO sits. Look at your example: > Say, we have new level FOO, and the CPUID is like this > level type eax.shifts > 0 SMT 1 > 1 CORE 5 > 2 FOO 8 FOO can be anything between CORE and PKG, so you cannot tell what it means. Simply heuristics _cannot_ be correct by definition. So why trying to come up with them just because? What's the problem you are trying to solve? Some real world issue or some academic though experiment which might never become a real problem? Thanks, tglx
> What's the problem you are trying to solve? Some real world issue or some academic though experiment which might never become a real problem?
There are several existing bugs, bad practices, and latent future bugs in today's x86 topology code.
First, with multiple cores having the same core_id.
A consumer of core_id must know about packages to understand core_id.
This is the original sin of the current interface -- which should never have used the word "sibling" *anyplace*,
Because to make sense of the word sibling, you must know what *contains* the siblings.
We introduced "core_cpus" a number of years ago to address this for core_ids (and for other levels,
Such as die_cpus). Unfortunately, we can probably never actually delete threads_siblings and core_siblings
Without breaking some program someplace...
Core_id should be system-wide global, just like the cpu number is system wide global.
Today, it is the only level id that is not system wide global.
This could be implemented by simply not masking off the package_id bits when creating the core_id,
Like we have done for other levels.
Yes, this could be awkward for some existing code that indexes arrays with core_id, and doesn't like them to be sparse.
But that rough edge is a much smaller problem than having to comprehend a level (package) that you may
Otherwise not care about. Besides, core_id's can already be sparse today.
Secondly, with the obsolescence of CPUID.0b and its replacement with CPUID.1F, the contract between
The hardware and the software is that a level can appear and can in between any existing levels.
(the only exception is that SMT is married to core). It is not possible
For an old kernel to know the name or position of a new level in the hierarchy, going forward.
Today, this manifests with a (currently) latent bug that I caused. For I implemented die_id
In the style of package_id, and I shouldn't have followed that example.
Today, if CPUID.1F doesn't know anything about multiple DIE, Linux conjurs up
A die_id 0 in sysfs. It should not. The reason is that when CPUID.1F enumerates
A level that legacy code doesn't know about, we can't possibly tell if it is above DIE,
or below DIE. If it is above DIE, then our default die_id 0 is becomes bogus.
That said, I have voiced my objection inside Intel to the creation of random levels
Which do not have an architectural (software) definition; and I'm advocating that
They be *removed* from the SDM until a software programming definition that
Spans all generation is documented.
SMT, core, module, die and the (implicit) package may not be well documented,
But they do have existing uses and will thus live on.
The others maybe not.
-Len
Hi, Thomas, On Mon, 2023-08-14 at 14:26 +0200, Thomas Gleixner wrote: > > On Sun, 2023-08-13 at 17:04 +0200, Thomas Gleixner wrote: > > > > With this, we set dom_offset[DIE] to 7 first when parsing TILE, and > > then overwrite it to 8 when parsing UBER_TILE, and set > > dom_offset[PACKAGE] to 9 when parsinig DIE. > > > > lossing TILE.eax.shifts is okay, because it is for UBER_TILE id. > > No. That's just wrong. TILE is defined and potentially used in the > kernel. Sure. > How can you rightfully assume that UBER TILE is a valid > substitution? You can't. TILE.eax.shifts tells 1. the number of maximum addressable threads in TILE domain, which should be saved in x86_topo_system.dom_size[TILE] 2. the highest bit in APIC ID for tile id, but we don't need this if we use package/system scope unique tile id 3. the lowest bit in APIC ID for the upper level of tile if the upper level is a known level, say, die, this info is saved in dom_offset[die] if the upper level is an unknown level, then we don't need this to decode the topology information for the unknown level. maybe I missed something, for now I don't see how things break here. > > > Currently, die topology information is mandatory in Linux, we > > cannot > > make it right without patching enum topo_types/enum > > x86_topology_domains/topo_domain_map (which in fact tells the > > relationship between DIE and FOO). > > You cannot just nilly willy assume at which domain level FOO sits. exactly. > Look > at your example: > > > Say, we have new level FOO, and the CPUID is like this > > level type eax.shifts > > 0 SMT 1 > > 1 CORE 5 > > 2 FOO 8 > > FOO can be anything between CORE and PKG, so you cannot tell what it > means. Exactly. Anything related with MODULE/TILE/DIE can break in this case. Say this is a system with 1 package, 2 FOOs, 8 cores. In current design (in this patch set), kernel has to tell how many dies/tiles/modules this system has, and kernel cannot do this right. But if using optional Die (and surely optional module/tile), kernel can tell that this is a 1-package-0-die-0-tile-0-module-8-core system before knowing what FOO means, we don't need to make up anything we don't know. > > Simply heuristics _cannot_ be correct by definition. So why trying to > come up with them just because? > > What's the problem you are trying to solve? Some real world issue or > some academic though experiment which might never become a real > problem? > Maybe I was misleading previously, IMO, I totally agree with your points, and "using optional die/tile/module" is what I propose to address these concerns. thanks, rui
On Mon, Aug 14 2023 at 15:28, Rui Zhang wrote: > On Mon, 2023-08-14 at 14:26 +0200, Thomas Gleixner wrote: >> What's the problem you are trying to solve? Some real world issue or >> some academic though experiment which might never become a real >> problem? >> > Maybe I was misleading previously, IMO, I totally agree with your > points, and "using optional die/tile/module" is what I propose to > address these concerns. That's exactly what's implemented. If module, tile, die are not advertised, then you end up with: N threads N/2 cores 1 module 1 tile 1 die in a package because the bits occupied by module, tile and die are exactly 0. But from a conceptual and consistency point of view they exist, no? Thanks, tglx
--- a/arch/x86/kernel/cpu/Makefile +++ b/arch/x86/kernel/cpu/Makefile @@ -18,7 +18,7 @@ KMSAN_SANITIZE_common.o := n KCSAN_SANITIZE_common.o := n obj-y := cacheinfo.o scattered.o -obj-y += topology_common.o topology.o +obj-y += topology_common.o topology_ext.o topology.o obj-y += common.o obj-y += rdrand.o obj-y += match.o --- a/arch/x86/kernel/cpu/topology.h +++ b/arch/x86/kernel/cpu/topology.h @@ -16,6 +16,7 @@ void cpu_init_topology(struct cpuinfo_x8 void cpu_parse_topology(struct cpuinfo_x86 *c); void topology_set_dom(struct topo_scan *tscan, enum x86_topology_domains dom, unsigned int shift, unsigned int ncpus); +bool cpu_parse_topology_ext(struct topo_scan *tscan); static inline u32 topo_shift_apicid(u32 apicid, enum x86_topology_domains dom) { @@ -31,4 +32,15 @@ static inline u32 topo_relative_domain_i return apicid & (x86_topo_system.dom_size[dom] - 1); } +/* + * Update a domain level after the fact without propagating. Used to fixup + * broken CPUID enumerations. + */ +static inline void topology_update_dom(struct topo_scan *tscan, enum x86_topology_domains dom, + unsigned int shift, unsigned int ncpus) +{ + tscan->dom_shifts[dom] = shift; + tscan->dom_ncpus[dom] = ncpus; +} + #endif /* ARCH_X86_TOPOLOGY_H */ --- /dev/null +++ b/arch/x86/kernel/cpu/topology_ext.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/cpu.h> + +#include <asm/apic.h> +#include <asm/memtype.h> +#include <asm/processor.h> + +#include "cpu.h" + +enum topo_types { + INVALID_TYPE = 0, + SMT_TYPE = 1, + CORE_TYPE = 2, + MODULE_TYPE = 3, + TILE_TYPE = 4, + DIE_TYPE = 5, + DIEGRP_TYPE = 6, + MAX_TYPE = 7, +}; + +/* + * Use a lookup table for the case that there are future types > 6 which + * describe an intermediate domain level which does not exist today. + * + * A table will also be handy to parse the new AMD 0x80000026 leaf which + * has defined different domain types, but otherwise uses the same layout + * with some of the reserved bits used for new information. + */ +static const unsigned int topo_domain_map[MAX_TYPE] = { + [SMT_TYPE] = TOPO_SMT_DOMAIN, + [CORE_TYPE] = TOPO_CORE_DOMAIN, + [MODULE_TYPE] = TOPO_MODULE_DOMAIN, + [TILE_TYPE] = TOPO_TILE_DOMAIN, + [DIE_TYPE] = TOPO_DIE_DOMAIN, + [DIEGRP_TYPE] = TOPO_PKG_DOMAIN, +}; + +static inline bool topo_subleaf(struct topo_scan *tscan, u32 leaf, u32 subleaf) +{ + unsigned int dom, maxtype = leaf == 0xb ? CORE_TYPE + 1 : MAX_TYPE; + struct { + // eax + u32 x2apic_shift : 5, // Number of bits to shift APIC ID right + // for the topology ID at the next level + __rsvd0 : 27; // Reserved + // ebx + u32 num_processors : 16, // Number of processors at current level + __rsvd1 : 16; // Reserved + // ecx + u32 level : 8, // Current topology level. Same as sub leaf number + type : 8, // Level type. If 0, invalid + __rsvd2 : 16; // Reserved + // edx + u32 x2apic_id : 32; // X2APIC ID of the current logical processor + } sl; + + cpuid_subleaf(leaf, subleaf, &sl); + + if (!sl.num_processors || sl.type == INVALID_TYPE) + return false; + + if (sl.type >= maxtype) { + /* + * As the subleafs are ordered in domain level order, this + * could be recovered in theory by propagating the + * information at the last parsed level. + * + * But if the infinite wisdom of hardware folks decides to + * create a new domain type between CORE and MODULE or DIE + * and DIEGRP, then that would overwrite the CORE or DIE + * information. + * + * It really would have been too obvious to make the domain + * type space sparse and leave a few reserved types between + * the points which might change instead of forcing + * software to either create a monstrosity of workarounds + * or just being up the creek without a paddle. + * + * Refuse to implement monstrosity, emit an error and try + * to survive. + */ + pr_err_once("Topology: leaf 0x%x:%d Unknown domain type %u\n", + leaf, subleaf, sl.type); + return true; + } + + dom = topo_domain_map[sl.type]; + if (!dom) { + tscan->c->topo.initial_apicid = sl.x2apic_id; + } else if (tscan->c->topo.initial_apicid != sl.x2apic_id) { + pr_warn_once(FW_BUG "CPUID leaf 0x%x subleaf %d APIC ID mismatch %x != %x\n", + leaf, subleaf, tscan->c->topo.initial_apicid, sl.x2apic_id); + } + + topology_set_dom(tscan, dom, sl.x2apic_shift, sl.num_processors); + return true; +} + +static bool parse_topology_leaf(struct topo_scan *tscan, u32 leaf) +{ + u32 subleaf; + + if (tscan->c->cpuid_level < leaf) + return false; + + /* Read all available subleafs and populate the levels */ + for (subleaf = 0; topo_subleaf(tscan, leaf, subleaf); subleaf++); + + /* If subleaf 0 failed to parse, give up */ + if (!subleaf) + return false; + + /* + * There are machines in the wild which have shift 0 in the subleaf + * 0, but advertise 2 logical processors at that level. They are + * truly SMT. + */ + if (!tscan->dom_shifts[TOPO_SMT_DOMAIN] && tscan->dom_ncpus[TOPO_SMT_DOMAIN] > 1) { + unsigned int sft = get_count_order(tscan->dom_ncpus[TOPO_SMT_DOMAIN]); + + pr_warn_once(FW_BUG "CPUID leaf 0x%x subleaf 0 has shift level 0 but %u CPUs\n", + leaf, tscan->dom_ncpus[TOPO_SMT_DOMAIN]); + topology_update_dom(tscan, TOPO_SMT_DOMAIN, sft, tscan->dom_ncpus[TOPO_SMT_DOMAIN]); + } + + set_cpu_cap(tscan->c, X86_FEATURE_XTOPOLOGY); + return true; +} + +bool cpu_parse_topology_ext(struct topo_scan *tscan) +{ + /* Try lead 0x1F first. If not available try leaf 0x0b */ + if (parse_topology_leaf(tscan, 0x1f)) + return true; + return parse_topology_leaf(tscan, 0x0b); +}