Re: [PATCH v2 68/70] x86/setup: Rework MSR_S_CET handling for CET-IBT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.02.2022 21:58, Andrew Cooper wrote:
> On 15/02/2022 16:46, Jan Beulich wrote:
>> On 14.02.2022 13:51, Andrew Cooper wrote:
>>> CET-SS and CET-IBT can be independently controlled, so the configuration of
>>> MSR_S_CET can't be constant any more.
>>>
>>> Introduce xen_msr_s_cet_value(), mostly because I don't fancy
>>> writing/maintaining that logic in assembly.  Use this in the 3 paths which
>>> alter MSR_S_CET when both features are potentially active.
>>>
>>> To active CET-IBT, we only need CR4.CET and MSR_S_CET.ENDBR_EN.  This is
>>> common with the CET-SS setup, so reorder the operations to set up CR4 and
>>> MSR_S_CET for any nonzero result from xen_msr_s_cet_value(), and set up
>>> MSR_PL0_SSP and SSP if SHSTK_EN was also set.
>>>
>>> Adjust the crash path to disable CET-IBT too.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Thanks,
> 
>> albeit with a nit and a remark:
>>
>>> --- a/xen/arch/x86/acpi/wakeup_prot.S
>>> +++ b/xen/arch/x86/acpi/wakeup_prot.S
>>> @@ -63,7 +63,26 @@ ENTRY(s3_resume)
>>>          pushq   %rax
>>>          lretq
>>>  1:
>>> -#ifdef CONFIG_XEN_SHSTK
>>> +#if defined(CONFIG_XEN_SHSTK) || defined(CONFIG_XEN_IBT)
>>> +        call    xen_msr_s_cet_value
>>> +        test    %eax, %eax
>>> +        jz      .L_cet_done
>>> +
>>> +        /* Set up MSR_S_CET. */
>>> +        mov     $MSR_S_CET, %ecx
>>> +        xor     %edx, %edx
>>> +        wrmsr
>>> +
>>> +        /* Enable CR4.CET. */
>>> +        mov     $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx
>>> +        mov     %rcx, %cr4
>>> +
>>> +        /* WARNING! call/ret now fatal (iff SHSTK) until SETSSBSY loads 
>>> SSP */
>>> +
>>> +#if defined(CONFIG_XEN_SHSTK)
>> Just #ifdef, as it was before?
> 
> I can if you insist, but that's breaking consistency with the other
> ifdefary.

I guess consistent of not depends on the way you look at it. I
generally think simple conditionals should just use #ifdef. As
soon as there's an #elif or a more complex condition, #if
defined() is of course more consistent. But one #ifdef nested
inside another #if imo isn't a reason to use #if in both places.

Nevertheless, ftaod - I'm not going to insist, as I can see this
being a matter of personal preference.

>>> @@ -90,10 +101,6 @@ ENTRY(s3_resume)
>>>          mov     %edi, %eax
>>>          wrmsr
>>>  
>>> -        /* Enable CET.  MSR_INTERRUPT_SSP_TABLE is set up later in 
>>> load_system_tables(). */
>>> -        mov     $XEN_MINIMAL_CR4 | X86_CR4_CET, %ebx
>>> -        mov     %rbx, %cr4
>> The latter part of this comment could do with retaining.
> 
> So I tried that in v1, and concluded not for v2.
> 
> There is nowhere appropriate for it to live, anywhere in this block. 
> And it is an artefact of me bootstrapping SHSTK to start with.
> 
> The truth is that nothing about MSR_ISST_TABLE matters until
> load_system_table sets up both this and the TSS IST fields together. 
> IST exceptions are already fatal at this point for non-SHSTK reasons.

Well, okay. To me, not being as familiar with this code as you
are, the comments was quite helpful ...

Jan