Re: [PATCH 2/2] x86: guard synthetic feature and bug enumerators

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.10.2025 19:19, Andrew Cooper wrote:
> On 25/09/2025 11:48 am, Jan Beulich wrote:
>> --- a/xen/arch/x86/include/asm/alternative.h
>> +++ b/xen/arch/x86/include/asm/alternative.h
>> @@ -70,12 +70,12 @@ extern void alternative_instructions(voi
>>                      alt_repl_len(n2)) "-" alt_orig_len)
>>  
>>  #define ALTINSTR_ENTRY(feature, num)                                    \
>> -        " .if (" STR(feature & ~ALT_FLAG_NOT) ") >= " STR(NCAPINTS * 32) 
>> "\n" \
>> +        " .if (%c" #feature " & " STR(~ALT_FLAG_NOT) ") >= " STR(NCAPINTS * 
>> 32) "\n" \
>>          " .error \"alternative feature outside of featureset range\"\n" \
>>          " .endif\n"                                                     \
>>          " .long .LXEN%=_orig_s - .\n"             /* label           */ \
>>          " .long " alt_repl_s(num)" - .\n"         /* new instruction */ \
>> -        " .word " STR(feature) "\n"               /* feature bit     */ \
>> +        " .word %c" #feature "\n"                 /* feature bit     */ \
>>          " .byte " alt_orig_len "\n"               /* source len      */ \
>>          " .byte " alt_repl_len(num) "\n"          /* replacement len */ \
>>          " .byte " alt_pad_len "\n"                /* padding len     */ \
>> @@ -127,14 +127,14 @@ extern void alternative_instructions(voi
>>   */
>>  #define alternative(oldinstr, newinstr, feature)                        \
>>      asm_inline volatile (                                               \
>> -        ALTERNATIVE(oldinstr, newinstr, feature)                        \
>> -        ::: "memory" )
>> +        ALTERNATIVE(oldinstr, newinstr, [feat])                         \
>> +        :: [feat] "i" (feature) : "memory" )
> 
> I don't understand.  How is this related to putting the guard in place?

The change here is needed to fit the change to ALTINSTR_ENTRY() above. That
change in turn is needed because

#define X86_SYNTH(x) (FSCAPINTS * 32 + (x) + X86_CHECK_FEAT_NR(x, SYNTH))

with

#define X86_CHECK_FEAT_NR(x, n) BUILD_BUG_ON_ZERO((x) / 32 >= X86_NR_ ## n)

now needs to be evaluated by the compiler. If we used stringification as
before, the assembler would get to see BUILD_BUG_ON_ZERO().

>> --- a/xen/arch/x86/include/asm/cpufeatureset.h
>> +++ b/xen/arch/x86/include/asm/cpufeatureset.h
>> @@ -12,8 +12,13 @@ enum {
>>  };
>>  #undef XEN_CPUFEATURE
>>  
>> +#if __GNUC__ >= 15
>> +#define XEN_CPUFEATURE(name, value) asm (".equ X86_FEATURE_" #name ", %c0" \
>> +                                         :: "i" (X86_FEATURE_##name));
>> +#else
>>  #define XEN_CPUFEATURE(name, value) asm (".equ X86_FEATURE_" #name ", " \
>>                                           __stringify(value));
>> +#endif
> 
> Again - why are we making a no-op change for the sake of it?

See above.

>> --- a/xen/arch/x86/include/asm/pdx.h
>> +++ b/xen/arch/x86/include/asm/pdx.h
>> @@ -13,9 +13,9 @@
>>      asm_inline goto (                               \
>>          ALTERNATIVE(                                \
>>              "",                                     \
>> -            "jmp %l0",                              \
>> -            ALT_NOT(X86_FEATURE_PDX_COMPRESSION))   \
>> -        : : : : label )
>> +            "jmp %l1",                              \
>> +            [feat])                                 \
>> +        : : [feat] "i" (ALT_NOT(X86_FEATURE_PDX_COMPRESSION)) : : label )
> 
> Not a bug in this change, but the pre-existing use of positional labels
> is something I was expecting not to introduce at all seeing as we
> started cleanly with named labels.
> 
> The jmp wants to be:
> 
>   "jmp %l" #label
> 
> to cope with the fact it's a macro parameter too.

Unrelated change? I can of course do the adjustment in a separate prereq
patch, but then it would have been nice if you had commented along these
lines before that code actually had gone in.

That said, isn't it at least bad practice to not expose the label use to
the compiler? To avoid using positional operands, shouldn't we rather
name the operand, and then use "jmp %l[whatever_the_name]"? That's a
change I could see as being justified to do right here, rather than in a
separate patch.

>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>> @@ -73,7 +73,7 @@ static always_inline void spec_ctrl_new_
>>  
>>      /* (ab)use alternative_input() to specify clobbers. */
>>      alternative_input("", "DO_OVERWRITE_RSB xu=%=", X86_BUG_IBPB_NO_RET,
>> -                      : "rax", "rcx");
>> +                      "i" (0) : "rax", "rcx");
>>  }
> 
> As the comment says, this is already an abuse of the macro for a purpose
> it wasn't intended for.
> 
> Now requiring an extra "nop" parameter to get the abuse to compile is
> too far.  It can turn into a plain ALTERNATIVE(), and then both disappear.

Except that, for the reasons explained further up, I'd rather not see new
explicit uses of ALTERNATIVE() appear. In the end it's not clear which
one is the lesser evil. Maybe we should gain alternative_clobber()?

Jan