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.coop...@citrix.com>
> A little hesitantly
> Reviewed-by: Jan Beulich <jbeul...@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
