Re: [PATCH 3/3] x86: correct fencing around CLFLUSH

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.02.2022 13:33, Andrew Cooper wrote:
> On 23/02/2022 10:13, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -346,9 +346,14 @@ void __init early_cpu_init(void)
>>             c->x86_model, c->x86_model, c->x86_mask, eax);
>>  
>>      if (c->cpuid_level >= 7)
>> -            cpuid_count(7, 0, &eax, &ebx,
>> +            cpuid_count(7, 0, &eax,
>> +                            &c->x86_capability[FEATURESET_7b0],
>>                              &c->x86_capability[FEATURESET_7c0], &edx);
>>  
>> +    if (!(c->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ||
>> +        cpu_has(c, X86_FEATURE_CLFLUSHOPT))
>> +            setup_force_cpu_cap(X86_FEATURE_CLFLUSH_NO_MFENCE);
> 
> This is somewhat ugly, not only because it presumes that the early AMD
> implementation peculiarities are common.
> 
> It also has a corner case that goes wrong when the BSP enumerates
> CLFLUSHOPT but later CPUs don't.  In this case the workaround will be
> disengaged even when it's not safe to.

You realize though that this cannot be taken care of when alternatives
patching is involved? Are you suggesting to not use patching just to
deal with an asymmetry we don't really deal with anywhere else?

> Most importantly however, it makes the one current slow usecase (VT-d on
> early Intel with only CLFLUSH) even slower.
> 
> 
> I suggest inverting this workaround (and IMO, using the bug
> infrastructure, because that's exactly what it is) and leaving a big
> warning by the function saying "don't use on AMD before alternatives
> have run" or something.  It's quite possibly a problem we'll never need
> to solve in practice, although my plans for overhauling CPUID scanning
> will probably fix it because we can move the first alternatives pass far
> earlier as a consequence.

I've switched to marking this BUG, but I'm not sure about such a
comment: It really depends on the use whether it would be safe
without the MFENCEs. (We also aren't aware of problems, despite them
having been missing forever.) Furthermore it's not overly likely for
anyone to look here when adding a new use of FLUSH_CACHE. I'd
therefore rather consider it best effort behavior until patching has
taken place.

>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -245,12 +245,15 @@ unsigned int flush_area_local(const void
>>               c->x86_clflush_size && c->x86_cache_size && sz &&
>>               ((sz >> 10) < c->x86_cache_size) )
>>          {
>> -            alternative("", "sfence", X86_FEATURE_CLFLUSHOPT);
>> +            alternative("mfence", , X86_FEATURE_CLFLUSH_NO_MFENCE);
> 
> An an aside, the absence of "" is very weird parse, and only compiles
> because this is a macro rather than a function.
> 
> I'd request that it stays, simply to make the code read more like regular C.

To be honest I was half expecting this feedback. For now I've put
back the quotes, but I have a change halfway ready which will
eliminate the need for quotes in the same cases where the
assembler macros don't require their use (as you may guess: by
using the assembler macros instead of maintaining redundant C
infrastructure). I guess at that point it would become a little
inconsistent if quotes were present just to express "empty". Also
if I'm not mistaken this isn't the only place where we make use of
macro arguments being allowed to be empty.

Jan