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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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>


>  
>      asm volatile ("sfence" ::: "memory");
>  }
> diff --git a/xen/arch/x86/include/asm/system.h 
> b/xen/arch/x86/include/asm/system.h
> index 8ceaaf45d1..c3529f99dd 100644
> --- a/xen/arch/x86/include/asm/system.h
> +++ b/xen/arch/x86/include/asm/system.h
> @@ -28,14 +28,7 @@ static inline void clflushopt(const void *p)
>  
>  static inline void clwb(const void *p)
>  {
> -#if defined(HAVE_AS_CLWB)
>      asm volatile ( "clwb %0" :: "m" (*(const char *)p) );
> -#elif defined(HAVE_AS_XSAVEOPT)
> -    asm volatile ( "data16 xsaveopt %0" :: "m" (*(const char *)p) );
> -#else
> -    asm volatile ( ".byte 0x66, 0x0f, 0xae, 0x32"
> -                   :: "d" (p), "m" (*(const char *)p) );
> -#endif
>  }
>  
>  #define xchg(ptr,v) \