Re: [PATCH v3 2/3] x86/boot: Clear XD_DISABLE from the early boot path

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/06/2023 1:17 pm, Alejandro Vallejo wrote:
> Intel CPUs have a bit in MSR_IA32_MISC_ENABLE that may prevent the NX bit
> from being advertised. Clear it unconditionally if we can't find the NX
> feature right away on boot.
>
> The conditions for the MSR being read on early boot are (in this order):
>
> * Long Mode is supported
> * NX isn't advertised
> * The vendor is Intel
>
> The order of checks has been chosen carefully so a virtualized Xen on a
> hypervisor that doesn't emulate that MSR (but supports NX) doesn't triple
> fault trying to access the non-existing MSR.
>
> With that done, we can remove the XD_DISABLE checks in the intel-specific
> init path (as they are already done in early assembly). Keep a printk to
> highlight the fact that NX was forcefully enabled.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, with two minor
fixes.

> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 9fbd602ea5..0e02c28f37 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -652,16 +652,53 @@ trampoline_setup:
>          cpuid
>  1:      mov     %edx, CPUINFO_FEATURE_OFFSET(X86_FEATURE_LM) + 
> sym_esi(boot_cpu_data)
>  
> -        /* Check for NX. Adjust EFER setting if available. */
> -        bt      $cpufeat_bit(X86_FEATURE_NX), %edx
> -        jnc     1f
> -        orb     $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer)
> -1:
> -
>          /* Check for availability of long mode. */
>          bt      $cpufeat_bit(X86_FEATURE_LM),%edx
>          jnc     .Lbad_cpu
>  
> +        /* Check for NX */
> +        bt      $cpufeat_bit(X86_FEATURE_NX), %edx
> +        jc     .Lgot_nx
> +
> +        /*
> +         * NX appears to be unsupported, but it might be hidden.
> +         *
> +         * The feature is part of the AMD64 spec, but the very first Intel
> +         * 64bit CPUs lacked the feature, and thereafter there was a
> +         * firmware knob to disable the feature. Undo the disable if
> +         * possible.
> +         *
> +         * All 64bit Intel CPUs support this MSR. If virtualised, expect
> +         * the hypervisor to either emulate the MSR or give us NX.
> +         */
> +        xor     %eax, %eax
> +        cpuid
> +        cmp     $X86_VENDOR_INTEL_EBX, %ebx
> +        jnz     .Lno_nx
> +        cmp     $X86_VENDOR_INTEL_EDX, %edx
> +        jnz     .Lno_nx
> +        cmp     $X86_VENDOR_INTEL_ECX, %ecx
> +        jnz     .Lno_nx
> +
> +        /* Clear the XD_DISABLE bit */
> +        mov    $MSR_IA32_MISC_ENABLE, %ecx

Parameter indention here.

> diff --git a/xen/arch/x86/include/asm/x86-vendors.h 
> b/xen/arch/x86/include/asm/x86-vendors.h
> index 0a37024cbd..9191da26d7 100644
> --- a/xen/arch/x86/include/asm/x86-vendors.h
> +++ b/xen/arch/x86/include/asm/x86-vendors.h
> @@ -12,9 +12,9 @@
>  #define X86_VENDOR_UNKNOWN 0
>  
>  #define X86_VENDOR_INTEL (1 << 0)
> -#define X86_VENDOR_INTEL_EBX 0x756e6547U /* "GenuineIntel" */
> -#define X86_VENDOR_INTEL_ECX 0x6c65746eU
> -#define X86_VENDOR_INTEL_EDX 0x49656e69U
> +#define X86_VENDOR_INTEL_EBX _AC(0x756e6547, U) /* "GenuineIntel" */
> +#define X86_VENDOR_INTEL_ECX _AC(0x6c65746e, U)
> +#define X86_VENDOR_INTEL_EDX _AC(0x49656e69, U)
>  
>  #define X86_VENDOR_AMD (1 << 1)
>  #define X86_VENDOR_AMD_EBX 0x68747541U /* "AuthenticAMD" */

And all vendors should get equivalent _AC() conversions.

Can fix both on commit.

~Andrew