| Message ID | 96f493ee-a360-ce46-7a61-5f55ca436295@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: correct fencing around CLFLUSH (+some tidying) | expand |
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
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
--- 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
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().