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.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>>
>> 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 <jbeul...@suse.com>
You chopped the bit which has an explicit input for "[val]", making the
earlyclobber incorrect.
IIRC, one version of Clang complained.
~Andrew
