[V3,27/40] x86/cpu: Provide a sane leaf 0xb/0x1f parser

  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(-)
  

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