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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.01.2026 19:12, Andrew Cooper wrote:
> On 07/01/2026 2:11 pm, 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>
>> ---
>> v2: Fix shim build. Use named label operand in pdx.h.
> 
> I'm pretty sure that will be rejected by Eclair as a Rule R20.12
> violation (using a parameter as a regular value, and stringised), and is
> a blocking rule.

I don't think so - see the difference between the non-compliant and the
compliant examples in the spec. The label passed here isn't subject to
further macro expansion.

> But more generally...  I see why you want a guard rail here, I can't
> help feeling that the cure is worse than the poison.

That's a fair position to take, albeit I don't really agree.

> Updating every alternative is very invasive, and this in particular

Well, luckily it's far from all, but mainly those which use the upper-
case macros directly. Plus the two "(ab)uses" in asm/spec_ctrl.h. The
change in asm/tsc.h is actually in our favor, I would say.

>> --- 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");
>>  }
> 
> without even an explanation of why, is an immediate red flag.

Honestly, to me the "(ab)use" in the comment is enough of an explanation.
Plus, really, the end result now looks more "normal" than before (in no
longer having comma and colon next to each other).

Would adding /* dummy */ after the seemingly stray input satisfy your
request for "an explanation"? Else what exactly would you expect?

> Could we not split X86_SYNTH()/BUG() to take a leaf/bit pair, similar to
> how we express regular features as a*32+b?
> 
> That would at least make it more obvious than currently when a new leaf
> is needed, and contained it to a single header.

I'm pretty sure we could, but such a split would be largely artificial.
Hence why I discarded that option very early, the more that - as you say -
it still would only serve as a hint, without enforcing anything. In
particular I could easily see me using the next "major" index, but still
forgetting that X86_NR_{SYNTH,BUG} would also need bumping. (What might
help a little is if the two really moved to the end of their blocks, so
they would be more likely to be spotted when adding something to the end.

Bottom line: I'd prefer if we would stick to actually doing the checking
(or yet better derive X86_NR_{SYNTH,BUG} from the uses of
X86_{SYNTH,BUG}() [1]). I'm not particularly happy with the way the checking
is done right now, so I'm all ears towards improvement suggestions.

Jan

[1] Some initial idea: Have

XEN_CPUFEATURE(SYNTH_1ST_UNUSED, X86_SYNTH(...)) /* Must stay last. */

#define X86_NR_SYNTH ((X86_FEATURE_SYNTH_1ST_UNUSED - 1) / 32 - FSCAPINTS)