Re: [PATCH 2/2] x86/efi: Unlock NX if necessary

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 3419 bytes --]

On Mon, Jul 22, 2024 at 11:18:38AM +0100, Andrew Cooper wrote:
> EFI systems can run with NX disabled, as has been discovered on a Broadwell
> Supermicro X10SRM-TF system.
> 
> Prior to commit fc3090a47b21 ("x86/boot: Clear XD_DISABLE from the early boot
> path"), the logic to unlock NX was common to all boot paths, but that commit
> moved it out of the native-EFI booth path.
> 
> Have the EFI path attempt to unlock NX, rather than just blindly refusing to
> boot when CONFIG_REQUIRE_NX is active.
> 
> Fixes: fc3090a47b21 ("x86/boot: Clear XD_DISABLE from the early boot path")
> Link: https://xcp-ng.org/forum/post/80520
> Reported-by: Gene Bright <gene@cyberlight.us>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> CC: Gene Bright <gene@cyberlight.us>
> 
> Note.  Entirely speculative coding, based only on the forum report.
> ---
>  xen/arch/x86/efi/efi-boot.h | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 4e4be7174751..158350aa14e4 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -736,13 +736,33 @@ static void __init efi_arch_handle_module(const struct file *file,
>      efi_bs->FreePool(ptr);
>  }
>  
> +static bool __init intel_unlock_nx(void)
> +{
> +    uint64_t val, disable;
> +
> +    rdmsrl(MSR_IA32_MISC_ENABLE, val);
> +
> +    disable = val & MSR_IA32_MISC_ENABLE_XD_DISABLE;
> +
> +    if ( !disable )
> +        return false;
> +
> +    wrmsrl(MSR_IA32_MISC_ENABLE, val & ~disable);
> +    trampoline_misc_enable_off |= disable;
> +
> +    return true;
> +}
> +
>  static void __init efi_arch_cpu(void)
>  {
> -    uint32_t eax;
> +    uint32_t eax, ebx, ecx, edx;
>      uint32_t *caps = boot_cpu_data.x86_capability;
>  
>      boot_tsc_stamp = rdtsc();
>  
> +    cpuid(0, &eax, &ebx, &ecx, &edx);
> +    boot_cpu_data.x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
> +
>      caps[FEATURESET_1c] = cpuid_ecx(1);
>  
>      eax = cpuid_eax(0x80000000U);
> @@ -752,10 +772,17 @@ static void __init efi_arch_cpu(void)
>      caps[FEATURESET_e1d] = cpuid_edx(0x80000001U);
>  
>      /*
> -     * This check purposefully doesn't use cpu_has_nx because
> +     * These checks purposefully doesn't use cpu_has_nx because
>       * cpu_has_nx bypasses the boot_cpu_data read if Xen was compiled
> -     * with CONFIG_REQUIRE_NX
> +     * with CONFIG_REQUIRE_NX.
> +     *
> +     * If NX isn't available, it might be hidden.  Try to reactivate it.
>       */
> +    if ( !boot_cpu_has(X86_FEATURE_NX) &&
> +         boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> +         intel_unlock_nx() )
> +        caps[FEATURESET_e1d] = cpuid_edx(0x80000001U);
> +
>      if ( IS_ENABLED(CONFIG_REQUIRE_NX) &&
>           !boot_cpu_has(X86_FEATURE_NX) )
>          blexit(L"This build of Xen requires NX support");
> -- 
> 2.39.2
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]