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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Feng Tang <feng.tang@intel.com>,
        Peter Zijlstra <peterz@infradead.org>
Cc:     Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H . Peter Anvin" <hpa@zytor.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        "Paul E . McKenney" <paulmck@kernel.org>, x86@kernel.org,
        linux-kernel@vger.kernel.org, rui.zhang@intel.com,
        tim.c.chen@intel.com
Subject: Re: [Patch v2 2/2] x86/tsc: use logical_packages as a better
 estimation of socket numbers
In-Reply-To: <ZIwMstkB7CG3pDYu@feng-clx>
References: <20230613052523.1106821-1-feng.tang@intel.com>
 <20230613052523.1106821-2-feng.tang@intel.com>
 <20230615092021.GE1683497@hirez.programming.kicks-ass.net>
 <ZIwMstkB7CG3pDYu@feng-clx>
Date:   Thu, 22 Jun 2023 16:27:23 +0200
Message-ID: <87h6qz7et0.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Fri, Jun 16 2023 at 15:18, Feng Tang wrote:
> On Thu, Jun 15, 2023 at 11:20:21AM +0200, Peter Zijlstra wrote:
> Yes. Rui is working on a MADT based parsing which may take a while
> before being stable, given all kinds of fancy firmware out there.

Please not yet another mad table parser.

The topology can be evaluated during early boot via:

  1) The APIC IDs of the possible CPUs.

  2) CPUID leaf 0xb or 0x1f where the topmost subleaf gives the number
     of bits to shift the APIC ID right for the package/socket

Trying to accomodate for anything else than the documented enumeration
is crazy. If fancy firmware is broken then they can keep the pieces.

So something like the below should just work.

I fundamentally hate the hackery in topology.c, but cleaning this mess
up is a completely different problem and already worked on.

Thanks,

        tglx
---
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -509,9 +509,12 @@ extern int default_check_phys_apicid_pre
 #ifdef CONFIG_SMP
 bool apic_id_is_primary_thread(unsigned int id);
 void apic_smt_update(void);
+extern unsigned int apic_to_pkg_shift;
+bool logical_packages_update(u32 apicid);
 #else
 static inline bool apic_id_is_primary_thread(unsigned int id) { return false; }
 static inline void apic_smt_update(void) { }
+static inline bool logical_packages_update(u32 apicid) { return true; }
 #endif
 
 struct msi_msg;
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -177,6 +177,9 @@ static int acpi_register_lapic(int id, u
 		return -EINVAL;
 	}
 
+	if (!logical_packages_update(acpiid))
+		return -EINVAL;
+
 	if (!enabled) {
 		++disabled_cpus;
 		return -EINVAL;
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -692,6 +692,8 @@ static void early_init_amd(struct cpuinf
 		}
 	}
 
+	detect_extended_topology_early(c);
+
 	if (cpu_has(c, X86_FEATURE_TOPOEXT))
 		smp_num_siblings = ((cpuid_ebx(0x8000001e) >> 8) & 0xff) + 1;
 }
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -29,6 +29,8 @@ unsigned int __max_die_per_package __rea
 EXPORT_SYMBOL(__max_die_per_package);
 
 #ifdef CONFIG_SMP
+unsigned int apic_to_pkg_shift __ro_after_init;
+
 /*
  * Check if given CPUID extended topology "leaf" is implemented
  */
@@ -66,7 +68,7 @@ int detect_extended_topology_early(struc
 {
 #ifdef CONFIG_SMP
 	unsigned int eax, ebx, ecx, edx;
-	int leaf;
+	int leaf, subleaf;
 
 	leaf = detect_extended_topology_leaf(c);
 	if (leaf < 0)
@@ -80,6 +82,14 @@ int detect_extended_topology_early(struc
 	 */
 	c->initial_apicid = edx;
 	smp_num_siblings = max_t(int, smp_num_siblings, LEVEL_MAX_SIBLINGS(ebx));
+
+	for (subleaf = 1; subleaf < 8; subleaf++) {
+		cpuid_count(leaf, subleaf, &eax, &ebx, &ecx, &edx);
+
+		if (ebx == 0 || !LEAFB_SUBTYPE(ecx))
+			break;
+		apic_to_pkg_shift = BITS_SHIFT_NEXT_LEVEL(eax);
+	}
 #endif
 	return 0;
 }
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1501,17 +1501,50 @@ void __init native_smp_prepare_boot_cpu(
 	native_pv_lock_init();
 }
 
+bool logical_packages_update(u32 apicid)
+{
+	unsigned int pkg;
+
+	if (!apic_to_pkg_shift)
+		return true;
+
+	pkg = (apicid >> apic_to_pkg_shift) + 1;
+	if (pkg <= __max_logical_packages)
+		return true;
+
+	if (system_state == SYSTEM_BOOTING) {
+		__max_logical_packages = pkg;
+		return true;
+	}
+
+	pr_err("Physical hotplug APICID %x package %u > max logical packages %u\n",
+	       apicid, pkg, __max_logical_packages);
+	return false;
+}
+
 void __init calculate_max_logical_packages(void)
 {
-	int ncpus;
+	unsigned int ncpus, npkg;
 
 	/*
 	 * Today neither Intel nor AMD support heterogeneous systems so
 	 * extrapolate the boot cpu's data to all packages.
 	 */
 	ncpus = cpu_data(0).booted_cores * topology_max_smt_threads();
-	__max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
-	pr_info("Max logical packages: %u\n", __max_logical_packages);
+	npkg = DIV_ROUND_UP(total_cpus, ncpus);
+
+	/* Did logical_packages_update() set up __max_logical_packages? */
+	if (!__max_logical_packages) {
+		__max_logical_packages = npkg;
+	} else {
+		pr_info("Max logical packages ACPI enumeration: %u\n",
+		       __max_logical_packages);
+		if (npkg <= __max_logical_packages)
+			return;
+		__max_logical_packages = npkg;
+	}
+
+	pr_info("Max logical packages estimated: %u\n", __max_logical_packages);
 }
 
 void __init native_smp_cpus_done(unsigned int max_cpus)