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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/09/2021 07:09, Jan Beulich wrote:
> 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(),

These are writing multiple selectors in one go, and a register
constraint is the only sane option.

> and do_set_segment_base() all use just "r".

I had missed this one.

>  This grep has not produced
> any use of "rm". What are you talking about?

TRY_LOAD_SEG(), pv_emul_read_descriptor() for both lar and lsl,
do_double_fault() for another lsl, lldt(), ltr().

So ok - not everything, but most.

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

No, because a maintainer saying "I'd prefer this to be changed" is still
an instruction to the submitter to make the change.

But the request is inappropriate.  "Last time I checked, the compiler
would" presumably means you've checked GCC and not Clang, and therefore
any conclusions about the behaviour are incomplete.

Unless there is a real concrete compiler bug to work around, "rm" is the
appropriate constraint to use, all other things being equal.  If the
complier is merely doing something dumb with the flexibility it has been
permitted, then fix the compiler and the problem will resolve itself the
proper way.

>
>>>> @@ -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.

There is actually a problem.  From a non-AMD source, I've found the
Sucor Z+ CPU which is a Fam17h Model 0x50 Zen1.

So instead I'm going to recommend dropping all model checks and just
keeping the family checks.  This will extend the probe function to Zen1
too, but it is once on boot, trivial in terms of complexity, and really
not worth the time/effort it has taken to discover that the model list
wasn't correct to start with.

~Andrew