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>
---
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
Well, damn. At least it was found rather quickly.
On Mon Jul 22, 2024 at 11:18 AM BST, 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.
I suspect you meant boot rather than booth.
>
> 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>
> ---
> 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;
> +}
Do we want "#ifdef CONFIG_INTEL" the contents?
> +
> 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");
Cheers,
Alejandro
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
On Tue, Jul 23, 2024 at 12:25:32PM +0200, Marek Marczykowski-Górecki wrote:
> 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>
Ugh, wrong copy paste:
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
I should finish my coffee first...
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
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@cyberlight.us>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@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.
> --- 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
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 <gene@cyberlight.us>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@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
On 22/07/2024 6:04 pm, Andrew Cooper wrote:
> On 22/07/2024 11:43 am, Jan Beulich wrote:
>> On 22.07.2024 12:18, Andrew Cooper wrote:
>>> --- 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.
In fact, the compiler sees through this "clever" trick and undoes the XOR.
Swapping &~ for ^ makes no change in the compiled binary, because in
both cases GCC chooses a BTR instruction instead.
While BTR might be a poor choice of instruction for this purpose, it
reinforces my opinion that trickery such as this is not something we
want to do.
If you want a more useful optimisation task, we should figure out how to
write rdmsrl()/wrmsrl() better so GCC is happy working on %edx in
isolation, rather than always merging it into %rax to be operated on.
The rdmsr()/wrmsr() helpers taking a split hi and lo generate far better
code, even if they are much more awkward to use at a C level.
~Andrew
On 22.07.2024 19:38, Andrew Cooper wrote:
> On 22/07/2024 6:04 pm, Andrew Cooper wrote:
>> On 22/07/2024 11:43 am, Jan Beulich wrote:
>>> On 22.07.2024 12:18, Andrew Cooper wrote:
>>>> --- 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.
>
> In fact, the compiler sees through this "clever" trick and undoes the XOR.
>
> Swapping &~ for ^ makes no change in the compiled binary, because in
> both cases GCC chooses a BTR instruction instead.
Oh, okay.
> While BTR might be a poor choice of instruction for this purpose, it
> reinforces my opinion that trickery such as this is not something we
> want to do.
Just to mention it: I wouldn't have considered this to be "trickery".
> If you want a more useful optimisation task, we should figure out how to
> write rdmsrl()/wrmsrl() better so GCC is happy working on %edx in
> isolation, rather than always merging it into %rax to be operated on.
> The rdmsr()/wrmsr() helpers taking a split hi and lo generate far better
> code, even if they are much more awkward to use at a C level.
That may end up quite challenging without actually fiddling with the
compiler itself. Plus rdmsrl()/wrmsrl() themselves won't know how the
values are used in surrounding code, so improving one set of cases
may make things worse for another set. Introducing yet another variant
of them may also not be very desirable.
Jan
© 2016 - 2024 Red Hat, Inc.