| Message ID | cdf93d64-dcc0-4e01-88fe-71145ffff1ff@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: restore semicolon after explicit DS prefix | expand |
On 08/10/2024 5:00 pm, Jan Beulich wrote: > It's not unnecessary (as the earlier commit claimed): The integrated > assembler of Clang up to 11 complains about an "invalid operand for > instruction". > > Fixes: b42cf31d1165 ("x86: use alternative_input() in cache_flush()") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/flushtlb.c > +++ b/xen/arch/x86/flushtlb.c > @@ -286,7 +286,7 @@ void cache_flush(const void *addr, unsig > * + prefix than a clflush + nop, and hence the prefix is added instead > * of letting the alternative framework fill the gap by appending nops. > */ > - alternative_input("ds clflush %[p]", > + alternative_input("ds; clflush %[p]", /* Clang-IAS < 12 needs the semicolon */ which can probably fit on the end of the line. Or we stop supporting such old versions of Clang/LLVM. ~Andrew
On 08.10.2024 18:37, Andrew Cooper wrote: > On 08/10/2024 5:00 pm, Jan Beulich wrote: >> It's not unnecessary (as the earlier commit claimed): The integrated >> assembler of Clang up to 11 complains about an "invalid operand for >> instruction". >> >> Fixes: b42cf31d1165 ("x86: use alternative_input() in cache_flush()") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/flushtlb.c >> +++ b/xen/arch/x86/flushtlb.c >> @@ -286,7 +286,7 @@ void cache_flush(const void *addr, unsig >> * + prefix than a clflush + nop, and hence the prefix is added instead >> * of letting the alternative framework fill the gap by appending nops. >> */ >> - alternative_input("ds clflush %[p]", >> + alternative_input("ds; clflush %[p]", > > /* Clang-IAS < 12 needs the semicolon */ which can probably fit on the > end of the line. I've made it "Semicolon for Clang-IAS < 12" to actually fit on the line. I wonder whether I can take the reply as "ack with that change"? > Or we stop supporting such old versions of Clang/LLVM. As indicated in reply to Roger's proposal, that would leave me without any way to test with at least some Clang versions (unless I got into the business of also building my own Clang binaries). IOW - I could live with such a move, but I'd prefer us not to be overly aggressive there. Jan
On 09/10/2024 7:04 am, Jan Beulich wrote: > On 08.10.2024 18:37, Andrew Cooper wrote: >> On 08/10/2024 5:00 pm, Jan Beulich wrote: >>> It's not unnecessary (as the earlier commit claimed): The integrated >>> assembler of Clang up to 11 complains about an "invalid operand for >>> instruction". >>> >>> Fixes: b42cf31d1165 ("x86: use alternative_input() in cache_flush()") >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> --- a/xen/arch/x86/flushtlb.c >>> +++ b/xen/arch/x86/flushtlb.c >>> @@ -286,7 +286,7 @@ void cache_flush(const void *addr, unsig >>> * + prefix than a clflush + nop, and hence the prefix is added instead >>> * of letting the alternative framework fill the gap by appending nops. >>> */ >>> - alternative_input("ds clflush %[p]", >>> + alternative_input("ds; clflush %[p]", >> /* Clang-IAS < 12 needs the semicolon */ which can probably fit on the >> end of the line. > I've made it "Semicolon for Clang-IAS < 12" to actually fit on the line. > > I wonder whether I can take the reply as "ack with that change"? Yeah, that's good enough. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> Or we stop supporting such old versions of Clang/LLVM. > As indicated in reply to Roger's proposal, that would leave me without > any way to test with at least some Clang versions (unless I got into the > business of also building my own Clang binaries). Or using the upstream provided builds. (Seriously - this is something the LLVM community completely outshine the GCC/Binutils community.) Or using Gitlab CI. ~Andrew
--- a/xen/arch/x86/flushtlb.c +++ b/xen/arch/x86/flushtlb.c @@ -286,7 +286,7 @@ void cache_flush(const void *addr, unsig * + prefix than a clflush + nop, and hence the prefix is added instead * of letting the alternative framework fill the gap by appending nops. */ - alternative_input("ds clflush %[p]", + alternative_input("ds; clflush %[p]", "data16 clflush %[p]", /* clflushopt */ X86_FEATURE_CLFLUSHOPT, [p] "m" (*(const char *)(addr)));
It's not unnecessary (as the earlier commit claimed): The integrated assembler of Clang up to 11 complains about an "invalid operand for instruction". Fixes: b42cf31d1165 ("x86: use alternative_input() in cache_flush()") Signed-off-by: Jan Beulich <jbeulich@suse.com>