[PATCH 0/2] x86/efi: Fix booting when NX is disabled

[PATCH 0/2] x86/efi: Fix booting when NX is disabled

[Andrew Cooper]

Bugfix from the XCP-ng forums.  See patch 2 for details.

Apparently the native path with NX disabled is broken too, but I've not had
any time to look into that yet.

Andrew Cooper (2):
  x86/efi: Simplify efi_arch_cpu() a little
  x86/efi: Unlock NX if necessary

 xen/arch/x86/efi/efi-boot.h | 56 +++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 14 deletions(-)


base-commit: ff652ed5dcd797a46c84258255dfd429ae68a2d6
-- 
2.39.2

[PATCH 1/2] x86/efi: Simplify efi_arch_cpu() a little

[Andrew Cooper]

Make the "no extended leaves" case fatal and remove one level of indentation.
Defer the max-leaf aquisition until it is first used.

No functional change.

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>
---
 xen/arch/x86/efi/efi-boot.h | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index f282358435f1..4e4be7174751 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -738,29 +738,30 @@ static void __init efi_arch_handle_module(const struct file *file,
 
 static void __init efi_arch_cpu(void)
 {
-    uint32_t eax = cpuid_eax(0x80000000U);
+    uint32_t eax;
     uint32_t *caps = boot_cpu_data.x86_capability;
 
     boot_tsc_stamp = rdtsc();
 
     caps[FEATURESET_1c] = cpuid_ecx(1);
 
-    if ( (eax >> 16) == 0x8000 && eax > 0x80000000U )
-    {
-        caps[FEATURESET_e1d] = cpuid_edx(0x80000001U);
+    eax = cpuid_eax(0x80000000U);
+    if ( (eax >> 16) != 0x8000 || eax < 0x80000000U )
+        blexit(L"In 64bit mode, but no extended CPUID leaves?!?");
 
-        /*
-         * This check 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
-         */
-        if ( IS_ENABLED(CONFIG_REQUIRE_NX) &&
-             !boot_cpu_has(X86_FEATURE_NX) )
-            blexit(L"This build of Xen requires NX support");
+    caps[FEATURESET_e1d] = cpuid_edx(0x80000001U);
 
-        if ( cpu_has_nx )
-            trampoline_efer |= EFER_NXE;
-    }
+    /*
+     * This check 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
+     */
+    if ( IS_ENABLED(CONFIG_REQUIRE_NX) &&
+         !boot_cpu_has(X86_FEATURE_NX) )
+        blexit(L"This build of Xen requires NX support");
+
+    if ( cpu_has_nx )
+        trampoline_efer |= EFER_NXE;
 }
 
 static void __init efi_arch_blexit(void)
-- 
2.39.2

Re: [PATCH 1/2] x86/efi: Simplify efi_arch_cpu() a little

[Marek Marczykowski-Górecki]

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

On Mon, Jul 22, 2024 at 11:18:37AM +0100, Andrew Cooper wrote:
> Make the "no extended leaves" case fatal and remove one level of indentation.
> Defer the max-leaf aquisition until it is first used.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.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>
> ---
>  xen/arch/x86/efi/efi-boot.h | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index f282358435f1..4e4be7174751 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -738,29 +738,30 @@ static void __init efi_arch_handle_module(const struct file *file,
>  
>  static void __init efi_arch_cpu(void)
>  {
> -    uint32_t eax = cpuid_eax(0x80000000U);
> +    uint32_t eax;
>      uint32_t *caps = boot_cpu_data.x86_capability;
>  
>      boot_tsc_stamp = rdtsc();
>  
>      caps[FEATURESET_1c] = cpuid_ecx(1);
>  
> -    if ( (eax >> 16) == 0x8000 && eax > 0x80000000U )
> -    {
> -        caps[FEATURESET_e1d] = cpuid_edx(0x80000001U);
> +    eax = cpuid_eax(0x80000000U);
> +    if ( (eax >> 16) != 0x8000 || eax < 0x80000000U )
> +        blexit(L"In 64bit mode, but no extended CPUID leaves?!?");
>  
> -        /*
> -         * This check 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
> -         */
> -        if ( IS_ENABLED(CONFIG_REQUIRE_NX) &&
> -             !boot_cpu_has(X86_FEATURE_NX) )
> -            blexit(L"This build of Xen requires NX support");
> +    caps[FEATURESET_e1d] = cpuid_edx(0x80000001U);
>  
> -        if ( cpu_has_nx )
> -            trampoline_efer |= EFER_NXE;
> -    }
> +    /*
> +     * This check 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
> +     */
> +    if ( IS_ENABLED(CONFIG_REQUIRE_NX) &&
> +         !boot_cpu_has(X86_FEATURE_NX) )
> +        blexit(L"This build of Xen requires NX support");
> +
> +    if ( cpu_has_nx )
> +        trampoline_efer |= EFER_NXE;
>  }
>  
>  static void __init efi_arch_blexit(void)
> -- 
> 2.39.2
> 

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

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

Re: [PATCH 1/2] x86/efi: Simplify efi_arch_cpu() a little

[Alejandro Vallejo]

On Mon Jul 22, 2024 at 11:18 AM BST, Andrew Cooper wrote:
> Make the "no extended leaves" case fatal and remove one level of indentation.
> Defer the max-leaf aquisition until it is first used.
>
> No functional change.
>
> 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>
> ---
>  xen/arch/x86/efi/efi-boot.h | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index f282358435f1..4e4be7174751 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -738,29 +738,30 @@ static void __init efi_arch_handle_module(const struct file *file,
>  
>  static void __init efi_arch_cpu(void)
>  {
> -    uint32_t eax = cpuid_eax(0x80000000U);
> +    uint32_t eax;
>      uint32_t *caps = boot_cpu_data.x86_capability;
>  
>      boot_tsc_stamp = rdtsc();
>  
>      caps[FEATURESET_1c] = cpuid_ecx(1);
>  
> -    if ( (eax >> 16) == 0x8000 && eax > 0x80000000U )
> -    {
> -        caps[FEATURESET_e1d] = cpuid_edx(0x80000001U);
> +    eax = cpuid_eax(0x80000000U);

Why this movement?

> +    if ( (eax >> 16) != 0x8000 || eax < 0x80000000U )
> +        blexit(L"In 64bit mode, but no extended CPUID leaves?!?");

I'm not sure about the condition even for the old code. If eax had 0x90000000
(because new convention appeared 10y in the future), then there would be
extended leaves but we would be needlessly bailing out. Why not simply check
that eax < 0x80000001 in here?

>  
> -        /*
> -         * This check 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
> -         */
> -        if ( IS_ENABLED(CONFIG_REQUIRE_NX) &&
> -             !boot_cpu_has(X86_FEATURE_NX) )
> -            blexit(L"This build of Xen requires NX support");
> +    caps[FEATURESET_e1d] = cpuid_edx(0x80000001U);
>  
> -        if ( cpu_has_nx )
> -            trampoline_efer |= EFER_NXE;
> -    }
> +    /*
> +     * This check 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
> +     */
> +    if ( IS_ENABLED(CONFIG_REQUIRE_NX) &&
> +         !boot_cpu_has(X86_FEATURE_NX) )
> +        blexit(L"This build of Xen requires NX support");
> +
> +    if ( cpu_has_nx )
> +        trampoline_efer |= EFER_NXE;
>  }
>  
>  static void __init efi_arch_blexit(void)

Cheers,
Alejandro

Re: [PATCH 1/2] x86/efi: Simplify efi_arch_cpu() a little

[Jan Beulich]

On 23.07.2024 15:47, Alejandro Vallejo wrote:
> On Mon Jul 22, 2024 at 11:18 AM BST, Andrew Cooper wrote:
>> +    if ( (eax >> 16) != 0x8000 || eax < 0x80000000U )
>> +        blexit(L"In 64bit mode, but no extended CPUID leaves?!?");
> 
> I'm not sure about the condition even for the old code. If eax had 0x90000000
> (because new convention appeared 10y in the future), then there would be
> extended leaves but we would be needlessly bailing out. Why not simply check
> that eax < 0x80000001 in here?

eax = 0x90000000 is in leaf group 0x9000, not in the extended leaf group
(0x8000). The splitting into groups may not be written down very well,
but you can see the pattern in e.g. groups 0x8086 and 0xc000 also being
used (by non-Intel non-AMD hardware), without those really being extended
leaves in the sense that 0x8000xxxx are.

Jan

Re: [PATCH 1/2] x86/efi: Simplify efi_arch_cpu() a little

[Alejandro Vallejo]

On Wed Jul 24, 2024 at 6:42 AM BST, Jan Beulich wrote:
> On 23.07.2024 15:47, Alejandro Vallejo wrote:
> > On Mon Jul 22, 2024 at 11:18 AM BST, Andrew Cooper wrote:
> >> +    if ( (eax >> 16) != 0x8000 || eax < 0x80000000U )
> >> +        blexit(L"In 64bit mode, but no extended CPUID leaves?!?");
> > 
> > I'm not sure about the condition even for the old code. If eax had 0x90000000
> > (because new convention appeared 10y in the future), then there would be
> > extended leaves but we would be needlessly bailing out. Why not simply check
> > that eax < 0x80000001 in here?
>
> eax = 0x90000000 is in leaf group 0x9000, not in the extended leaf group
> (0x8000). The splitting into groups may not be written down very well,
> but you can see the pattern in e.g. groups 0x8086 and 0xc000 also being
> used (by non-Intel non-AMD hardware), without those really being extended
> leaves in the sense that 0x8000xxxx are.
>
> Jan

The code is checking for a number specifically in the extended group, but
that's the output of leaf 0x80000000 which is defined to be just that.

AMD: "The value returned in EAX provides the largest extended function number
      supported by the processor"

Intel: "Maximum Input Value for Extended Function CPUID Information."

Unless there are quirks I don't know about (I admit it's not unlikely) I just
don't see why this condition needs to be anything else than a check that the
maximum function number is bigger than any of the leaves we read further ahead.

If the number happens to start with 8000, that'd be fine; but there's no reason
to bail out if it was 8001. And even if there was, the exit message is
misleading as it's claiming there's no extended CPUID leaves when in reality an
unexpected max-extended-leaf was read off the base extended leaf.

Not that it matters a whole lot in practice because that's going to be within
range. But it feels like a needless complication of the check.

Regardless, as I said it's more of a comment on the previous code than it is
about this mechanical transformation.

Cheers,
Alejandro

Re: [PATCH 1/2] x86/efi: Simplify efi_arch_cpu() a little

[Jan Beulich]

On 24.07.2024 15:28, Alejandro Vallejo wrote:
> On Wed Jul 24, 2024 at 6:42 AM BST, Jan Beulich wrote:
>> On 23.07.2024 15:47, Alejandro Vallejo wrote:
>>> On Mon Jul 22, 2024 at 11:18 AM BST, Andrew Cooper wrote:
>>>> +    if ( (eax >> 16) != 0x8000 || eax < 0x80000000U )
>>>> +        blexit(L"In 64bit mode, but no extended CPUID leaves?!?");
>>>
>>> I'm not sure about the condition even for the old code. If eax had 0x90000000
>>> (because new convention appeared 10y in the future), then there would be
>>> extended leaves but we would be needlessly bailing out. Why not simply check
>>> that eax < 0x80000001 in here?
>>
>> eax = 0x90000000 is in leaf group 0x9000, not in the extended leaf group
>> (0x8000). The splitting into groups may not be written down very well,
>> but you can see the pattern in e.g. groups 0x8086 and 0xc000 also being
>> used (by non-Intel non-AMD hardware), without those really being extended
>> leaves in the sense that 0x8000xxxx are.
>>
>> Jan
> 
> The code is checking for a number specifically in the extended group, but
> that's the output of leaf 0x80000000 which is defined to be just that.
> 
> AMD: "The value returned in EAX provides the largest extended function number
>       supported by the processor"
> 
> Intel: "Maximum Input Value for Extended Function CPUID Information."
> 
> Unless there are quirks I don't know about (I admit it's not unlikely) I just
> don't see why this condition needs to be anything else than a check that the
> maximum function number is bigger than any of the leaves we read further ahead.
> 
> If the number happens to start with 8000, that'd be fine; but there's no reason
> to bail out if it was 8001.

How do you know? We'll learn once someone starts populating that leaf
group. It _may_ be the continuation of extended leaves then (once the
other 64k were all consumed, i.e. in perhaps hundreds of years). Just
take again the case where the 8086 groups is populated: What if there
[80000000].eax = 8086yyyy? That'll be wrong, as 8086 forms its own group.
So no, I'm similarly unaware of quirks, but with this we're trying to
guard ourselves against some entirely bogus output (from all we know
today).

Jan

Re: [PATCH 1/2] x86/efi: Simplify efi_arch_cpu() a little

[Jan Beulich]

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
> @@ -738,29 +738,30 @@ static void __init efi_arch_handle_module(const struct file *file,
>  
>  static void __init efi_arch_cpu(void)
>  {
> -    uint32_t eax = cpuid_eax(0x80000000U);
> +    uint32_t eax;
>      uint32_t *caps = boot_cpu_data.x86_capability;
>  
>      boot_tsc_stamp = rdtsc();
>  
>      caps[FEATURESET_1c] = cpuid_ecx(1);
>  
> -    if ( (eax >> 16) == 0x8000 && eax > 0x80000000U )
> -    {
> -        caps[FEATURESET_e1d] = cpuid_edx(0x80000001U);
> +    eax = cpuid_eax(0x80000000U);
> +    if ( (eax >> 16) != 0x8000 || eax < 0x80000000U )

Only in the context of the further discussion with Alejandro I've spotted
that the rhs of the || is now dead code. A proper transformation of the
earlier condition would have required <= in place of <.

Jan

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

[Andrew Cooper]

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

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

[Jan Beulich]

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

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

[Andrew Cooper]

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

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

[Andrew Cooper]

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

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

[Jan Beulich]

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

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

[Marek Marczykowski-Górecki]

[-- 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 --]

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

[Marek Marczykowski-Górecki]

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

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

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

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

[Alejandro Vallejo]

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