Re: [PATCH] x86: Add Kconfig option to require NX bit support

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 01/06/2023 6:43 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 09bebf8635..8414266281 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -647,11 +653,18 @@ 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 NX:
> +         *   - If Xen was compiled requiring it simply assert it's
> +         *     supported. The trampoline already has the right constant.
> +         *   - Otherwise, update the trampoline EFER mask accordingly.
> +         */
>          bt      $cpufeat_bit(X86_FEATURE_NX), %edx
> -        jnc     1f
> +        jnc     no_nx_bit
> +#if !IS_ENABLED(CONFIG_REQUIRE_NX_BIT)
>          orb     $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer)
> -1:
> +no_nx_bit:
> +#endif

It occurs to me...  This will prevent Xen booting in firmware
configurations where XD-Disable is active, despite Xen having
unconditional logic to turn XD off later in boot.

Linux deals with this in verify_cpu() (early asm) along with a FMS check
protecting the access to MSR_MISC_ENABLE, rather than using rdmsr_safe()
and catching the #GP.


In terms of which CPUs are a problem, we almost got very lucky.  NX is
part of the AMD64 spec, and all AMD, VIA, Centaur and Intel Atoms have
this property.  64bit and XD were both added midway through the Pentium
4 era, and appear in the Prescott E0 stepping.

However, it appears that the prior stepping, D0, had 64bit but was only
unlocked for certain OEMs.  (At the time, Intel were still trying to
push Itaniaum as the future, and were trying hard not to go the x86_64
route.)

Specifically,
https://en.wikipedia.org/wiki/List_of_Intel_Xeon_processors_(NetBurst-based)#%22Nocona%22_(90_nm)
is the suspected problem set.


So, I think this does want to turn into a series, with the first patch
moving the XD-disable logic into this path, after which I think there is
a strong case to be made for defaulting CONFIG_REQUIRE_NX to yes.
 
~Andrew