Re: [PATCH v1 03/11] x86/cpufeatures: Add TDX Guest CPU feature

Borislav Petkov
2021-06-10T12:28:31Z

On Tue, Jun 01, 2021 at 07:21:28PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Add CPU feature detection for Trusted Domain Extensions support.
> TDX feature adds capabilities to keep guest register state and
> memory isolated from hypervisor.
>
> For TDX guest platforms, executing CPUID(0x21, 0) will return

I'm assuming that 0 is ECX?

> following values in EAX, EBX, ECX and EDX.
>
> EAX: Maximum sub-leaf number: 0
> EBX/EDX/ECX: Vendor string:
>
> EBX = "Inte"
> EDX = "lTDX"
> ECX = " "
>
> So when above condition is true, set X86_FEATURE_TDX_GUEST
> feature cap bit
^
Fullstop.

> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index ac37830ae941..dddc3a27cc8a 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -238,6 +238,7 @@
> #define X86_FEATURE_VMW_VMMCALL ( 8*32+19) /* "" VMware prefers VMMCALL hypercall instruction */
> #define X86_FEATURE_PVUNLOCK ( 8*32+20) /* "" PV unlock function */
> #define X86_FEATURE_VCPUPREEMPT ( 8*32+21) /* "" PV vcpu_is_preempted function */
> +#define X86_FEATURE_TDX_GUEST ( 8*32+22) /* Trusted Domain Extensions Guest */

What's the name of the feature bit? "TDX guest"? Why not only
X86_FEATURE_TDX and then you can have "tdx" in cpuinfo?

>
> /* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */
> #define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> new file mode 100644
> index 000000000000..679500e807f3
> --- /dev/null
> +++ b/arch/x86/include/asm/tdx.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2020 Intel Corporation */
> +#ifndef _ASM_X86_TDX_H
> +#define _ASM_X86_TDX_H
> +
> +#define TDX_CPUID_LEAF_ID 0x21
> +
> +#ifdef CONFIG_INTEL_TDX_GUEST
> +
> +#include <asm/cpufeature.h>

As before - put the include at the top.

> +void __init tdx_early_init(void);
> +
> +#else // !CONFIG_INTEL_TDX_GUEST

No need for that // - comment

> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
> new file mode 100644
> index 000000000000..5b14b72e41c5
> --- /dev/null
> +++ b/arch/x86/kernel/tdx.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2020 Intel Corporation */
> +
> +#include <asm/tdx.h>
> +
> +static inline bool cpuid_has_tdx_guest(void)
> +{
> + u32 eax, signature[3];

Shorten that array name to "sig" so that you don't have to break the
cpuid_count() line below.

> +
> + if (cpuid_eax(0) < TDX_CPUID_LEAF_ID)
> + return false;
> +
> + cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &signature[0],
> + &signature[1], &signature[2]);
> +
> + if (memcmp("IntelTDX ", signature, 12))
> + return false;
> +
> + return true;

or simply:

return !memcmp(...

> +}
> +
> +void __init tdx_early_init(void)
> +{
> + if (!cpuid_has_tdx_guest())
> + return;
> +
> + setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
> +
> + pr_info("TDX guest is initialized\n");

Use pr_fmt in this file:

#undef pr_fmt
#define pr_fmt(fmt) "x86/tdx: " fmt

and then do

pr_info("Guest initialized\n");

Thx.

--
Regards/Gruss,
Boris.