linux-kernel - Re: [Patch v2 2/2] x86/tsc: use logical_packages as a better estimation of socket numbers

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]

Date:   Fri, 23 Jun 2023 15:36:57 +0000
From:   "Zhang, Rui" <rui.zhang@...el.com>
To:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "Tang, Feng" <feng.tang@...el.com>
CC:     "Chen, Tim C" <tim.c.chen@...el.com>,
        "bp@...en8.de" <bp@...en8.de>, "x86@...nel.org" <x86@...nel.org>,
        "hpa@...or.com" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "paulmck@...nel.org" <paulmck@...nel.org>,
        "Woodhouse, David" <dwmw@...zon.co.uk>
Subject: Re: [Patch v2 2/2] x86/tsc: use logical_packages as a better
 estimation of socket numbers

Hi, Thomas,

On Thu, 2023-06-22 at 16:27 +0200, Thomas Gleixner wrote:
> 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
> 
exactly the same in my proposal.

The difference is that I also
1. get the bitshift of the core id and count the number of cores in a
   package.
   On Intel hybrid platforms, using nr_package_cpus / nr_core_cpus to
   get the number of cores in a package (x86_max_cores) is broken.
   e.g. for a 6Pcore + 8Ecore system, package has 20 CPUs and 14 cores.
2. get the maximum number of SMT siblings in each core to set correct
   smp_num_siblings.
   On future hybrid platforms, it is possible that Ecore is used
   as boot CPU. This could result in smp_num_siblings set to 1 during 
   boot cpu startup, and cpu_smt_control set to CPU_SMT_NOT_SUPPORTED.

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

Do we ever see ebx == 0 for a valid subtype?
When decoding CPUID.0B/1F, we check for invalid subtype only.

thanks,
rui