[PATCH 0/3] x86: correct fencing around CLFLUSH (+some tidying)

[PATCH 0/3] x86: correct fencing around CLFLUSH (+some tidying)

[Jan Beulich]

While it'll make backporting of the last patch a little harder,
it seemed better to me to do some cleanup first this time round.

1: drop NOP_DS_PREFIX
2: cpuid: replace more cpufeat_word() uses
3: correct fencing around CLFLUSH

Jan

[PATCH 1/3] x86: drop NOP_DS_PREFIX

[Jan Beulich]

This wasn't really necessary to introduce: The binutils change
permitting use of standalone "ds" (and "cs") in 64-bit code predates
the minimum binutils version we support.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
In fact we could patch _just_ the opcode prefix in flush_area_local().

--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -247,8 +247,7 @@ unsigned int flush_area_local(const void
         {
             alternative("", "sfence", X86_FEATURE_CLFLUSHOPT);
             for ( i = 0; i < sz; i += c->x86_clflush_size )
-                alternative_input(".byte " __stringify(NOP_DS_PREFIX) ";"
-                                  " clflush %0",
+                alternative_input("ds; clflush %0",
                                   "data16 clflush %0",      /* clflushopt */
                                   X86_FEATURE_CLFLUSHOPT,
                                   "m" (((const char *)va)[i]));
@@ -298,11 +297,11 @@ void cache_writeback(const void *addr, u
 # define INPUT(addr) "a" (addr), BASE_INPUT(addr)
 #endif
         /*
-         * Note regarding the use of NOP_DS_PREFIX: it's faster to do a clflush
+         * Note regarding the "ds" prefix use: it's faster to do a clflush
          * + prefix than a clflush + nop, and hence the prefix is added instead
          * of letting the alternative framework fill the gap by appending nops.
          */
-        alternative_io_2(".byte " __stringify(NOP_DS_PREFIX) "; clflush %[p]",
+        alternative_io_2("ds; clflush %[p]",
                          "data16 clflush %[p]", /* clflushopt */
                          X86_FEATURE_CLFLUSHOPT,
                          CLWB_ENCODING,
--- a/xen/arch/x86/include/asm/nops.h
+++ b/xen/arch/x86/include/asm/nops.h
@@ -5,8 +5,6 @@
  * Define nops for use with alternative().
  */
 
-#define NOP_DS_PREFIX 0x3e
-
 /*
  * Opteron 64bit nops
  * 1: nop

Re: [PATCH 1/3] x86: drop NOP_DS_PREFIX

[Andrew Cooper]

On 23/02/2022 10:12, Jan Beulich wrote:
> This wasn't really necessary to introduce: The binutils change
> permitting use of standalone "ds" (and "cs") in 64-bit code predates
> the minimum binutils version we support.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

I was never a fan of NOP_DS_PREFIX.  Far too verbose for what it's doing.

> ---
> In fact we could patch _just_ the opcode prefix in flush_area_local().
>
> --- a/xen/arch/x86/flushtlb.c
> +++ b/xen/arch/x86/flushtlb.c
> @@ -247,8 +247,7 @@ unsigned int flush_area_local(const void
>          {
>              alternative("", "sfence", X86_FEATURE_CLFLUSHOPT);
>              for ( i = 0; i < sz; i += c->x86_clflush_size )
> -                alternative_input(".byte " __stringify(NOP_DS_PREFIX) ";"
> -                                  " clflush %0",
> +                alternative_input("ds; clflush %0",

Binutils appears to be happy with "ds clflush", i.e. treating it like a
proper prefix on the instruction.  Drop the semicolon at the same time?

~Andrew

Re: [PATCH 1/3] x86: drop NOP_DS_PREFIX

[Jan Beulich]

On 23.02.2022 11:54, Andrew Cooper wrote:
> On 23/02/2022 10:12, Jan Beulich wrote:
>> This wasn't really necessary to introduce: The binutils change
>> permitting use of standalone "ds" (and "cs") in 64-bit code predates
>> the minimum binutils version we support.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> I was never a fan of NOP_DS_PREFIX.  Far too verbose for what it's doing.
> 
>> ---
>> In fact we could patch _just_ the opcode prefix in flush_area_local().
>>
>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -247,8 +247,7 @@ unsigned int flush_area_local(const void
>>          {
>>              alternative("", "sfence", X86_FEATURE_CLFLUSHOPT);
>>              for ( i = 0; i < sz; i += c->x86_clflush_size )
>> -                alternative_input(".byte " __stringify(NOP_DS_PREFIX) ";"
>> -                                  " clflush %0",
>> +                alternative_input("ds; clflush %0",
> 
> Binutils appears to be happy with "ds clflush", i.e. treating it like a
> proper prefix on the instruction.  Drop the semicolon at the same time?

I'd rather not. A clever assembler may eliminate the prefix as redundant
when the base register isn't stack or frame pointer. In 64-bit mode an
assembler might even decide to eliminate all non-standalone segment
overrides using the pre-386 segment registers.

Jan

[PATCH 2/3] x86/cpuid: replace more cpufeat_word() uses

[Jan Beulich]

Complete what e3662437eb43 ("x86/cpuid: Disentangle logic for new
feature leaves") has begun:

"Switch to using FEATURESET_* just like the policy/featureset helpers.  This
 breaks the cognitive complexity of needing to know which leaf a specifically
 named feature should reside in, and is shorter to write.  It is also far
 easier to identify as correct at a glance, given the correlation with the
 CPUID leaf being read."

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -332,23 +332,22 @@ void __init early_cpu_init(void)
 	cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
 	c->x86 = get_cpu_family(eax, &c->x86_model, &c->x86_mask);
 
-	edx &= ~cleared_caps[cpufeat_word(X86_FEATURE_FPU)];
-	ecx &= ~cleared_caps[cpufeat_word(X86_FEATURE_SSE3)];
+	edx &= ~cleared_caps[FEATURESET_1d];
+	ecx &= ~cleared_caps[FEATURESET_1c];
 	if (edx & cpufeat_mask(X86_FEATURE_CLFLUSH))
 		c->x86_cache_alignment = ((ebx >> 8) & 0xff) * 8;
 	/* Leaf 0x1 capabilities filled in early for Xen. */
-	c->x86_capability[cpufeat_word(X86_FEATURE_FPU)] = edx;
-	c->x86_capability[cpufeat_word(X86_FEATURE_SSE3)] = ecx;
+	c->x86_capability[FEATURESET_1d] = edx;
+	c->x86_capability[FEATURESET_1c] = ecx;
 
 	printk(XENLOG_INFO
 	       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
 	       x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
 	       c->x86_model, c->x86_model, c->x86_mask, eax);
 
-	if (c->cpuid_level >= 7) {
-		cpuid_count(7, 0, &eax, &ebx, &ecx, &edx);
-		c->x86_capability[cpufeat_word(X86_FEATURE_CET_SS)] = ecx;
-	}
+	if (c->cpuid_level >= 7)
+		cpuid_count(7, 0, &eax, &ebx,
+                            &c->x86_capability[FEATURESET_7c0], &edx);
 
 	eax = cpuid_eax(0x80000000);
 	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -685,11 +685,11 @@ static void __init efi_arch_cpu(void)
 
     boot_tsc_stamp = rdtsc();
 
-    caps[cpufeat_word(X86_FEATURE_HYPERVISOR)] = cpuid_ecx(1);
+    caps[FEATURESET_1c] = cpuid_ecx(1);
 
     if ( (eax >> 16) == 0x8000 && eax > 0x80000000 )
     {
-        caps[cpufeat_word(X86_FEATURE_SYSCALL)] = cpuid_edx(0x80000001);
+        caps[FEATURESET_e1d] = cpuid_edx(0x80000001);
 
         if ( cpu_has_nx )
             trampoline_efer |= EFER_NXE;
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -516,7 +516,7 @@ static inline void __init construct_defa
 				   (boot_cpu_data.x86_model << 4) |
 				   boot_cpu_data.x86_mask;
 	processor.mpc_featureflag =
-            boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_FPU)];
+            boot_cpu_data.x86_capability[FEATURESET_1d];
 	processor.mpc_reserved[0] = 0;
 	processor.mpc_reserved[1] = 0;
 	for (i = 0; i < 2; i++) {
--- a/xen/arch/x86/tsx.c
+++ b/xen/arch/x86/tsx.c
@@ -48,7 +48,7 @@ void tsx_init(void)
         bool has_rtm_always_abort;
 
         if ( boot_cpu_data.cpuid_level >= 7 )
-            boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_ARCH_CAPS)]
+            boot_cpu_data.x86_capability[FEATURESET_7d0]
                 = cpuid_count_edx(7, 0);
 
         if ( cpu_has_arch_caps )

Re: [PATCH 2/3] x86/cpuid: replace more cpufeat_word() uses

[Andrew Cooper]

On 23/02/2022 10:12, Jan Beulich wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -332,23 +332,22 @@ void __init early_cpu_init(void)
>  	cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
>  	c->x86 = get_cpu_family(eax, &c->x86_model, &c->x86_mask);
>  
> -	edx &= ~cleared_caps[cpufeat_word(X86_FEATURE_FPU)];
> -	ecx &= ~cleared_caps[cpufeat_word(X86_FEATURE_SSE3)];
> +	edx &= ~cleared_caps[FEATURESET_1d];
> +	ecx &= ~cleared_caps[FEATURESET_1c];
>  	if (edx & cpufeat_mask(X86_FEATURE_CLFLUSH))
>  		c->x86_cache_alignment = ((ebx >> 8) & 0xff) * 8;
>  	/* Leaf 0x1 capabilities filled in early for Xen. */
> -	c->x86_capability[cpufeat_word(X86_FEATURE_FPU)] = edx;
> -	c->x86_capability[cpufeat_word(X86_FEATURE_SSE3)] = ecx;
> +	c->x86_capability[FEATURESET_1d] = edx;
> +	c->x86_capability[FEATURESET_1c] = ecx;
>  
>  	printk(XENLOG_INFO
>  	       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
>  	       x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
>  	       c->x86_model, c->x86_model, c->x86_mask, eax);
>  
> -	if (c->cpuid_level >= 7) {
> -		cpuid_count(7, 0, &eax, &ebx, &ecx, &edx);
> -		c->x86_capability[cpufeat_word(X86_FEATURE_CET_SS)] = ecx;
> -	}
> +	if (c->cpuid_level >= 7)
> +		cpuid_count(7, 0, &eax, &ebx,
> +                            &c->x86_capability[FEATURESET_7c0], &edx);

Spaces vs tabs here.  This hunk interacts with the CET-IBT series (which
collects edx too), but the rebase either way around is easy.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Jan Beulich]

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);
+
 	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);
             for ( i = 0; i < sz; i += c->x86_clflush_size )
                 alternative_input("ds; clflush %0",
                                   "data16 clflush %0",      /* clflushopt */
                                   X86_FEATURE_CLFLUSHOPT,
                                   "m" (((const char *)va)[i]));
+            alternative_2("mfence",
+                          , X86_FEATURE_CLFLUSH_NO_MFENCE,
+                          "sfence", X86_FEATURE_CLFLUSHOPT);
             flags &= ~FLUSH_CACHE;
         }
         else
