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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> CC: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> CC: Gene Bright <gene@xxxxxxxxxxxxx>
> ---
>  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