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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.09.2021 20:07, Andrew Cooper wrote:
> On 06/09/2021 16:17, Jan Beulich wrote:
>> 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).
> 
> This is "rm" because it is what we use elsewhere in Xen for selectors,
> and because it is the correct constraints based on the legal instruction
> encodings.

grep-ing for "%%[defgs]s" reveals:

efi_arch_post_exit_boot(), svm_ctxt_switch_to(), and
do_set_segment_base() all use just "r". This grep has not produced
any use of "rm". What are you talking about?

> If you want to work around what you perceive to be bugs in compilers
> then submit a independent change yourself.

I don't perceive this as a bug; perhaps a desirable feature. I also
did start my response with "While I don't strictly mind the "m"
part ..." - was this not careful enough to indicate I'm not going
to insist on the change, but I'd prefer it to be made?

>>> @@ -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?
> 
> 0x30, although it turns out that some of the mobile Zen2 CPUs exceed
> 0x60 in terms of model number.
> 
> As Zen3 changes the family number to 0x19, I'd just drop the upper bound.

Minor note: Even if it didn't, the !cpu_has_nscb would also be enough
to avoid the probing there.

>>  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
> 
> From talking to people at AMD.
> 
>>  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.
> 
> From talking to people at Hygon.

Fair enough, but imo this wants mentioning in the description.

Jan