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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.