@@ -274,6 +277,8 @@ void cache_writeback(const void *addr, u
     unsigned int clflush_size = current_cpu_data.x86_clflush_size ?: 16;
     const void *end = addr + size;
 
+    alternative("mfence", , X86_FEATURE_CLFLUSH_NO_MFENCE);
+
     addr -= (unsigned long)addr & (clflush_size - 1);
     for ( ; addr < end; addr += clflush_size )
     {
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -24,7 +24,7 @@ XEN_CPUFEATURE(APERFMPERF,        X86_SY
 XEN_CPUFEATURE(MFENCE_RDTSC,      X86_SYNTH( 9)) /* MFENCE synchronizes RDTSC */
 XEN_CPUFEATURE(XEN_SMEP,          X86_SYNTH(10)) /* SMEP gets used by Xen itself */
 XEN_CPUFEATURE(XEN_SMAP,          X86_SYNTH(11)) /* SMAP gets used by Xen itself */
-/* Bit 12 - unused. */
+XEN_CPUFEATURE(CLFLUSH_NO_MFENCE, X86_SYNTH(12)) /* No MFENCE needed to serialize CLFLUSH */
 XEN_CPUFEATURE(IND_THUNK_LFENCE,  X86_SYNTH(13)) /* Use IND_THUNK_LFENCE */
 XEN_CPUFEATURE(IND_THUNK_JMP,     X86_SYNTH(14)) /* Use IND_THUNK_JMP */
 XEN_CPUFEATURE(SC_NO_BRANCH_HARDEN, X86_SYNTH(15)) /* (Disable) Conditional branch hardening */

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

[Andrew Cooper]

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

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

[Jan Beulich]

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