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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/09/2025 11:48 am, Jan Beulich wrote:
> While adding new enumerators one may overlook the (rare) need to bump
> X86_NR_{SYNTH,BUG}. Guard against that happening by adding respective
> checking. The use of BUILD_BUG_ON_ZERO(), however, entails a number of
> other changes, as the expansion may not appear in the assembly produced.
> Furthermore inputs to file-scope asm() are only supported in gcc15 (or
> newer).
>
> No difference in generated code (debug info, however, grows quite a bit).
>
> An implication from the changes is that users of the alternatives patching
> macros may not use unnamed asm() input operands anymore, as the "injected"
> new operands would break numbering expectations.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- 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?


> --- 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?

>  #include <public/arch-x86/cpufeatureset.h>
>  #include <asm/cpufeatures.h>
>  
> --- 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.

>  
>  static inline unsigned long pfn_to_pdx(unsigned long pfn)
>  {
> --- 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.

~Andrew