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

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

On 23/02/2022 10:13, Jan Beulich wrote:
> As noted in the context of 3330013e6739 ("VT-d / x86: re-arrange cache
> syncing"): While cache_writeback() has the SFENCE on the correct side of
> CLFLUSHOPT, flush_area_local() doesn't. While I can't prove it due to
> lacking a copy of the old SDM version, I can only assume this placement
> was a result of what had been described there originally. In any event
> recent versions of the SDM hve been telling us otherwise.
>
> For AMD (and Hygon, albeit there it's benign, since all their CPUs are
> expected to support CLFLUSHOPT) the situation is more complex: MFENCE is
> needed ahead and/or after CLFLUSH when the CPU doesn't also support
> CLFLUSHOPT. (It's "and" in our case, as we cannot know what the caller's
> needs are.)
>
> Fixes: 623c720fc8da3 ("x86: use CLFLUSHOPT when available")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- 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.

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.


> +
>  	eax = cpuid_eax(0x80000000);
>  	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
>  		ebx = eax >= 0x8000001f ? cpuid_ebx(0x8000001f) : 0;
> --- 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.

~Andrew