Re: [PATCH v8 06/55] i386/tdx: Introduce is_tdx_vm() helper and cache tdx_guest object

[Xiaoyao Li]

On 4/18/2025 5:45 PM, Zhao Liu wrote:
On Tue, Apr 01, 2025 at 09:01:16AM -0400, Xiaoyao Li wrote:
Date: Tue,  1 Apr 2025 09:01:16 -0400
From: Xiaoyao Li <xiaoyao...@intel.com>
Subject: [PATCH v8 06/55] i386/tdx: Introduce is_tdx_vm() helper and cache
  tdx_guest object
X-Mailer: git-send-email 2.34.1
It will need special handling for TDX VMs all around the QEMU.
Introduce is_tdx_vm() helper to query if it's a TDX VM.

Cache tdx_guest object thus no need to cast from ms->cgs every time.

Signed-off-by: Xiaoyao Li <xiaoyao...@intel.com>
Acked-by: Gerd Hoffmann <kra...@redhat.com>
Reviewed-by: Isaku Yamahata <isaku.yamah...@intel.com>
---
changes in v3:
- replace object_dynamic_cast with TDX_GUEST();
---
  target/i386/kvm/tdx.c | 15 ++++++++++++++-
  target/i386/kvm/tdx.h | 10 ++++++++++
  2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index c67be5e618e2..16f67e18ae78 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -18,8 +18,16 @@
  #include "kvm_i386.h"
  #include "tdx.h"
  
+static TdxGuest *tdx_guest;
+
  static struct kvm_tdx_capabilities *tdx_caps;
  
+/* Valid after kvm_arch_init()->confidential_guest_kvm_init()->tdx_kvm_init() */
+bool is_tdx_vm(void)
+{
+    return !!tdx_guest;
+}
+
  enum tdx_ioctl_level {
      TDX_VM_IOCTL,
      TDX_VCPU_IOCTL,
@@ -117,15 +125,20 @@ static int get_tdx_capabilities(Error **errp)
  
  static int tdx_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
  {
+    TdxGuest *tdx = TDX_GUEST(cgs);
      int r = 0;
  
      kvm_mark_guest_state_protected();
  
      if (!tdx_caps) {
          r = get_tdx_capabilities(errp);
+        if (r) {
+            return r;
+        }
      }
  
-    return r;
+    tdx_guest = tdx;
+    return 0;
  }
  
  static int tdx_kvm_type(X86ConfidentialGuest *cg)
diff --git a/target/i386/kvm/tdx.h b/target/i386/kvm/tdx.h
index f3b725336161..de8ae9196163 100644
--- a/target/i386/kvm/tdx.h
+++ b/target/i386/kvm/tdx.h
@@ -3,6 +3,10 @@
  #ifndef QEMU_I386_TDX_H
  #define QEMU_I386_TDX_H
  
+#ifndef CONFIG_USER_ONLY
+#include CONFIG_DEVICES /* CONFIG_TDX */
+#endif
+
  #include "confidential-guest.h"
  
  #define TYPE_TDX_GUEST "tdx-guest"
@@ -18,4 +22,10 @@ typedef struct TdxGuest {
      uint64_t attributes;    /* TD attributes */
  } TdxGuest;
  
+#ifdef CONFIG_TDX
+bool is_tdx_vm(void);
+#else
+#define is_tdx_vm() 0
+#endif /* CONFIG_TDX */
+
  
a little nit: could we rename it as "tdx_enabled"?

Then the cases like these would be neater?

When sev support was added, it was seen as a feature for the VMs that 
are created on AMD platform. I think that's why it got called sev_enabled().

But for TDX, it is introduced as a different type of VM in contrast to 
the legacy/normal VMX VMs. We need to pass specific TDX vm type to 
KVM_CREATE_VM. Based on this, is_tdx_vm() was chosen.

I don't think the different name is a big issue, as nobody mentions it 
from the initial RFC to current v8 until you. So again, I will just keep 
it unchanged unless someone objects it strongly.

if (sev_enabled() || is_tdx_vm()) {
     ...
}


if (sev_enabled()) {
     ...
} else if (is_tdx_vm()) {
     ...
}

Otherwise,

Reviewed-by: Zhao Liu <zhao1....@intel.com>

Thanks!

Thanks,
Zhao