Re: [PATCH v4 1/4] x86: record SSP at non-guest entry points

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.10.2026 19:12, Andrew Cooper wrote:
> On 27/07/2026 11:26 am, Jan Beulich wrote:
>> --- a/xen/arch/x86/include/asm/asm_defns.h
>> +++ b/xen/arch/x86/include/asm/asm_defns.h
>> @@ -220,9 +220,11 @@ static always_inline void stac(void)
>>  
>>  #ifdef __ASSEMBLER__
>>  /*
>> - * Push and clear GPRs
>> + * Push and clear GPRs.
>> + *
>> + * Use sites may override ssp to 0. It should never be overridden to 1.
>>   */
>> -.macro PUSH_AND_CLEAR_GPRS
>> +.macro PUSH_AND_CLEAR_GPRS ssp=IS_ENABLED(CONFIG_XEN_SHSTK)
>>          push  %rdi
>>          xor   %edi, %edi
>>          push  %rsi
>> @@ -233,6 +235,9 @@ static always_inline void stac(void)
>>          xor   %ecx, %ecx
>>          push  %rax
>>          xor   %eax, %eax
>> + .if \ssp
>> +        rdsspq %rcx
>> + .endif
>>          push  %r8
>>          xor   %r8d, %r8d
>>          push  %r9
>> @@ -259,12 +264,18 @@ static always_inline void stac(void)
>>          xor   %r14d, %r14d
>>          push  %r15
>>          xor   %r15d, %r15d
>> +#ifdef CONFIG_XEN_SHSTK
>> +        push  %rcx
>> +#endif
>>  .endm
> 
> I don't like the ssp= parameter here.  It's invasive at may callsites,
> doesn't 0 the value in all Ring3 frames anyway, and the asymmetry makes
> the construct fragile.  Furthermore, the manual register scheduling
> really isn't necessary on CPUs where it's not a NOP.
> 
> I suggest having this:
> 
> +#ifdef CONFIG_XEN_SHSTK
> +        rdsspq %rcx
> +        push  %rcx
> +#endif
> 
> always filling in the value.  rdsspq is pretty close to free (frankly, I
> would be surprised if it wasn't subject to mov elimination in the
> pipeline, given how tightly coupled SSP needs to be to RSP for CALL/RET
> uops to work).

I can make that change, but I didn't code it that way from the start as
there continues to be no evidence of what you say (no matter how
plausible it reads): None of the latency / throughput documents I can
find lists RDSSP.

>> --- a/xen/arch/x86/include/asm/cpu-user-regs.h
>> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
>> @@ -11,6 +11,15 @@
>>   */
>>  struct cpu_user_regs
>>  {
>> +#ifdef CONFIG_XEN_SHSTK
>> +    /*
>> +     * This points _at_ the corresponding shadow stack frame; it is _not_ 
>> the
>> +     * outer context's SSP.  That, if the outer context has CET-SS enabled,
>> +     * is stored in the top slot of the pointed to shadow stack.
>> +     */
>> +    uint64_t entry_ssp;
>> +#endif
> 
> This needs to say "only valid for interrupt Ring0" or so.  Even as
> written using the ssp= parameter, there will be a non-zero value here on
> some Ring3 entries.

The value being non-zero doesn't mean it's invalid. It may point to the
very top of the shadow stack, which is still a valid value. If you want
this made explicit, I can add "only meaningful when having interrupted
Ring0" or some such. Please clarify.

Jan