Re: [PATCH v3 07/22] x86/traps: Alter switch_stack_and_jump() for FRED mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.02.2026 17:21, Andrew Cooper wrote:
> On 10/02/2026 11:55 am, Jan Beulich wrote:
>> On 10.02.2026 12:15, Andrew Cooper wrote:
>>> On 07/10/2025 4:58 pm, Jan Beulich wrote:
>>>> On 04.10.2025 00:53, Andrew Cooper wrote:
>>>>> FRED and IDT differ by a Supervisor Token on the base of the shstk.  This
>>>>> means that switch_stack_and_jump() needs to discard one extra word when 
>>>>> FRED
>>>>> is active.
>>>>>
>>>>> Fix a typo in the parameter name, which should be shstk_base.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> ---
>>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>>
>>>>> Leave as $%c.  Otherwise it doesn't assemble correctly presented with 
>>>>> $$24568
>>>>> to parse as an instruction immediate.
>>>> I don't follow. Where would the 2nd $ come from if you write ...
>>>>
>>>>> --- a/xen/arch/x86/include/asm/current.h
>>>>> +++ b/xen/arch/x86/include/asm/current.h
>>>>> @@ -154,7 +154,9 @@ unsigned long get_stack_dump_bottom (unsigned long 
>>>>> sp);
>>>>>      "rdsspd %[ssp];"                                            \
>>>>>      "cmp $1, %[ssp];"                                           \
>>>>>      "je .L_shstk_done.%=;" /* CET not active?  Skip. */         \
>>>>> -    "mov $%c[skstk_base], %[val];"                              \
>>>>> +    ALTERNATIVE("mov $%c[shstk_base], %[val];",                 \
>>>>> +                "mov $%c[shstk_base] + 8, %[val];",             \
>>>>> +                X86_FEATURE_XEN_FRED)                           \
>>>>     ALTERNATIVE("mov %[shstk_base], %[val];",                   \
>>>>                 "mov %[shstk_base] + 8, %[val];",               \
>>>>                 X86_FEATURE_XEN_FRED)                           \
>>> I find this feedback completely uncharacteristic.  You always goes out
>>> of your way to hide % inside macros to prohibit non-register operands.
>>>
>>> This is exactly the same, except to force an immediate operand, so the
>>> length of the two instructions is the same.
>> Thinking about it more, are you perhaps referring to assembler macros?
>> There indeed I prefer to have the % inside the macros; the same may go
>> for $ there, but I don't think we had the need so far. For inline
>> assembly the situation is different: The compiler emits the % (and also
>> the $), unless special modifiers are used. It wouldn't even occur to me
>> to ask that we use %%%V[val] for a register operand. That really is the
>> register equivalent of the $%c[val] that you use above.
> 
> We can't use %V anyway because it's not available in our toolchain baseline.

Sure, I used this to get my point across. Technically we could macro-ize
this to use plain % on older gcc and %%%V on ones recognizing the
modifier.

> But, bottom line.  How insistent are you going to be here, because this
> is the only thing holding up committing 6 patches.

The construct you use is technically correct, so it feels odd for me to
block this (hence the R-b I gave). Otoh I have to return the question: How
insistent are you on using a more complicated construct when a simpler one
will do (and will be correct as long as we get the constraint right)?

Jan

PS: As to the constraint, I wonder if it wouldn't better be "e". The
value used is small enough right now, but if whatever change led to it
becoming huge (e.g. an address instead of a size, as "shstk_base" may be
taken to imply), this would still assemble (to what - oddly - AT&T syntax
calls MOVABS). Yet we may want to be aware of such an encoding change.
Having said this, while things ought to assemble okay in that case, I'm
not as certain as to this also compiling successfully, due to our request
for PIC code. Yet that then ought to be the case regardless of "i" or "e",
and might then only work correctly with the new %cc modifier (which
obviously we can't use unconditionally anyway).