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.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.
> 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 <jbeul...@suse.com>
>> 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
