| Message ID | 3de09e4e2a3320e0f314803e349fbe6520d04b12.1743719892.git.alexander@edera.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: drop XSAVEOPT and CLWB build flags | expand |
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@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.cooper3@citrix.com> > > 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) \
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@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.cooper3@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)
On 2025-04-03 19:28, 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. Will try to be more detailed in the commit message next time, thanks for pointing out. >> + 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. Makes sense. I had searched for existing examples of this and saw a mix with and without spaces. > Also, given the expression is so simple, I'd just use %0 and drop the > [p]. It's just line-verbosity here. Yes, agreed. > Can fix both on commit if you're happy. > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Yes please do. Thanks for the review!
On 04.04.2025 01:22, Alexander M. Merritt wrote: > The new toolchain baseline knows both the XSAVEOPT and CLWB instructions. > > Resolves: https://gitlab.com/xen-project/xen/-/work_items/205 > Signed-off-by: Alexander M. Merritt <alexander@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(-) For XSAVEOPT there's more work to do, even if not connected via HAVE_AS_XSAVEOPT. Look for "xsaveopt" (case-insensitively) in xstate.c. Imo (just like was asked for for the RDRAND counterpart patch) this wants doing all in one go. Jan
On 04/04/2025 8:21 am, Jan Beulich wrote: > On 04.04.2025 01:22, Alexander M. Merritt wrote: >> The new toolchain baseline knows both the XSAVEOPT and CLWB instructions. >> >> Resolves: https://gitlab.com/xen-project/xen/-/work_items/205 >> Signed-off-by: Alexander M. Merritt <alexander@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(-) > For XSAVEOPT there's more work to do, even if not connected via HAVE_AS_XSAVEOPT. > Look for "xsaveopt" (case-insensitively) in xstate.c. Imo (just like was asked > for for the RDRAND counterpart patch) this wants doing all in one go. I've got a different task pending for xsave. It's a bit more involved than simply dropping the -D's. We'll get to it. ~Andrew
On 2025-04-03 19:34, Andrew Cooper wrote: > On 04/04/2025 12:28 am, Andrew Cooper wrote: > 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]", Agree on these changes. However, I see branch staging uses alternative_input and the below /* no outputs */ does not exist. > 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)
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))); 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) \
The new toolchain baseline knows both the XSAVEOPT and CLWB instructions. Resolves: https://gitlab.com/xen-project/xen/-/work_items/205 Signed-off-by: Alexander M. Merritt <alexander@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(-)