Re: [PATCH 63/65] x86/setup: Rework MSR_S_CET handling for CET-IBT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2021 13:34, Andrew Cooper wrote:
> --- a/xen/arch/x86/acpi/wakeup_prot.S
> +++ b/xen/arch/x86/acpi/wakeup_prot.S
> @@ -63,7 +63,24 @@ 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
> +        je      .L_cet_done

Nit: I consider it generally misleading to use JE / JNE (and a few
other Jcc) with other than CMP-like insns. Only those handle actual
"relations", whereas e.g. TEST only produces particular flag states,
so would more consistently be followed by JZ / JNZ in cases like
this one. But since this is very much a matter of taste, I'm not
going to insist on a change here.

> +        /* 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

Is it valid / safe to enable CR4.CET (with CET_SHSTK_EN already
active) before ...

> +#if defined(CONFIG_XEN_SHSTK)
> +        test    $CET_SHSTK_EN, %eax

(Intermediate remark: Using %al would seem to suffice and be a
shorter encoding.)

> +        je      .L_cet_done
> +
>          /*
>           * Restoring SSP is a little complicated, because we are intercepting
>           * an in-use shadow stack.  Write a temporary token under the stack,
> @@ -71,14 +88,6 @@ ENTRY(s3_resume)
>           * reset MSR_PL0_SSP to its usual value and pop the temporary token.
>           */
>          mov     saved_ssp(%rip), %rdi
> -        cmpq    $1, %rdi
> -        je      .L_shstk_done
> -
> -        /* Set up MSR_S_CET. */
> -        mov     $MSR_S_CET, %ecx
> -        xor     %edx, %edx
> -        mov     $CET_SHSTK_EN | CET_WRSS_EN, %eax
> -        wrmsr
>  
>          /* Construct the temporary supervisor token under SSP. */
>          sub     $8, %rdi
> @@ -90,12 +99,9 @@ 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 writing of MSR_PL0_SSP in context here? ISTR some ordering
issues back at the time when you introduced CET-SS, so I thought I'd
better ask to be sure.

Jan