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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

>
>> @@ -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.

~Andrew