Re: [Xen-devel] [PATCH 4/8] x86/AMD: distinguish compute units from hyper-threads

[Brian Woods <brian.woods@xxxxxxx>]

On Wed, Jul 11, 2018 at 06:07:42AM -0600, Jan Beulich wrote:
> Fam17 replaces CUs by HTs, which we should reflect accordingly, even if
> the difference is not very big. The most relevant change (requiring some
> code restructuring) is that the topoext feature no longer means there is
> a valid CU ID.
> 
> Take the opportunity and convert wrongly plain int variables in
> set_cpu_sibling_map() to unsigned int.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -504,17 +504,23 @@ static void amd_get_topology(struct cpui
>                  u32 eax, ebx, ecx, edx;
>  
>                  cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
> -                c->compute_unit_id = ebx & 0xFF;
>                  c->x86_num_siblings = ((ebx >> 8) & 0x3) + 1;
> +
> +             if (c->x86 < 0x17)
> +                     c->compute_unit_id = ebx & 0xFF;
> +             else {
> +                     c->cpu_core_id = ebx & 0xFF;
> +                     c->x86_max_cores /= c->x86_num_siblings;
> +             }
>          }
>          
>          if (opt_cpu_info)
>                  printk("CPU %d(%d) -> Processor %d, %s %d\n",
>                         cpu, c->x86_max_cores, c->phys_proc_id,
> -                       cpu_has(c, X86_FEATURE_TOPOEXT) ? "Compute Unit" : 
> -                                                         "Core",
> -                       cpu_has(c, X86_FEATURE_TOPOEXT) ? c->compute_unit_id :
> -                                                         c->cpu_core_id);
> +                       c->compute_unit_id != INVALID_CUID ? "Compute Unit"
> +                                                          : "Core",
> +                       c->compute_unit_id != INVALID_CUID ? 
> c->compute_unit_id
> +                                                          : c->cpu_core_id);
>  }
>  
>  static void early_init_amd(struct cpuinfo_x86 *c)
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -236,33 +236,41 @@ static void link_thread_siblings(int cpu
>      cpumask_set_cpu(cpu2, per_cpu(cpu_core_mask, cpu1));
>  }
>  
> -static void set_cpu_sibling_map(int cpu)
> +static void set_cpu_sibling_map(unsigned int cpu)
>  {
> -    int i;
> +    unsigned int i;
>      struct cpuinfo_x86 *c = cpu_data;
>  
>      cpumask_set_cpu(cpu, &cpu_sibling_setup_map);
>  
>      cpumask_set_cpu(cpu, socket_cpumask[cpu_to_socket(cpu)]);
> +    cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
> +    cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
>  
>      if ( c[cpu].x86_num_siblings > 1 )
>      {
>          for_each_cpu ( i, &cpu_sibling_setup_map )
>          {
> -            if ( cpu_has(c, X86_FEATURE_TOPOEXT) ) {
> -                if ( (c[cpu].phys_proc_id == c[i].phys_proc_id) &&
> -                     (c[cpu].compute_unit_id == c[i].compute_unit_id) )
> +            if ( cpu == i || c[cpu].phys_proc_id != c[i].phys_proc_id )
> +                continue;
> +            if ( c[cpu].compute_unit_id != INVALID_CUID &&
> +                 c[i].compute_unit_id != INVALID_CUID )
> +            {
> +                if ( c[cpu].compute_unit_id == c[i].compute_unit_id )
>                      link_thread_siblings(cpu, i);
> -            } else if ( (c[cpu].phys_proc_id == c[i].phys_proc_id) &&
> -                        (c[cpu].cpu_core_id == c[i].cpu_core_id) ) {
> -                link_thread_siblings(cpu, i);
>              }
> +            else if ( c[cpu].cpu_core_id != XEN_INVALID_CORE_ID &&
> +                      c[i].cpu_core_id != XEN_INVALID_CORE_ID )
> +            {
> +                if ( c[cpu].cpu_core_id == c[i].cpu_core_id )
> +                    link_thread_siblings(cpu, i);
> +            }
> +            else
> +                printk(XENLOG_WARNING
> +                       "CPU%u: unclear relationship with CPU%u\n",
> +                       cpu, i);
>          }
>      }
> -    else
> -    {
> -        cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
> -    }
>  
>      if ( c[cpu].x86_max_cores == 1 )
>      {
> 
> 
> 
> 

Side note: if cpu_core_id isn't the logical cpu, it might be worth
updating the comments in processor.h

Reviewed-by: Brian Woods <brian.woods@xxxxxxx>

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel