Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.09.2021 14:00, Jane Malalane wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>                         c->x86_capability);
>  }
>  
> +void detect_zen2_null_seg_behaviour(void)

This can in principle be marked __init.

> +{
> +     uint64_t base;
> +
> +     wrmsrl(MSR_FS_BASE, 1);
> +     asm volatile ( "mov %0, %%fs" :: "rm" (0) );

While I don't strictly mind the "m" part of the constraint to remain
there (in the hope for compilers actually to support this), iirc it's
not useful to have when the value is a constant: Last time I checked,
the compiler would not instantiate an anonymous (stack) variable to
fulfill this constraint (as can be seen when dropping the "r" part of
the constraint).

> @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c)
>       else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
>               amd_init_lfence(c);
>  
> +     /* Probe for NSCB on Zen2 CPUs when not virtualised */
> +     if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
> +         c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f)

DYM 0x30 here? Or 0x1e? In any event 0x5f should be accompanied by
another hex constant. And it would also help if in the description
you said where these bounds as well as ...

> --- a/xen/arch/x86/cpu/hygon.c
> +++ b/xen/arch/x86/cpu/hygon.c
> @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c)
>  
>       amd_init_lfence(c);
>  
> +     /* Probe for NSCB on Zen2 CPUs when not virtualised */
> +     if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
> +         c->x86 == 0x18 && c->x86_model >= 4)

... this one come from.

Jan