| Message ID | 20230217122203.790737-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] xen: Work around Clang-IAS macro expansion bug | expand |
On 17.02.2023 13:22, Andrew Cooper wrote: > https://github.com/llvm/llvm-project/issues/60792 > > It turns out that Clang-IAS does not expand \@ uniquely in a translaition > unit, and the XSA-426 change tickles this bug: > > <instantiation>:4:1: error: invalid symbol redefinition > .L1_fill_rsb_loop: > ^ > make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1 > > Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in > too, which Clang does seem to expand properly. > > Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > > v1 (vs RFC): > * Rename \foo to \x, reorder WRT \@ to avoid needing \() too. > > I originally tried a parameter named uniq but this found a different Clang-IAS > bug: > > In file included from arch/x86/asm-macros.c:3: > ./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode] > .L\@\uniq\()fill_rsb_loop: > ^ > > It appears that Clang is looking for unicode escapes before completing > parameter substitution. But the macro didn't originate from a context where a > unicode escape was even applicable to begin with. > > The consequence is that you can't use macro prameters beginning with a u. How nice. If really needed, I guess we could use -Wno-unicode on the assumption that we don't use anything that could legitimately trigger that warning. > --- a/xen/arch/x86/include/asm/spec_ctrl.h > +++ b/xen/arch/x86/include/asm/spec_ctrl.h > @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void) > wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB); > > /* (ab)use alternative_input() to specify clobbers. */ > - alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET, > + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET, > : "rax", "rcx"); > } > > @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct cpu_info *info) > * > * (ab)use alternative_input() to specify clobbers. > */ > - alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE, > + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE, > : "rax", "rcx"); > } Since inside the macro you retain the uses of \@, I'd find it desirable to keep gcc-generated code tidy by omitting the extra argument there. This could be done by introducing another C macro along the lines of: #ifdef __clang__ #define CLANG_UNIQUE(name) " " #name "=%=" #else #define CLANG_UNIQUE(name) #endif Besides the less confusing label names this would also have the benefit of providing a link at the use sites to what the issue is that is being worked around. Plus, once (if ever) fixed in Clang, we could then adjust the condition to just the affected versions. > --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h > +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h > @@ -117,11 +117,15 @@ > .L\@_done: > .endm > > -.macro DO_OVERWRITE_RSB tmp=rax > +.macro DO_OVERWRITE_RSB tmp=rax x= The "=" in "x=" isn't needed, is it? It looks a little confusing to me, raising the question "Why is this there?" Furthermore I think it would be good practice if macro parameters were comma-separated. I realize we don't have this anyway near consistent right now, but still. > /* > * Requires nothing > * Clobbers \tmp (%rax by default), %rcx > * > + * x= is an optional parameter to work around > + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't > + * expand \@ uniquely, and is intended for muxing %= in too. With the above suggestion I'd see this comment move to next to that helper macro, with a link to there left here. Just to clarify: I'm not going to insist on any of the suggested adjustments, but personally I think they would be beneficial. If you are pretty certain the other way around, please let me know, and I'll consider ack-ing (largely) as-is. Jan
On 21/02/2023 10:31 am, Jan Beulich wrote: > On 17.02.2023 13:22, Andrew Cooper wrote: >> https://github.com/llvm/llvm-project/issues/60792 >> >> It turns out that Clang-IAS does not expand \@ uniquely in a translaition >> unit, and the XSA-426 change tickles this bug: >> >> <instantiation>:4:1: error: invalid symbol redefinition >> .L1_fill_rsb_loop: >> ^ >> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1 >> >> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in >> too, which Clang does seem to expand properly. >> >> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions") >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Wei Liu <wl@xen.org> >> >> v1 (vs RFC): >> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too. Sadly, this is not quite correct. There needs to be a non-numeric character separating \@ and \x or the concatenation of the two can end up non-unique. So I think we need the \(). >> I originally tried a parameter named uniq but this found a different Clang-IAS >> bug: >> >> In file included from arch/x86/asm-macros.c:3: >> ./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode] >> .L\@\uniq\()fill_rsb_loop: >> ^ >> >> It appears that Clang is looking for unicode escapes before completing >> parameter substitution. But the macro didn't originate from a context where a >> unicode escape was even applicable to begin with. >> >> The consequence is that you can't use macro prameters beginning with a u. > How nice. If really needed, I guess we could use -Wno-unicode on the > assumption that we don't use anything that could legitimately trigger that > warning. It's proving very stubborn to narrow down. I really can't reproduce it outside of this specific occurrence in the Xen build system, but my gut feeling is that it's something to do with the asm level .include. >> --- a/xen/arch/x86/include/asm/spec_ctrl.h >> +++ b/xen/arch/x86/include/asm/spec_ctrl.h >> @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void) >> wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB); >> >> /* (ab)use alternative_input() to specify clobbers. */ >> - alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET, >> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET, >> : "rax", "rcx"); >> } >> >> @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct cpu_info *info) >> * >> * (ab)use alternative_input() to specify clobbers. >> */ >> - alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE, >> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE, >> : "rax", "rcx"); >> } > Since inside the macro you retain the uses of \@, I'd find it desirable > to keep gcc-generated code tidy by omitting the extra argument there. As I said in the RFC, I tried to have x=\@ but gas really didn't like that. But given the concatenation observation, we also cannot simply replace \@ with %= (given the option, which I couldn't figure out), because they can overlap. > This could be done by introducing another C macro along the lines of: > > #ifdef __clang__ > #define CLANG_UNIQUE(name) " " #name "=%=" > #else > #define CLANG_UNIQUE(name) > #endif > > Besides the less confusing label names this would also have the benefit > of providing a link at the use sites to what the issue is that is being > worked around. Plus, once (if ever) fixed in Clang, we could then adjust > the condition to just the affected versions. It's only Clang IAS, but there's no suitable define to figure this out. And while I appreciate the effort to be more descriptive, this name is literally longer than the thing it wraps and ... >> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h >> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h >> @@ -117,11 +117,15 @@ >> .L\@_done: >> .endm >> >> -.macro DO_OVERWRITE_RSB tmp=rax >> +.macro DO_OVERWRITE_RSB tmp=rax x= > The "=" in "x=" isn't needed, is it? It looks a little confusing to me, > raising the question "Why is this there?" Because I started out with u=\@ >> /* >> * Requires nothing >> * Clobbers \tmp (%rax by default), %rcx >> * >> + * x= is an optional parameter to work around >> + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't >> + * expand \@ uniquely, and is intended for muxing %= in too. > With the above suggestion I'd see this comment move to next to that > helper macro, with a link to there left here. ... there's no getting rid of this comment, at least in some form. The parameter is odd, and needs explaining. Passing %= unconditionally doesn't matter for GCC/Binuitls, and in this case, attempts to "declutter" the niche usecase of inspecting the assembled file comes with a substantial complexity at the C level. It's really not worth the extra complexity. ~Andrew
On 21.02.2023 13:26, Andrew Cooper wrote: > On 21/02/2023 10:31 am, Jan Beulich wrote: >> On 17.02.2023 13:22, Andrew Cooper wrote: >>> https://github.com/llvm/llvm-project/issues/60792 >>> >>> It turns out that Clang-IAS does not expand \@ uniquely in a translaition >>> unit, and the XSA-426 change tickles this bug: >>> >>> <instantiation>:4:1: error: invalid symbol redefinition >>> .L1_fill_rsb_loop: >>> ^ >>> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1 >>> >>> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in >>> too, which Clang does seem to expand properly. >>> >>> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions") >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> CC: Wei Liu <wl@xen.org> >>> >>> v1 (vs RFC): >>> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too. > > Sadly, this is not quite correct. There needs to be a non-numeric > character separating \@ and \x or the concatenation of the two can end > up non-unique. How that if \@ is always 1? > So I think we need the \(). Or put one at the start of the label and the other at the end? >>> --- a/xen/arch/x86/include/asm/spec_ctrl.h >>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h >>> @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void) >>> wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB); >>> >>> /* (ab)use alternative_input() to specify clobbers. */ >>> - alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET, >>> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET, >>> : "rax", "rcx"); >>> } >>> >>> @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct cpu_info *info) >>> * >>> * (ab)use alternative_input() to specify clobbers. >>> */ >>> - alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE, >>> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE, >>> : "rax", "rcx"); >>> } >> Since inside the macro you retain the uses of \@, I'd find it desirable >> to keep gcc-generated code tidy by omitting the extra argument there. > > As I said in the RFC, I tried to have x=\@ but gas really didn't like that. > > But given the concatenation observation, we also cannot simply replace > \@ with %= (given the option, which I couldn't figure out), because they > can overlap. > >> This could be done by introducing another C macro along the lines of: >> >> #ifdef __clang__ >> #define CLANG_UNIQUE(name) " " #name "=%=" >> #else >> #define CLANG_UNIQUE(name) >> #endif >> >> Besides the less confusing label names this would also have the benefit >> of providing a link at the use sites to what the issue is that is being >> worked around. Plus, once (if ever) fixed in Clang, we could then adjust >> the condition to just the affected versions. > > It's only Clang IAS, but there's no suitable define to figure this out. "this" being what? The case of Clang but with / without integrated as? Since we have to turn that off, we could as well add a -D option along with the -no-integrated-as one. > And while I appreciate the effort to be more descriptive, this name is > literally longer than the thing it wraps and ... > >>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h >>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h >>> @@ -117,11 +117,15 @@ >>> .L\@_done: >>> .endm >>> >>> -.macro DO_OVERWRITE_RSB tmp=rax >>> +.macro DO_OVERWRITE_RSB tmp=rax x= >> The "=" in "x=" isn't needed, is it? It looks a little confusing to me, >> raising the question "Why is this there?" > > Because I started out with u=\@ > >>> /* >>> * Requires nothing >>> * Clobbers \tmp (%rax by default), %rcx >>> * >>> + * x= is an optional parameter to work around >>> + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't >>> + * expand \@ uniquely, and is intended for muxing %= in too. >> With the above suggestion I'd see this comment move to next to that >> helper macro, with a link to there left here. > > ... there's no getting rid of this comment, at least in some form. The > parameter is odd, and needs explaining. Of course, hence my "with a link to there left here". > Passing %= unconditionally doesn't matter for GCC/Binuitls, and in this > case, attempts to "declutter" the niche usecase of inspecting the > assembled file comes with a substantial complexity at the C level. > It's really not worth the extra complexity. I wouldn't call that one simple macro "complexity". I'm in particular not advocating for (conditionally) removing the extra macro parameter. Jan
On 21/02/2023 12:46 pm, Jan Beulich wrote: > On 21.02.2023 13:26, Andrew Cooper wrote: >> On 21/02/2023 10:31 am, Jan Beulich wrote: >>> On 17.02.2023 13:22, Andrew Cooper wrote: >>>> https://github.com/llvm/llvm-project/issues/60792 >>>> >>>> It turns out that Clang-IAS does not expand \@ uniquely in a translaition >>>> unit, and the XSA-426 change tickles this bug: >>>> >>>> <instantiation>:4:1: error: invalid symbol redefinition >>>> .L1_fill_rsb_loop: >>>> ^ >>>> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1 >>>> >>>> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in >>>> too, which Clang does seem to expand properly. >>>> >>>> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions") >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> --- >>>> CC: Jan Beulich <JBeulich@suse.com> >>>> CC: Roger Pau Monné <roger.pau@citrix.com> >>>> CC: Wei Liu <wl@xen.org> >>>> >>>> v1 (vs RFC): >>>> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too. >> Sadly, this is not quite correct. There needs to be a non-numeric >> character separating \@ and \x or the concatenation of the two can end >> up non-unique. > How that if \@ is always 1? It isn't always 1. Global (file scope) have \@ expand properly from 0 to N. Function scope have \@ expand properly from 0 to N in a single asm() statement. The 1 here is actually because mknops took the 0 and didn't use it. If instead we had something like: asm ("DO_OVERWRITE_RSB"); // \@ = 0 asm ("mkops 2;" "DO_OVERWRITE_RSB"); // \@ = 1 then it would assemble fine, but the way we build alternatives in asm() statements reliably causes the alternative block to consume \@=1. >> So I think we need the \(). > Or put one at the start of the label and the other at the end? We already played that trick with \n for the .irp, which suffers similarly with concatenation. ~Andrew
diff --git a/xen/arch/x86/include/asm/spec_ctrl.h b/xen/arch/x86/include/asm/spec_ctrl.h index 3cf8a7d304d4..ddb2247fcb0a 100644 --- a/xen/arch/x86/include/asm/spec_ctrl.h +++ b/xen/arch/x86/include/asm/spec_ctrl.h @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void) wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB); /* (ab)use alternative_input() to specify clobbers. */ - alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET, + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET, : "rax", "rcx"); } @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct cpu_info *info) * * (ab)use alternative_input() to specify clobbers. */ - alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE, + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE, : "rax", "rcx"); } diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h index fab27ff5532b..1220f219f695 100644 --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h @@ -117,11 +117,15 @@ .L\@_done: .endm -.macro DO_OVERWRITE_RSB tmp=rax +.macro DO_OVERWRITE_RSB tmp=rax x= /* * Requires nothing * Clobbers \tmp (%rax by default), %rcx * + * x= is an optional parameter to work around + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't + * expand \@ uniquely, and is intended for muxing %= in too. + * * Requires 256 bytes of {,shadow}stack space, but %rsp/SSP has no net * change. Based on Google's performance numbers, the loop is unrolled to 16 * iterations and two calls per iteration. @@ -136,27 +140,27 @@ mov $16, %ecx /* 16 iterations, two calls per loop */ mov %rsp, %\tmp /* Store the current %rsp */ -.L\@_fill_rsb_loop: +.L\x\@_fill_rsb_loop: .irp n, 1, 2 /* Unrolled twice. */ - call .L\@_insert_rsb_entry_\n /* Create an RSB entry. */ + call .L\x\@_insert_rsb_entry_\n /* Create an RSB entry. */ int3 /* Halt rogue speculation. */ -.L\@_insert_rsb_entry_\n: +.L\x\@_insert_rsb_entry_\n: .endr sub $1, %ecx - jnz .L\@_fill_rsb_loop + jnz .L\x\@_fill_rsb_loop mov %\tmp, %rsp /* Restore old %rsp */ #ifdef CONFIG_XEN_SHSTK mov $1, %ecx rdsspd %ecx cmp $1, %ecx - je .L\@_shstk_done + je .L\x\@_shstk_done mov $64, %ecx /* 64 * 4 bytes, given incsspd */ incsspd %ecx /* Restore old SSP */ -.L\@_shstk_done: +.L\x\@_shstk_done: #endif .endm
https://github.com/llvm/llvm-project/issues/60792 It turns out that Clang-IAS does not expand \@ uniquely in a translaition unit, and the XSA-426 change tickles this bug: <instantiation>:4:1: error: invalid symbol redefinition .L1_fill_rsb_loop: ^ make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1 Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in too, which Clang does seem to expand properly. Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> v1 (vs RFC): * Rename \foo to \x, reorder WRT \@ to avoid needing \() too. I originally tried a parameter named uniq but this found a different Clang-IAS bug: In file included from arch/x86/asm-macros.c:3: ./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode] .L\@\uniq\()fill_rsb_loop: ^ It appears that Clang is looking for unicode escapes before completing parameter substitution. But the macro didn't originate from a context where a unicode escape was even applicable to begin with. The consequence is that you can't use macro prameters beginning with a u. --- xen/arch/x86/include/asm/spec_ctrl.h | 4 ++-- xen/arch/x86/include/asm/spec_ctrl_asm.h | 18 +++++++++++------- 2 files changed, 13 insertions(+), 9 deletions(-)