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@suse.com>
--- 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" )
#define alternative_2(oldinstr, newinstr1, feature1, newinstr2, feature2) \
asm_inline volatile ( \
- ALTERNATIVE_2(oldinstr, newinstr1, feature1, \
- newinstr2, feature2) \
- ::: "memory" )
+ ALTERNATIVE_2(oldinstr, newinstr1, [feat1], \
+ newinstr2, [feat2]) \
+ :: [feat1] "i" (feature1), [feat2] "i" (feature2) : "memory" )
/*
* Alternative inline assembly with input.
@@ -148,14 +148,14 @@ extern void alternative_instructions(voi
*/
#define alternative_input(oldinstr, newinstr, feature, input...) \
asm_inline volatile ( \
- ALTERNATIVE(oldinstr, newinstr, feature) \
- :: input )
+ ALTERNATIVE(oldinstr, newinstr, [feat]) \
+ :: [feat] "i" (feature), ## input )
/* Like alternative_input, but with a single output argument */
#define alternative_io(oldinstr, newinstr, feature, output, input...) \
asm_inline volatile ( \
- ALTERNATIVE(oldinstr, newinstr, feature) \
- : output : input )
+ ALTERNATIVE(oldinstr, newinstr, [feat]) \
+ : output : [feat] "i" (feature), ## input )
/*
* This is similar to alternative_io. But it has two features and
@@ -168,9 +168,9 @@ extern void alternative_instructions(voi
#define alternative_io_2(oldinstr, newinstr1, feature1, newinstr2, \
feature2, output, input...) \
asm_inline volatile ( \
- ALTERNATIVE_2(oldinstr, newinstr1, feature1, \
- newinstr2, feature2) \
- : output : input )
+ ALTERNATIVE_2(oldinstr, newinstr1, [feat1], \
+ newinstr2, [feat2]) \
+ : output : [feat1] "i" (feature1), [feat2] "i" (feature2), ## input )
/* Use this macro(s) if you need more than one output parameter. */
#define ASM_OUTPUT2(a...) a
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -6,9 +6,16 @@
/* Number of capability words covered by the featureset words. */
#define FSCAPINTS FEATURESET_NR_ENTRIES
+#if !defined(__ASSEMBLY__) && __GNUC__ >= 15
+#include <xen/macros.h>
+#define X86_CHECK_FEAT_NR(x, n) BUILD_BUG_ON_ZERO((x) / 32 >= X86_NR_ ## n)
+#else
+#define X86_CHECK_FEAT_NR(x, n) 0
+#endif
+
/* Synthetic words follow the featureset words. */
#define X86_NR_SYNTH 2
-#define X86_SYNTH(x) (FSCAPINTS * 32 + (x))
+#define X86_SYNTH(x) (FSCAPINTS * 32 + (x) + X86_CHECK_FEAT_NR(x, SYNTH))
/* Synthetic features */
XEN_CPUFEATURE(CONSTANT_TSC, X86_SYNTH( 0)) /* TSC ticks at a constant rate */
@@ -47,7 +54,8 @@ XEN_CPUFEATURE(XEN_REP_MOVSB, X86_SY
/* Bug words follow the synthetic words. */
#define X86_NR_BUG 1
-#define X86_BUG(x) ((FSCAPINTS + X86_NR_SYNTH) * 32 + (x))
+#define X86_BUG(x) ((FSCAPINTS + X86_NR_SYNTH) * 32 + (x) + \
+ X86_CHECK_FEAT_NR(x, BUG))
#define X86_BUG_FPU_PTRS X86_BUG( 0) /* (F)X{SAVE,RSTOR} doesn't save/restore FOP/FIP/FDP. */
#define X86_BUG_NULL_SEG X86_BUG( 1) /* NULL-ing a selector preserves the base and limit. */
--- 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
#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 )
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");
}
extern int8_t opt_ibpb_ctxt_switch;
@@ -163,7 +163,7 @@ static always_inline void __spec_ctrl_en
* (ab)use alternative_input() to specify clobbers.
*/
alternative_input("", "DO_OVERWRITE_RSB xu=%=", X86_FEATURE_SC_RSB_IDLE,
- : "rax", "rcx");
+ "i" (0) : "rax", "rcx");
}
/* WARNING! `ret`, `call *`, `jmp *` not safe after this call. */
--- a/xen/arch/x86/include/asm/tsc.h
+++ b/xen/arch/x86/include/asm/tsc.h
@@ -29,8 +29,7 @@ static inline uint64_t rdtsc_ordered(voi
alternative_io_2("lfence; rdtsc",
"mfence; rdtsc", X86_FEATURE_MFENCE_RDTSC,
"rdtscp", X86_FEATURE_RDTSCP,
- ASM_OUTPUT2("=a" (low), "=d" (high), "=c" (aux)),
- /* no inputs */);
+ ASM_OUTPUT2("=a" (low), "=d" (high), "=c" (aux)));
return (high << 32) | low;
}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@suse.com>
>
> --- 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
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
On 09.10.2025 09:45, Jan Beulich wrote: > 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/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. Hmm, no, labels can't be named; they are their own names. I.e. what I think we want here is "jmp %l[" #label "]". Jan
On 2025-09-25 06:48, 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@suse.com>
>
> --- 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" )
>
> #define alternative_2(oldinstr, newinstr1, feature1, newinstr2, feature2) \
> asm_inline volatile ( \
> - ALTERNATIVE_2(oldinstr, newinstr1, feature1, \
> - newinstr2, feature2) \
> - ::: "memory" )
> + ALTERNATIVE_2(oldinstr, newinstr1, [feat1], \
> + newinstr2, [feat2]) \
> + :: [feat1] "i" (feature1), [feat2] "i" (feature2) : "memory" )
>
> /*
> * Alternative inline assembly with input.
> @@ -148,14 +148,14 @@ extern void alternative_instructions(voi
> */
> #define alternative_input(oldinstr, newinstr, feature, input...) \
> asm_inline volatile ( \
> - ALTERNATIVE(oldinstr, newinstr, feature) \
> - :: input )
> + ALTERNATIVE(oldinstr, newinstr, [feat]) \
> + :: [feat] "i" (feature), ## input )
>
> /* Like alternative_input, but with a single output argument */
> #define alternative_io(oldinstr, newinstr, feature, output, input...) \
> asm_inline volatile ( \
> - ALTERNATIVE(oldinstr, newinstr, feature) \
> - : output : input )
> + ALTERNATIVE(oldinstr, newinstr, [feat]) \
> + : output : [feat] "i" (feature), ## input )
>
> /*
> * This is similar to alternative_io. But it has two features and
> @@ -168,9 +168,9 @@ extern void alternative_instructions(voi
> #define alternative_io_2(oldinstr, newinstr1, feature1, newinstr2, \
> feature2, output, input...) \
> asm_inline volatile ( \
> - ALTERNATIVE_2(oldinstr, newinstr1, feature1, \
> - newinstr2, feature2) \
> - : output : input )
> + ALTERNATIVE_2(oldinstr, newinstr1, [feat1], \
> + newinstr2, [feat2]) \
> + : output : [feat1] "i" (feature1), [feat2] "i" (feature2), ## input )
>
> /* Use this macro(s) if you need more than one output parameter. */
> #define ASM_OUTPUT2(a...) a
> --- a/xen/arch/x86/include/asm/cpufeatures.h
> +++ b/xen/arch/x86/include/asm/cpufeatures.h
> @@ -6,9 +6,16 @@
> /* Number of capability words covered by the featureset words. */
> #define FSCAPINTS FEATURESET_NR_ENTRIES
>
> +#if !defined(__ASSEMBLY__) && __GNUC__ >= 15
> +#include <xen/macros.h>
> +#define X86_CHECK_FEAT_NR(x, n) BUILD_BUG_ON_ZERO((x) / 32 >= X86_NR_ ## n)
> +#else
> +#define X86_CHECK_FEAT_NR(x, n) 0
> +#endif
> +
> /* Synthetic words follow the featureset words. */
> #define X86_NR_SYNTH 2
> -#define X86_SYNTH(x) (FSCAPINTS * 32 + (x))
> +#define X86_SYNTH(x) (FSCAPINTS * 32 + (x) + X86_CHECK_FEAT_NR(x, SYNTH))
>
> /* Synthetic features */
> XEN_CPUFEATURE(CONSTANT_TSC, X86_SYNTH( 0)) /* TSC ticks at a constant rate */
> @@ -47,7 +54,8 @@ XEN_CPUFEATURE(XEN_REP_MOVSB, X86_SY
>
> /* Bug words follow the synthetic words. */
> #define X86_NR_BUG 1
> -#define X86_BUG(x) ((FSCAPINTS + X86_NR_SYNTH) * 32 + (x))
> +#define X86_BUG(x) ((FSCAPINTS + X86_NR_SYNTH) * 32 + (x) + \
> + X86_CHECK_FEAT_NR(x, BUG))
>
> #define X86_BUG_FPU_PTRS X86_BUG( 0) /* (F)X{SAVE,RSTOR} doesn't save/restore FOP/FIP/FDP. */
> #define X86_BUG_NULL_SEG X86_BUG( 1) /* NULL-ing a selector preserves the base and limit. */
> --- 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
> #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 )
>
> 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");
"i" (0) is to work around the trailing comma in alternative_input() and
does nothing?
Thanks,
Jason
> }
>
> extern int8_t opt_ibpb_ctxt_switch;
> @@ -163,7 +163,7 @@ static always_inline void __spec_ctrl_en
> * (ab)use alternative_input() to specify clobbers.
> */
> alternative_input("", "DO_OVERWRITE_RSB xu=%=", X86_FEATURE_SC_RSB_IDLE,
> - : "rax", "rcx");
> + "i" (0) : "rax", "rcx");
> }
>
> /* WARNING! `ret`, `call *`, `jmp *` not safe after this call. */
> --- a/xen/arch/x86/include/asm/tsc.h
> +++ b/xen/arch/x86/include/asm/tsc.h
> @@ -29,8 +29,7 @@ static inline uint64_t rdtsc_ordered(voi
> alternative_io_2("lfence; rdtsc",
> "mfence; rdtsc", X86_FEATURE_MFENCE_RDTSC,
> "rdtscp", X86_FEATURE_RDTSCP,
> - ASM_OUTPUT2("=a" (low), "=d" (high), "=c" (aux)),
> - /* no inputs */);
> + ASM_OUTPUT2("=a" (low), "=d" (high), "=c" (aux)));
>
> return (high << 32) | low;
> }
>
>
On 30.09.2025 01:36, Jason Andryuk wrote:
> On 2025-09-25 06:48, Jan Beulich wrote:
>> --- 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");
>
> "i" (0) is to work around the trailing comma in alternative_input() and
> does nothing?
Yes. If more such "uses" appeared, we may want to introduce some kind of
abstraction.
Jan
On 2025-10-07 08:22, Jan Beulich wrote:
> On 30.09.2025 01:36, Jason Andryuk wrote:
>> On 2025-09-25 06:48, Jan Beulich wrote:
>>> --- 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");
>>
>> "i" (0) is to work around the trailing comma in alternative_input() and
>> does nothing?
>
> Yes. If more such "uses" appeared, we may want to introduce some kind of
> abstraction.
Thanks for confirming.
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Though I also wondered if just #define X86_BUG_MAX/X86_SYNTH_MAX
combined with a BUILD_BUG_ON might be good enough. Your approach avoids
the extra define but is more complicated. Anyway, just a thought.
Regards,
Jason
On 07.10.2025 21:38, Jason Andryuk wrote:
> On 2025-10-07 08:22, Jan Beulich wrote:
>> On 30.09.2025 01:36, Jason Andryuk wrote:
>>> On 2025-09-25 06:48, Jan Beulich wrote:
>>>> --- 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");
>>>
>>> "i" (0) is to work around the trailing comma in alternative_input() and
>>> does nothing?
>>
>> Yes. If more such "uses" appeared, we may want to introduce some kind of
>> abstraction.
>
> Thanks for confirming.
>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Thanks.
> Though I also wondered if just #define X86_BUG_MAX/X86_SYNTH_MAX
> combined with a BUILD_BUG_ON might be good enough. Your approach avoids
> the extra define but is more complicated. Anyway, just a thought.
How would that end up simplifying things? IOW what would the BUILD_BUG_ON()
look like that you're thinking about? After all X86_{SYNTH,BUG}_MAX aren't
meaningfully different from X86_NR_{SYNTH,BUG}.
Jan
On 2025-10-08 01:56, Jan Beulich wrote:
> On 07.10.2025 21:38, Jason Andryuk wrote:
>> On 2025-10-07 08:22, Jan Beulich wrote:
>>> On 30.09.2025 01:36, Jason Andryuk wrote:
>>>> On 2025-09-25 06:48, Jan Beulich wrote:
>>>>> --- 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");
>>>>
>>>> "i" (0) is to work around the trailing comma in alternative_input() and
>>>> does nothing?
>>>
>>> Yes. If more such "uses" appeared, we may want to introduce some kind of
>>> abstraction.
>>
>> Thanks for confirming.
>>
>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>
> Thanks.
>
>> Though I also wondered if just #define X86_BUG_MAX/X86_SYNTH_MAX
>> combined with a BUILD_BUG_ON might be good enough. Your approach avoids
>> the extra define but is more complicated. Anyway, just a thought.
>
> How would that end up simplifying things? IOW what would the BUILD_BUG_ON()
> look like that you're thinking about? After all X86_{SYNTH,BUG}_MAX aren't
> meaningfully different from X86_NR_{SYNTH,BUG}.
Originally, I was thinking something like
XEN_CPUFEATURE(PDX_COMPRESSION, X86_SYNTH(31)) /* PDX compression */
+#define X86_SYNTH_MAX 31 /* Bump when adding flags */
and:
BUILD_BUG_ON( ((X86_SYNTH_MAX / 32) + 1) > X86_NR_SYNTH )
Not automated, but adding a new flag should make it obvious
X86_SYNTH_MAX should increase.
But as you point out the redundancy of X86_{SYNTH,BUG}_MAX and
X86_NR_{SYNTH,BUG}. But we could re-arrange to make X86_NR_{SYNTH,BUG}
calculated from X86_{SYNTH,BUG}_MAX like below.
Again, it's not automated, but it should make it harder to miss
increasing the value.
Regards,
Jason
diff --git i/xen/arch/x86/include/asm/cpufeatures.h
w/xen/arch/x86/include/asm/cpufeatures.h
index 0a98676c16..724eb1599f 100644
--- i/xen/arch/x86/include/asm/cpufeatures.h
+++ w/xen/arch/x86/include/asm/cpufeatures.h
@@ -7,7 +7,6 @@
#define FSCAPINTS FEATURESET_NR_ENTRIES
/* Synthetic words follow the featureset words. */
-#define X86_NR_SYNTH 1
#define X86_SYNTH(x) (FSCAPINTS * 32 + (x))
/* Synthetic features */
@@ -43,9 +42,10 @@ XEN_CPUFEATURE(IBPB_ENTRY_PV, X86_SYNTH(28)) /*
MSR_PRED_CMD used by Xen for
XEN_CPUFEATURE(IBPB_ENTRY_HVM, X86_SYNTH(29)) /* MSR_PRED_CMD used
by Xen for HVM */
XEN_CPUFEATURE(USE_VMCALL, X86_SYNTH(30)) /* Use VMCALL instead
of VMMCALL */
XEN_CPUFEATURE(PDX_COMPRESSION, X86_SYNTH(31)) /* PDX compression */
+#define X86_SYNTH_MAX 31 /* Bump when adding new flags. */
+#define X86_NR_SYNTH ((X86_SYNTH_MAX / 32) + 1)
/* Bug words follow the synthetic words. */
-#define X86_NR_BUG 1
#define X86_BUG(x) ((FSCAPINTS + X86_NR_SYNTH) * 32 + (x))
#define X86_BUG_FPU_PTRS X86_BUG( 0) /* (F)X{SAVE,RSTOR}
doesn't save/restore FOP/FIP/FDP. */
@@ -62,6 +62,8 @@ XEN_CPUFEATURE(PDX_COMPRESSION, X86_SYNTH(31)) /*
PDX compression */
#define X86_SPEC_BHB_TSX X86_BUG(19) /* Use clear_bhb_tsx for
BHI mitigation. */
#define X86_SPEC_BHB_LOOPS X86_BUG(20) /* Use clear_bhb_loops
for BHI mitigation.*/
#define X86_SPEC_BHB_LOOPS_LONG X86_BUG(21) /* Upgrade
clear_bhb_loops to the "long" sequence. */
+#define X86_BUX_MAX 21 /* Bump when adding new flags. */
+#define X86_NR_BUG ((X86_BUG_MAX / 32) + 1)
/* Total number of capability words, inc synth and bug words. */
#define NCAPINTS (FSCAPINTS + X86_NR_SYNTH + X86_NR_BUG) /* N 32-bit
words worth of info */
On 11.10.2025 02:30, Jason Andryuk wrote:
> On 2025-10-08 01:56, Jan Beulich wrote:
>> On 07.10.2025 21:38, Jason Andryuk wrote:
>>> On 2025-10-07 08:22, Jan Beulich wrote:
>>>> On 30.09.2025 01:36, Jason Andryuk wrote:
>>>>> On 2025-09-25 06:48, Jan Beulich wrote:
>>>>>> --- 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");
>>>>>
>>>>> "i" (0) is to work around the trailing comma in alternative_input() and
>>>>> does nothing?
>>>>
>>>> Yes. If more such "uses" appeared, we may want to introduce some kind of
>>>> abstraction.
>>>
>>> Thanks for confirming.
>>>
>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>>
>> Thanks.
>>
>>> Though I also wondered if just #define X86_BUG_MAX/X86_SYNTH_MAX
>>> combined with a BUILD_BUG_ON might be good enough. Your approach avoids
>>> the extra define but is more complicated. Anyway, just a thought.
>>
>> How would that end up simplifying things? IOW what would the BUILD_BUG_ON()
>> look like that you're thinking about? After all X86_{SYNTH,BUG}_MAX aren't
>> meaningfully different from X86_NR_{SYNTH,BUG}.
>
> Originally, I was thinking something like
> XEN_CPUFEATURE(PDX_COMPRESSION, X86_SYNTH(31)) /* PDX compression */
> +#define X86_SYNTH_MAX 31 /* Bump when adding flags */
I don't really like this. Especially as presented is creates an ambiguity:
Would one need to increase the value upon any flag addition, or only upon
adding a new flag with a value divisible by 32. (From the patch fragment
you appended it's clear the latter is meant, but that's not clear here,
and when one learns to routinely ignore the comment, there's a risk of also
ignoring it when it shouldn't be ignored. Whereas if the bump was required
every time a new flag was added, I would dislike the unnecessary churn. In
the end - yes, there's a reason why I did things the way done, even if
there are other aspects to it which are (for the patch itself, but imo not
once it would have gone in) not overly nice.
Jan
© 2016 - 2026 Red Hat, Inc.