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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

Jan