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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2021 15:27, Andrew Cooper wrote:
> 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().

TRY_LOAD_SEG() and pv_emul_read_descriptor() don't pass constants
as asm() argument values. do_double_fault()'s use of lsl is indeed
an example matching the pattern here. lldt() and ltr() are generic
inline helpers, so validly allow for both because they should not
make assumptions on what the caller passes. Plus "m" there is okay,
because if the caller passes a constant there will be a named
variable (the function parameter), i.e. the compiler does not need
to instantiate any anonymous one.

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

It was a request to _consider_ dropping the m part, yes. But (see
below) now that you've forced me to re-check (I presume you didn't
check yourself, or else I would expect you would have drawn the same
conclusion as I did), I actually feel stronger about this needing
adjustment.

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

Not really, no. IIRC I did check the version of clang that I have easy
access to. (For gcc I've just now re-checked with 10.x and 11.x.)

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

I disagree. When an asm() constraint permits multiple kinds of values,
dropping one or more of the alternatives should IMO still yield valid
(perhaps sub-optimal) code (IOW every one of the supplied kinds should
be valid). The issue here is that it is not spelled out clearly
whether something like "m" (0) is actually legal. The error messages,
however, suggest this is intended to be illegal: gcc says "memory
input ... is not directly addressable", whereas clang says "invalid
lvalue in asm input for constraint 'm'".

IOW I do think the one case of LSL in do_double_fault() does need
adjusting.

Jan