Re: [PATCH v2 1/2] x86/boot: Clear XD_DISABLE from the early boot path

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/06/2023 4:31 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 09bebf8635..ce62eae6f3 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -142,8 +142,8 @@ efi_platform:
>  
>          .section .init.text, "ax", @progbits
>  
> -bad_cpu:
> -        add     $sym_offs(.Lbad_cpu_msg),%esi   # Error message
> +.Lbad_cpu:
> +        add     $sym_offs(.Lbad_cpu_msg),%esi
>          jmp     .Lget_vtb
>  not_multiboot:
>          add     $sym_offs(.Lbad_ldr_msg),%esi   # Error message
> @@ -647,15 +647,59 @@ 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. */
> +        /* 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     .Lhas_nx_bit

This part of the diff is far more legible when rebased over the 0.5/2
patch I've just emailed out.  I've got the rebase locally if you want it.

Although I'd suggest that this label name be .Lgot_nx.

> +
> +        /*
> +         * NX appears to be unsupported, but it might be hidden.
> +         *
> +         * Intel CPUs (may) implement MSR_IA32_MISC_ENABLE. Among other
> +         * things this MSR has a bit that artificially hides NX support in
> +         * CPUID. Xen _really_ wants that feature enabled if present, so we
> +         * have to determine if (a) the MSR exists and if so (b) clear the
> +         * bit.
> +         *
> +         * For native boots this is perfectly fine because the MSR was
> +         * introduced before Netburst, which was the first family to
> +         * provide 64bit support. So we're safe simply accessing it as long
> +         * as long mode support has already been checked.
> +         *
> +         * For the virtualized case the MSR might not be emulated though,
> +         * so we make sure to do an initial check for NX in order to bypass
> +         * this MSR read

I'd suggest reordering this a bit, and swapping some what for why.  How
about:

---
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
> +        cmpl    $X86_VENDOR_INTEL_EBX,%ebx

Where possible, spaces after commas and without size suffixes (the l on
cmpl here).  While not relevant here

> +        jnz     .Lno_nx_bit

The "_bit" is still redundant.  Simply .Lno_nx will do.

> +        cmpl    $X86_VENDOR_INTEL_EDX,%edx
> +        jnz     .Lno_nx_bit
> +        cmpl    $X86_VENDOR_INTEL_ECX,%ecx
> +        jnz     .Lno_nx_bit
> +
> +        /* Clear the XD_DISABLE bit */
> +        movl    $MSR_IA32_MISC_ENABLE, %ecx
> +        rdmsr
> +        btrl    $2, %edx

It's unfortunate that we don't have an asm-safe ilog2().  Oh well.

>          jnc     1f
> -        orb     $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer)
> -1:
> +        wrmsr
> +        orb     $MSR_IA32_MISC_ENABLE_XD_DISABLE >> 32, 4 + 
> sym_esi(trampoline_misc_enable_off)
>  
> -        /* Check for availability of long mode. */
> -        bt      $cpufeat_bit(X86_FEATURE_LM),%edx
> -        jnc     bad_cpu
> +1:      /* Check again for NX */

Failing to clear XD_DISABLE wants to go to .Lno_nx, not to re-scan CPUID
having done nothing to the MSR.

> +        mov     $0x80000001,%eax
> +        cpuid
> +        bt      $cpufeat_bit(X86_FEATURE_NX), %edx
> +        jnc     .Lno_nx_bit
> +
> +.Lhas_nx_bit:
> +        /* Adjust EFER is NX is present */

"Adjust EFER, given that NX is present".

> diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
> index 168cd58f36..46b0cd8dbb 100644
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -305,23 +305,23 @@ static void cf_check early_init_intel(struct 
> cpuinfo_x86 *c)
>               c->x86_cache_alignment = 128;
>  
>       /* Unmask CPUID levels and NX if masked: */
> -     rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> -
> -     disable = misc_enable & (MSR_IA32_MISC_ENABLE_LIMIT_CPUID |
> -                              MSR_IA32_MISC_ENABLE_XD_DISABLE);
> -     if (disable) {
> -             wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable & ~disable);
> -             bootsym(trampoline_misc_enable_off) |= disable;
> -             bootsym(trampoline_efer) |= EFER_NXE;
> -     }
> +     if (rdmsr_safe(MSR_IA32_MISC_ENABLE, misc_enable) == 0) {

There's no need to change rdmsrl() to rdmsr_safe(), and not doing so
will shrink this diff a lot.

The only thing you need to alter the re-enable NX printk(), which
probably wants to move ahead of the "if (disable)" and source itself
from bootsym(trampoline_misc_enable_off) instead.

~Andrew