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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/08/2025 4:35 pm, Jan Beulich wrote:
> On 08.08.2025 22:23, 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.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> RFC.  I don't like this, but it does work.
>>
>> This emits opt_fred logic outside of CONFIG_XEN_SHSTK.
> opt_fred and XEN_SHSTK are orthogonal, so that's fine anyway. What I guess
> you may mean is that you now have a shstk-related calculation outside of
> a respective #ifdef.

I really mean "outside of the path where shadow stacks are known to be
active", i.e. inside the middle of SHADOW_STACK_WORK

>  Given the simplicity of the calculation, ...
>
>>  But frankly, the
>> construct is already too unweildly, and all options I can think of make it
>> moreso.
> ... I agree having it like this is okay.

Yes, but it is a read of a global even when it's not used.

And as a tangent, we probably want __ro_after_init_read_mostly too.  The
read mostly is about cache locality, and is applicable even to the
__ro_after_init section.

>
>> @@ -154,7 +155,6 @@ 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];"                              \
>>      "and $%c[stack_mask], %[ssp];"                              \
>>      "sub %[ssp], %[val];"                                       \
>>      "shr $3, %[val];"                                           \
> With the latter two insns here, ...
>
>> @@ -177,6 +177,8 @@ unsigned long get_stack_dump_bottom (unsigned long sp);
>>  
>>  #define switch_stack_and_jump(fn, instr, constr)                        \
>>      ({                                                                  \
>> +        unsigned int token_offset =                                     \
>> +            (PRIMARY_SHSTK_SLOT + 1) * PAGE_SIZE - (opt_fred ? 0 : 8);  \
>>          unsigned int tmp;                                               \
>>          BUILD_BUG_ON(!ssaj_has_attr_noreturn(fn));                      \
>>          __asm__ __volatile__ (                                          \
>> @@ -184,12 +186,11 @@ unsigned long get_stack_dump_bottom (unsigned long sp);
>>              "mov %[stk], %%rsp;"                                        \
>>              CHECK_FOR_LIVEPATCH_WORK                                    \
>>              instr "[fun]"                                               \
>> -            : [val] "=&r" (tmp),                                        \
>> +            : [val] "=r" (tmp),                                         \
> ... I don't think you can legitimately drop the & from here? With it
> retained:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

You chopped the bit which has an explicit input for "[val]", making the
earlyclobber incorrect.

IIRC, one version of Clang complained.

~Andrew