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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2025 22:55, Andrew Cooper wrote:
> 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.

Not really: __read_mostly is to keep stuff rarely written apart from stuff
more frequently written (cache locality, yes). There's not going to be any
frequently written data next to a __ro_after_init item; it's all r/o post-
boot. And I don't think we care much during boot.

>>> @@ -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.

I was wondering whether there was a connection there, but ...

> IIRC, one version of Clang complained.

... that's not good. Without the early-clobber the asm() isn't quite
correct imo. If the same value appeared as another input, the compiler
may validly tie both together, assuming the register stays intact until
the very last insn (and hence even that last insn could still use the
register as an input). IOW if there's a Clang issue here, I think it
may need working around explicitly.

Jan