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

[Zhang, Rui]

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