Re: [PATCH v1 1/3] x86/tdx: Check for TDX partitioning during early TDX init

Jeremi Piotrowski
2023-11-22T17:19:30Z

On 22/11/2023 18:01, Jeremi Piotrowski wrote:
> Check for additional CPUID bits to identify TDX guests running with Trust
> Domain (TD) partitioning enabled. TD partitioning is like nested virtualization
> inside the Trust Domain so there is a L1 TD VM(M) and there can be L2 TD VM(s).
>
> In this arrangement we are not guaranteed that the TDX_CPUID_LEAF_ID is visible
> to Linux running as an L2 TD VM. This is because a majority of TDX facilities
> are controlled by the L1 VMM and the L2 TDX guest needs to use TD partitioning
> aware mechanisms for what's left. So currently such guests do not have
> X86_FEATURE_TDX_GUEST set.
>
> We want the kernel to have X86_FEATURE_TDX_GUEST set for all TDX guests so we
> need to check these additional CPUID bits, but we skip further initialization
> in the function as we aren't guaranteed access to TDX module calls.
>
> Cc: <stable@vger.kernel.org> # v6.5+
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> ---
> arch/x86/coco/tdx/tdx.c | 29 ++++++++++++++++++++++++++---
> arch/x86/include/asm/tdx.h | 3 +++
> 2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 1d6b863c42b0..c7bbbaaf654d 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -8,6 +8,7 @@
> #include <linux/export.h>
> #include <linux/io.h>
> #include <asm/coco.h>
> +#include <asm/hyperv-tlfs.h>
> #include <asm/tdx.h>
> #include <asm/vmx.h>
> #include <asm/insn.h>
> @@ -37,6 +38,8 @@
>
> #define TDREPORT_SUBTYPE_0 0
>
> +bool tdx_partitioning_active;
> +
> /* Called from __tdx_hypercall() for unrecoverable failure */
> noinstr void __tdx_hypercall_failed(void)
> {
> @@ -757,19 +760,38 @@ static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> return true;
> }
>
> +
> +static bool early_is_hv_tdx_partitioning(void)
> +{
> + u32 eax, ebx, ecx, edx;
> + cpuid(HYPERV_CPUID_ISOLATION_CONFIG, &eax, &ebx, &ecx, &edx);
> + return eax & HV_PARAVISOR_PRESENT &&
> + (ebx & HV_ISOLATION_TYPE) == HV_ISOLATION_TYPE_TDX;
> +}
> +
> void __init tdx_early_init(void)
> {
> u64 cc_mask;
> u32 eax, sig[3];
>
> cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2], &sig[1]);
> -
> - if (memcmp(TDX_IDENT, sig, sizeof(sig)))
> - return;
> + if (memcmp(TDX_IDENT, sig, sizeof(sig))) {
> + tdx_partitioning_active = early_is_hv_tdx_partitioning();
> + if (!tdx_partitioning_active)
> + return;
> + }

Hi Borislav,

Just wanted to run another option by you. Instead of checking the CPUID here we
could accomplish the same result by doing _this_ in the hyperv cc init:

diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
index 8c6bf07f7d2b..705794642d34 100644
--- a/arch/x86/hyperv/ivm.c
+++ b/arch/x86/hyperv/ivm.c
@@ -595,6 +595,8 @@ void __init hv_vtom_init(void)
#endif

case HV_ISOLATION_TYPE_TDX:
+ setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
+ tdx_partitioning_active = true;
cc_vendor = CC_VENDOR_INTEL;
break;

Which approach do you prefer?

Thanks,
Jeremi

>
> setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
>
> cc_vendor = CC_VENDOR_INTEL;
> +
> + /*
> + * Need to defer cc_mask and page visibility callback initializations
> + * to a TD-partitioning aware implementation.
> + */
> + if (tdx_partitioning_active)
> + goto exit;
> +
> tdx_parse_tdinfo(&cc_mask);
> cc_set_mask(cc_mask);
>
> @@ -820,5 +842,6 @@ void __init tdx_early_init(void)
> */
> x86_cpuinit.parallel_bringup = false;
>
> +exit:
> pr_info("Guest detected\n");
> }
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 603e6d1e9d4a..fe22f8675859 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -52,6 +52,7 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
>
> int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
>
> +extern bool tdx_partitioning_active;
> #else
>
> static inline void tdx_early_init(void) { };
> @@ -71,6 +72,8 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
> {
> return -ENODEV;
> }
> +
> +#define tdx_partitioning_active false
> #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
> #endif /* !__ASSEMBLY__ */
> #endif /* _ASM_X86_TDX_H */