| Message ID | 20230223203659.2594851-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] xen: Work around Clang-IAS macro \@ expansion bug | expand |
On 23.02.2023 21:36, 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 mix %= 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> A little hesitantly Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- 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 xu=%=", X86_BUG_IBPB_NO_RET, Especially with there possibly appearing more cases where we need to add such, wrapping the extra parameter in a C macro continues to seem better to me, for having a minimal level of documentation (I has CLANG in the suggested name for exactly this purpose) right at the place of use. The way you have it you force readers to go look up the assembler macro and read through the commentary there in order to find any explanation for the oddity. Jan
On 24/02/2023 7:14 am, Jan Beulich wrote: > On 23.02.2023 21:36, 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 mix %= 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> > A little hesitantly > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > >> --- 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 xu=%=", X86_BUG_IBPB_NO_RET, > Especially with there possibly appearing more cases where we need to > add such, wrapping the extra parameter in a C macro continues to > seem better to me, for having a minimal level of documentation (I > has CLANG in the suggested name for exactly this purpose) right at > the place of use. The way you have it you force readers to go look > up the assembler macro and read through the commentary there in order > to find any explanation for the oddity. I'm not massively happy with the patch in this form, but it is less bad than splitting the name out. We do not separate out parameters elsewhere. Doing so adds cognitive complexity to following the C, because now instead of having 2 places to look at to figure out what's going on, you have 3. Even a name like CLANG_EXTRA_UNIQUE is only meaningful to you and me. Everyone else has to go and find the two other places to understand what's going on. Finally, and most importantly, despite having moved to a 2-char macro parameter name, there's still not a meaningful identifier here in C that's shorter. I do understand the want to try and make this more obvious, but adding cognitive complexity and code volume isn't a good way of improving things. ~Andrew
diff --git a/xen/arch/x86/include/asm/spec_ctrl.h b/xen/arch/x86/include/asm/spec_ctrl.h index 3cf8a7d304d4..f718f94088a1 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 xu=%=", 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 xu=%=", 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..f23bb105c51e 100644 --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h @@ -117,11 +117,16 @@ .L\@_done: .endm -.macro DO_OVERWRITE_RSB tmp=rax +.macro DO_OVERWRITE_RSB tmp=rax xu /* * Requires nothing * Clobbers \tmp (%rax by default), %rcx * + * xu is an optional parameter to add eXtra Uniqueness. It is intended for + * passing %= in from an asm() block, in order to work around + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't + * expand \@ uniquely. + * * 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 +141,27 @@ mov $16, %ecx /* 16 iterations, two calls per loop */ mov %rsp, %\tmp /* Store the current %rsp */ -.L\@_fill_rsb_loop: +.L\@_fill_rsb_loop\xu: .irp n, 1, 2 /* Unrolled twice. */ - call .L\@_insert_rsb_entry_\n /* Create an RSB entry. */ + call .L\@_insert_rsb_entry\xu\n /* Create an RSB entry. */ int3 /* Halt rogue speculation. */ -.L\@_insert_rsb_entry_\n: +.L\@_insert_rsb_entry\xu\n: .endr sub $1, %ecx - jnz .L\@_fill_rsb_loop + jnz .L\@_fill_rsb_loop\xu mov %\tmp, %rsp /* Restore old %rsp */ #ifdef CONFIG_XEN_SHSTK mov $1, %ecx rdsspd %ecx cmp $1, %ecx - je .L\@_shstk_done + je .L\@_shstk_done\xu mov $64, %ecx /* 64 * 4 bytes, given incsspd */ incsspd %ecx /* Restore old SSP */ -.L\@_shstk_done: +.L\@_shstk_done\xu: #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 mix %= 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> v2: * Drop trailing = from parameter declaration. Rename to xu and adjust comment * Move \xu to the end of labels, to avoid directly concatinating with \@ 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 | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-)