[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 20/22] x86/traps: Alter switch_stack_and_jump() for FRED mode
On 22.08.2025 00:56, Andrew Cooper wrote: > 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: >>>>> @@ -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];" \ Oh, okay. But then please again without unnecessary use of $%c constructs, when just % will do. Tangential: Now that I look at this again, what's the 1st 'k' standing for in skstk_base? Was that maybe meant to be 'h'? Jan Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |