Re: [PATCH 19/22] x86/boot: Use RSTORSSP to establish SSP

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2025 22:09, Andrew Cooper wrote:
> On 14/08/2025 4:11 pm, Jan Beulich wrote:
>> On 08.08.2025 22:23, Andrew Cooper wrote:
>>> Under FRED, SETSSBSY is unavailable, and we want to be setting up FRED prior
>>> to setting up shadow stacks.  As we still need Supervisor Tokens in IDT 
>>> mode,
>>> we need mode-specific logic to establish SSP.
>>>
>>> In FRED mode, write a Restore Token, RSTORSSP it, and discard the resulting
>>> Previous-SSP token.
>>>
>>> No change outside of FRED mode.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Why is it that in patch 17 you could use identical code, but here you can't?
> 
> This caught me out at first too.
> 
> For S3, we're going from "no shadow stack" to "back to where we were on
> an active shadow stack".  All we need to do is get saved_ssp back into
> the SSP register.
> 
> Here, we're going from "no shadow stack" to "on a good, empty, shadow
> stack".  For FRED we only need to load a value into SSP, but in IDT mode
> we must also arrange to create a busy Supervisor Token on the base of
> the stack.
> 
> We could in principle conditionally write a busy supervisor token, then
> unconditionally RSTORSSP, but that's even more complicated to follow IMO.

Why would the write need to be conditional? Can't we write what effectively
is already there? Or is it more a safety measure to avoid the write when
it's supposed to be unnecessary, to avoid papering over bugs?

>>> @@ -912,10 +913,30 @@ static void __init noreturn reinit_bsp_stack(void)
>>>  
>>>      if ( cpu_has_xen_shstk )
>>>      {
>>> -        wrmsrl(MSR_PL0_SSP,
>>> -               (unsigned long)stack + (PRIMARY_SHSTK_SLOT + 1) * PAGE_SIZE 
>>> - 8);
>> Does this removal perhaps belong elsewhere, especially with "No change
>> outside of FRED mode" in the description?
> 
> This is the "Updating reinit_bsp_stack() is deferred until later." note
> in the previous patch.
> 
> This hunk was illegible without the split, although I have to admit that
> I can't quite remember why now.

Hmm, if it is to stay like this, would you mind adding a respective remark
also in the description here?

Jan