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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.07.2024 12:18, 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@xxxxxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
for both patches, yet with two remarks and a nit here:

First: Cleanup in the earlier patch will get in the way of backporting
this easily. Let's hope I won't screw up.

> --- 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);

The base ISA not having ANDN or NAND (and a prereq to my patch to add
minimum-ABI-level control to the build machinery still sitting there
unreviewed), using "val ^ disable" here would likely produce slightly
better code for the time being.

> @@ -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

Nit: With the change to plural, switch to "don't"?

Jan