Re: [PATCH v1] xen: Work around Clang-IAS macro expansion bug

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> 
> 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