Re: [PATCH] x86: drop XSAVEOPT and CLWB build flags

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/04/2025 12:28 am, Andrew Cooper wrote:
> On 04/04/2025 12:22 am, Alexander M. Merritt wrote:
>> The new toolchain baseline knows both the XSAVEOPT and CLWB instructions.
> I know that's what I wrote on the ticket, but what I'd forgotten was
> that we only use XSAVEOPT for it's operand.
>
> Really what we're doing here is knowing CLWB, and also getting rid of
> the XSAVEOPT workaround for somewhat-more-old toolchains.
>
>> Resolves: https://gitlab.com/xen-project/xen/-/work_items/205
>> Signed-off-by: Alexander M. Merritt <alexander@xxxxxxxxx>
>> ---
>>  xen/arch/x86/arch.mk              |  2 --
>>  xen/arch/x86/flushtlb.c           | 28 +---------------------------
>>  xen/arch/x86/include/asm/system.h |  7 -------
>>  3 files changed, 1 insertion(+), 36 deletions(-)
>>
>> diff --git a/xen/arch/x86/arch.mk b/xen/arch/x86/arch.mk
>> index 258e459bec..baa83418bc 100644
>> --- a/xen/arch/x86/arch.mk
>> +++ b/xen/arch/x86/arch.mk
>> @@ -15,9 +15,7 @@ $(call as-option-add,CFLAGS,CC,"crc32 
>> %eax$(comma)%eax",-DHAVE_AS_SSE4_2)
>>  $(call as-option-add,CFLAGS,CC,"invept (%rax)$(comma)%rax",-DHAVE_AS_EPT)
>>  $(call as-option-add,CFLAGS,CC,"rdrand %eax",-DHAVE_AS_RDRAND)
>>  $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
>> -$(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
>>  $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
>> -$(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB)
>>  $(call as-option-add,CFLAGS,CC,".equ \"x\"$(comma)1",-DHAVE_AS_QUOTED_SYM)
>>  $(call as-option-add,CFLAGS,CC,"invpcid 
>> (%rax)$(comma)%rax",-DHAVE_AS_INVPCID)
>>  $(call as-option-add,CFLAGS,CC,"movdiri 
>> %rax$(comma)(%rax)",-DHAVE_AS_MOVDIR)
>> diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
>> index 65be0474a8..962bb87d69 100644
>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -313,33 +313,7 @@ void cache_writeback(const void *addr, unsigned int 
>> size)
>>      clflush_size = current_cpu_data.x86_clflush_size ?: 16;
>>      addr -= (unsigned long)addr & (clflush_size - 1);
>>      for ( ; addr < end; addr += clflush_size )
>> -    {
>> -/*
>> - * The arguments to a macro must not include preprocessor directives. Doing 
>> so
>> - * results in undefined behavior, so we have to create some defines here in
>> - * order to avoid it.
>> - */
>> -#if defined(HAVE_AS_CLWB)
>> -# define CLWB_ENCODING "clwb %[p]"
>> -#elif defined(HAVE_AS_XSAVEOPT)
>> -# define CLWB_ENCODING "data16 xsaveopt %[p]" /* clwb */
>> -#else
>> -# define CLWB_ENCODING ".byte 0x66, 0x0f, 0xae, 0x30" /* clwb (%%rax) */
>> -#endif
>> -
>> -#define BASE_INPUT(addr) [p] "m" (*(const char *)(addr))
>> -#if defined(HAVE_AS_CLWB) || defined(HAVE_AS_XSAVEOPT)
>> -# define INPUT BASE_INPUT
>> -#else
>> -# define INPUT(addr) "a" (addr), BASE_INPUT(addr)
>> -#endif
>> -
>> -        asm volatile (CLWB_ENCODING :: INPUT(addr));
>> -
>> -#undef INPUT
>> -#undef BASE_INPUT
>> -#undef CLWB_ENCODING
>> -    }
>> +        asm volatile ("clwb %[p]" :: [p] "m" (*(const char *)(addr)));
> One minor note about whitespace.  We typically have spaces inside the
> outermost brackets on asm statements, as per the clwb() example below.
>
> Also, given the expression is so simple, I'd just use %0 and drop the
> [p].  It's just line-verbosity here.
>
> Can fix both on commit if you're happy.
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Also, I forgot to write in the ticket, clflushopt wants similar
treatment, even if there isn't an outward define for it.  I think the
following two hunks should do:

~Andrew

diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
index 18748b2bc805..ef30ef546336 100644
--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -287,7 +287,7 @@ void cache_flush(const void *addr, unsigned int size)
          * of letting the alternative framework fill the gap by
appending nops.
          */
         alternative_io("ds; clflush %[p]",
-                       "data16 clflush %[p]", /* clflushopt */
+                       "clflushopt %[p]",
                        X86_FEATURE_CLFLUSHOPT,
                        /* no outputs */,
                        [p] "m" (*(const char *)(addr)));
diff --git a/xen/arch/x86/include/asm/system.h
b/xen/arch/x86/include/asm/system.h
index 73cb16ca68d6..6f5b6d502911 100644
--- a/xen/arch/x86/include/asm/system.h
+++ b/xen/arch/x86/include/asm/system.h
@@ -23,7 +23,7 @@ static inline void clflush(const void *p)
 
 static inline void clflushopt(const void *p)
 {
-    asm volatile ( "data16 clflush %0" :: "m" (*(const char *)p) );
+    asm volatile ( "clflushopt %0" :: "m" (*(const char *)p) );
 }
 
 static inline void clwb(const void *p)