On 22/07/2024 11:43 am, Jan Beulich wrote:
> 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 <g...@cyberlight.us>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
> 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.
I'd just take both.
The reason the patches are this way around is because the reading of max
extd leaf needs moving in order to add the vendor check, and doing that
together in this patch made the diff far harder to follow.
>
>> --- 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.
While that might technically be true, you're assuming that everyone
reading the code can de-obfuscate ^ back into &~, and that the compiler
hasn't made its own alternative arrangements.
It's init code, not a fastpath.
>
>> @@ -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"?
Yes, my mistake. Fixed.
~Andrew
