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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/08/2025 10:10 am, Jan Beulich wrote:
> 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.

It's not about boot, but hot variables do need grouping.  opt_fred is
read on fastpaths, whereas trampoline_phys is not.

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

Given that I need an alternative anyway, this becomes much easier, and
shrinks to this single hunk:

diff --git a/xen/arch/x86/include/asm/current.h 
b/xen/arch/x86/include/asm/current.h
index c1eb27b1c4c2..35cc61fa88e7 100644
--- 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[skstk_base], %[val];",                 \
+                "mov $%c[skstk_base] + 8, %[val];",             \
+                X86_FEATURE_XEN_FRED)                           \
     "and $%c[stack_mask], %[ssp];"                              \
     "sub %[ssp], %[val];"                                       \
     "shr $3, %[val];"                                           \



~Andrew