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 <alexan...@edera.dev>
>> ---
>> 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.coop...@citrix.com>
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)
