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.li@intel.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Isaku Yamahata <isaku.yamahata@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 */
+
 #endif /* QEMU_I386_TDX_H */
-- 
2.34.1

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.li@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.li@intel.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Isaku Yamahata <isaku.yamahata@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?

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


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

Otherwise,

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

Thanks,
Zhao

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.li@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.li@intel.com>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>> Reviewed-by: Isaku Yamahata <isaku.yamahata@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.liu@intel.com>

Thanks!

> Thanks,
> Zhao
> 
>

> > > +#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.

But isn't AMD's SEV also defined as the VM type?

static const char *vm_type_name[] = {
    [KVM_X86_DEFAULT_VM] = "default",
    [KVM_X86_SEV_VM] = "SEV",
    [KVM_X86_SEV_ES_VM] = "SEV-ES",
    [KVM_X86_SNP_VM] = "SEV-SNP",
    [KVM_X86_TDX_VM] = "TDX",
};

Functionally, they are part of the coco functionality provided by
different vendors, so it's better thatt both could be in the same place
as much as possible, including file location, naming style. Of course,
it's not a big deal, and it can be cleaned up after merge if needed.

> I don't think the different name is a big issue, as nobody mentions it from
> the initial RFC to current v8 until you.

The Chinese saying: There are a thousand Hamlets in a thousand people's
eyes :-).

On 4/22/2025 10:20 PM, Zhao Liu wrote:
>>>> +#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.
> 
> But isn't AMD's SEV also defined as the VM type?

The initial SEV support that introduced sev_eanbled(), didn't introduce 
the VM type.

The specific type for SEV* was added later.

> static const char *vm_type_name[] = {
>      [KVM_X86_DEFAULT_VM] = "default",
>      [KVM_X86_SEV_VM] = "SEV",
>      [KVM_X86_SEV_ES_VM] = "SEV-ES",
>      [KVM_X86_SNP_VM] = "SEV-SNP",
>      [KVM_X86_TDX_VM] = "TDX",
> };
> 
> Functionally, they are part of the coco functionality provided by
> different vendors, so it's better thatt both could be in the same place
> as much as possible, including file location, naming style. Of course,
> it's not a big deal, and it can be cleaned up after merge if needed.

yes, cleanup can be a separate work if Paolo doesn't dislike.

>> I don't think the different name is a big issue, as nobody mentions it from
>> the initial RFC to current v8 until you.
> 
> The Chinese saying: There are a thousand Hamlets in a thousand people's
> eyes :-).
>