Invoke KVM_TDX_INIT in kvm_arch_pre_create_vcpu() that KVM_TDX_INIT
configures global TD configurations, e.g. the canonical CPUID config,
and must be executed prior to creating vCPUs.

Use kvm_x86_arch_cpuid() to setup the CPUID settings for TDX VM.

Note, this doesn't address the fact that QEMU may change the CPUID
configuration when creating vCPUs, i.e. punts on refactoring QEMU to
provide a stable CPUID config prior to kvm_arch_init().

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Markus Armbruster <armbru@redhat.com>
---
Changes in v6:
- setup xfam explicitly to fit with new uapi;
- use tdx_caps->cpuid to filter the input of cpuids because now KVM only
  allows the leafs that reported via KVM_TDX_GET_CAPABILITIES;

Changes in v4:
- mark init_vm with g_autofree() and use QEMU_LOCK_GUARD() to eliminate
  the goto labels; (Daniel)
Changes in v3:
- Pass @errp in tdx_pre_create_vcpu() and pass error info to it. (Daniel)
---
 accel/kvm/kvm-all.c         |  8 ++++
 target/i386/kvm/kvm.c       | 15 +++++--
 target/i386/kvm/kvm_i386.h  |  3 ++
 target/i386/kvm/meson.build |  2 +-
 target/i386/kvm/tdx-stub.c  |  8 ++++
 target/i386/kvm/tdx.c       | 87 +++++++++++++++++++++++++++++++++++++
 target/i386/kvm/tdx.h       |  6 +++
 7 files changed, 125 insertions(+), 4 deletions(-)
 create mode 100644 target/i386/kvm/tdx-stub.c

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 1732fa1adecd..4a1c9950894c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -536,8 +536,15 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
 
     trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
 
+    /*
+     * tdx_pre_create_vcpu() may call cpu_x86_cpuid(). It in turn may call
+     * kvm_vm_ioctl(). Set cpu->kvm_state in advance to avoid NULL pointer
+     * dereference.
+     */
+    cpu->kvm_state = s;
     ret = kvm_arch_pre_create_vcpu(cpu, errp);
     if (ret < 0) {
+        cpu->kvm_state = NULL;
         goto err;
     }
 
@@ -546,6 +553,7 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
         error_setg_errno(errp, -ret,
                          "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
                          kvm_arch_vcpu_id(cpu));
+        cpu->kvm_state = NULL;
         goto err;
     }
 
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index afbf67a7fdaa..db676c1336ab 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -38,6 +38,7 @@
 #include "kvm_i386.h"
 #include "../confidential-guest.h"
 #include "sev.h"
+#include "tdx.h"
 #include "xen-emu.h"
 #include "hyperv.h"
 #include "hyperv-proto.h"
@@ -1824,9 +1825,8 @@ static void kvm_init_nested_state(CPUX86State *env)
     }
 }
 
-static uint32_t kvm_x86_build_cpuid(CPUX86State *env,
-                                    struct kvm_cpuid_entry2 *entries,
-                                    uint32_t cpuid_i)
+uint32_t kvm_x86_build_cpuid(CPUX86State *env, struct kvm_cpuid_entry2 *entries,
+                             uint32_t cpuid_i)
 {
     uint32_t limit, i, j;
     uint32_t unused;
@@ -2358,6 +2358,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
     return r;
 }
 
+int kvm_arch_pre_create_vcpu(CPUState *cpu, Error **errp)
+{
+    if (is_tdx_vm()) {
+        return tdx_pre_create_vcpu(cpu, errp);
+    }
+
+    return 0;
+}
+
 int kvm_arch_destroy_vcpu(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
index efb0883bd968..b1baf9e7f910 100644
--- a/target/i386/kvm/kvm_i386.h
+++ b/target/i386/kvm/kvm_i386.h
@@ -24,6 +24,9 @@
 #define kvm_ioapic_in_kernel() \
     (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split())
 
+uint32_t kvm_x86_build_cpuid(CPUX86State *env, struct kvm_cpuid_entry2 *entries,
+                             uint32_t cpuid_i);
+
 #else
 
 #define kvm_pit_in_kernel()      0
diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
index 466bccb9cb17..3f44cdedb758 100644
--- a/target/i386/kvm/meson.build
+++ b/target/i386/kvm/meson.build
@@ -8,7 +8,7 @@ i386_kvm_ss.add(files(
 
 i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files('xen-emu.c'))
 
-i386_kvm_ss.add(when: 'CONFIG_TDX', if_true: files('tdx.c'))
+i386_kvm_ss.add(when: 'CONFIG_TDX', if_true: files('tdx.c'), if_false: files('tdx-stub.c'))
 
 i386_system_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
 
diff --git a/target/i386/kvm/tdx-stub.c b/target/i386/kvm/tdx-stub.c
new file mode 100644
index 000000000000..b614b46d3f4a
--- /dev/null
+++ b/target/i386/kvm/tdx-stub.c
@@ -0,0 +1,8 @@
+#include "qemu/osdep.h"
+
+#include "tdx.h"
+
+int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
+{
+    return -EINVAL;
+}
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index ff3ef9bd8657..1b7894e43c6f 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -137,6 +137,91 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg)
     return KVM_X86_TDX_VM;
 }
 
+static int setup_td_xfam(X86CPU *x86cpu, Error **errp)
+{
+    CPUX86State *env = &x86cpu->env;
+    uint64_t xfam;
+
+    xfam = env->features[FEAT_XSAVE_XCR0_LO] |
+           env->features[FEAT_XSAVE_XCR0_HI] |
+           env->features[FEAT_XSAVE_XSS_LO] |
+           env->features[FEAT_XSAVE_XSS_HI];
+
+    if (xfam & ~tdx_caps->supported_xfam) {
+        error_setg(errp, "Invalid XFAM 0x%lx for TDX VM (supported: 0x%llx))",
+                   xfam, tdx_caps->supported_xfam);
+        return -1;
+    }
+
+    tdx_guest->xfam = xfam;
+    return 0;
+}
+
+static void tdx_filter_cpuid(struct kvm_cpuid2 *cpuids)
+{
+    int i, dest_cnt = 0;
+    struct kvm_cpuid_entry2 *src, *dest, *conf;
+
+    for (i = 0; i < cpuids->nent; i++) {
+        src = cpuids->entries + i;
+        conf = cpuid_find_entry(&tdx_caps->cpuid, src->function, src->index);
+        if (!conf) {
+            continue;
+        }
+        dest = cpuids->entries + dest_cnt;
+
+        dest->function = src->function;
+        dest->index = src->index;
+        dest->flags = src->flags;
+        dest->eax = src->eax & conf->eax;
+        dest->ebx = src->ebx & conf->ebx;
+        dest->ecx = src->ecx & conf->ecx;
+        dest->edx = src->edx & conf->edx;
+
+        dest_cnt++;
+    }
+    cpuids->nent = dest_cnt++;
+}
+
+int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
+{
+    X86CPU *x86cpu = X86_CPU(cpu);
+    CPUX86State *env = &x86cpu->env;
+    g_autofree struct kvm_tdx_init_vm *init_vm = NULL;
+    int r = 0;
+
+    QEMU_LOCK_GUARD(&tdx_guest->lock);
+    if (tdx_guest->initialized) {
+        return r;
+    }
+
+    init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) +
+                        sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
+
+    r = setup_td_xfam(x86cpu, errp);
+    if (r) {
+        return r;
+    }
+
+    init_vm->cpuid.nent = kvm_x86_build_cpuid(env, init_vm->cpuid.entries, 0);
+    tdx_filter_cpuid(&init_vm->cpuid);
+
+    init_vm->attributes = tdx_guest->attributes;
+    init_vm->xfam = tdx_guest->xfam;
+
+    do {
+        r = tdx_vm_ioctl(KVM_TDX_INIT_VM, 0, init_vm);
+    } while (r == -EAGAIN);
+    if (r < 0) {
+        error_setg_errno(errp, -r, "KVM_TDX_INIT_VM failed");
+        return r;
+    }
+
+    tdx_guest->initialized = true;
+
+    return 0;
+}
+
 /* tdx guest */
 OBJECT_DEFINE_TYPE_WITH_INTERFACES(TdxGuest,
                                    tdx_guest,
@@ -150,6 +235,8 @@ static void tdx_guest_init(Object *obj)
     ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
     TdxGuest *tdx = TDX_GUEST(obj);
 
+    qemu_mutex_init(&tdx->lock);
+
     cgs->require_guest_memfd = true;
     tdx->attributes = 0;
 
diff --git a/target/i386/kvm/tdx.h b/target/i386/kvm/tdx.h
index bca19c833e18..e077fd7d1653 100644
--- a/target/i386/kvm/tdx.h
+++ b/target/i386/kvm/tdx.h
@@ -17,7 +17,11 @@ typedef struct TdxGuestClass {
 typedef struct TdxGuest {
     X86ConfidentialGuest parent_obj;
 
+    QemuMutex lock;
+
+    bool initialized;
     uint64_t attributes;    /* TD attributes */
+    uint64_t xfam;
 } TdxGuest;
 
 #ifdef CONFIG_TDX
@@ -26,4 +30,6 @@ bool is_tdx_vm(void);
 #define is_tdx_vm() 0
 #endif /* CONFIG_TDX */
 
+int tdx_pre_create_vcpu(CPUState *cpu, Error **errp);
+
 #endif /* QEMU_I386_TDX_H */
-- 
2.34.1

+Tony

On Tue, 2024-11-05 at 01:23 -0500, Xiaoyao Li wrote:
> +int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
> +{
> +    X86CPU *x86cpu = X86_CPU(cpu);
> +    CPUX86State *env = &x86cpu->env;
> +    g_autofree struct kvm_tdx_init_vm *init_vm = NULL;
> +    int r = 0;
> +
> +    QEMU_LOCK_GUARD(&tdx_guest->lock);
> +    if (tdx_guest->initialized) {
> +        return r;
> +    }
> +
> +    init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) +
> +                        sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
> +
> +    r = setup_td_xfam(x86cpu, errp);
> +    if (r) {
> +        return r;
> +    }
> +
> +    init_vm->cpuid.nent = kvm_x86_build_cpuid(env, init_vm->cpuid.entries, 0);
> +    tdx_filter_cpuid(&init_vm->cpuid);
> +
> +    init_vm->attributes = tdx_guest->attributes;
> +    init_vm->xfam = tdx_guest->xfam;
> +
> +    do {
> +        r = tdx_vm_ioctl(KVM_TDX_INIT_VM, 0, init_vm);
> +    } while (r == -EAGAIN);

KVM_TDX_INIT_VM can also return EBUSY. This should check for it, or KVM should
standardize on one for both conditions. In KVM, both cases handle
TDX_RND_NO_ENTROPY, but one tries to save some of the initialization for the
next attempt. I don't know why userspace would need to differentiate between the
two cases though, which makes me think we should just change the KVM side.

> +    if (r < 0) {
> +        error_setg_errno(errp, -r, "KVM_TDX_INIT_VM failed");
> +        return r;
> +    }
> +
> +    tdx_guest->initialized = true;
> +
> +    return 0;
> +}

On 11/6/2024 4:51 AM, Edgecombe, Rick P wrote:
> +Tony
> 
> On Tue, 2024-11-05 at 01:23 -0500, Xiaoyao Li wrote:
>> +int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
>> +{
>> +    X86CPU *x86cpu = X86_CPU(cpu);
>> +    CPUX86State *env = &x86cpu->env;
>> +    g_autofree struct kvm_tdx_init_vm *init_vm = NULL;
>> +    int r = 0;
>> +
>> +    QEMU_LOCK_GUARD(&tdx_guest->lock);
>> +    if (tdx_guest->initialized) {
>> +        return r;
>> +    }
>> +
>> +    init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) +
>> +                        sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
>> +
>> +    r = setup_td_xfam(x86cpu, errp);
>> +    if (r) {
>> +        return r;
>> +    }
>> +
>> +    init_vm->cpuid.nent = kvm_x86_build_cpuid(env, init_vm->cpuid.entries, 0);
>> +    tdx_filter_cpuid(&init_vm->cpuid);
>> +
>> +    init_vm->attributes = tdx_guest->attributes;
>> +    init_vm->xfam = tdx_guest->xfam;
>> +
>> +    do {
>> +        r = tdx_vm_ioctl(KVM_TDX_INIT_VM, 0, init_vm);
>> +    } while (r == -EAGAIN);
> 
> KVM_TDX_INIT_VM can also return EBUSY. This should check for it, or KVM should
> standardize on one for both conditions. In KVM, both cases handle
> TDX_RND_NO_ENTROPY, but one tries to save some of the initialization for the
> next attempt. I don't know why userspace would need to differentiate between the
> two cases though, which makes me think we should just change the KVM side.

I remember I tested retrying on the two cases and no surprise showed.

I agree to change KVM side to return -EAGAIN for the two cases.

>> +    if (r < 0) {
>> +        error_setg_errno(errp, -r, "KVM_TDX_INIT_VM failed");
>> +        return r;
>> +    }
>> +
>> +    tdx_guest->initialized = true;
>> +
>> +    return 0;
>> +}
> 

On Wed, Nov 06, 2024 at 10:01:04AM +0800, Xiaoyao Li wrote:
> On 11/6/2024 4:51 AM, Edgecombe, Rick P wrote:
> > +Tony
> > 
> > On Tue, 2024-11-05 at 01:23 -0500, Xiaoyao Li wrote:
> > > +int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
> > > +{
> > > +    X86CPU *x86cpu = X86_CPU(cpu);
> > > +    CPUX86State *env = &x86cpu->env;
> > > +    g_autofree struct kvm_tdx_init_vm *init_vm = NULL;
> > > +    int r = 0;
> > > +
> > > +    QEMU_LOCK_GUARD(&tdx_guest->lock);
> > > +    if (tdx_guest->initialized) {
> > > +        return r;
> > > +    }
> > > +
> > > +    init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) +
> > > +                        sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
> > > +
> > > +    r = setup_td_xfam(x86cpu, errp);
> > > +    if (r) {
> > > +        return r;
> > > +    }
> > > +
> > > +    init_vm->cpuid.nent = kvm_x86_build_cpuid(env, init_vm->cpuid.entries, 0);
> > > +    tdx_filter_cpuid(&init_vm->cpuid);
> > > +
> > > +    init_vm->attributes = tdx_guest->attributes;
> > > +    init_vm->xfam = tdx_guest->xfam;
> > > +
> > > +    do {
> > > +        r = tdx_vm_ioctl(KVM_TDX_INIT_VM, 0, init_vm);
> > > +    } while (r == -EAGAIN);
> > 
> > KVM_TDX_INIT_VM can also return EBUSY. This should check for it, or KVM should
> > standardize on one for both conditions. In KVM, both cases handle
> > TDX_RND_NO_ENTROPY, but one tries to save some of the initialization for the
> > next attempt. I don't know why userspace would need to differentiate between the
> > two cases though, which makes me think we should just change the KVM side.
> 
> I remember I tested retrying on the two cases and no surprise showed.
> 
> I agree to change KVM side to return -EAGAIN for the two cases.

OK yeah let's patch KVM for it.

Regards,

Tony

On Tue, Nov 05, 2024 at 01:23:17AM -0500, Xiaoyao Li wrote:
> Invoke KVM_TDX_INIT in kvm_arch_pre_create_vcpu() that KVM_TDX_INIT
> configures global TD configurations, e.g. the canonical CPUID config,
> and must be executed prior to creating vCPUs.
> 
> Use kvm_x86_arch_cpuid() to setup the CPUID settings for TDX VM.
> 
> Note, this doesn't address the fact that QEMU may change the CPUID
> configuration when creating vCPUs, i.e. punts on refactoring QEMU to
> provide a stable CPUID config prior to kvm_arch_init().
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Markus Armbruster <armbru@redhat.com>
> ---
> Changes in v6:
> - setup xfam explicitly to fit with new uapi;
> - use tdx_caps->cpuid to filter the input of cpuids because now KVM only
>   allows the leafs that reported via KVM_TDX_GET_CAPABILITIES;
> 
> Changes in v4:
> - mark init_vm with g_autofree() and use QEMU_LOCK_GUARD() to eliminate
>   the goto labels; (Daniel)
> Changes in v3:
> - Pass @errp in tdx_pre_create_vcpu() and pass error info to it. (Daniel)
> ---
>  accel/kvm/kvm-all.c         |  8 ++++
>  target/i386/kvm/kvm.c       | 15 +++++--
>  target/i386/kvm/kvm_i386.h  |  3 ++
>  target/i386/kvm/meson.build |  2 +-
>  target/i386/kvm/tdx-stub.c  |  8 ++++
>  target/i386/kvm/tdx.c       | 87 +++++++++++++++++++++++++++++++++++++
>  target/i386/kvm/tdx.h       |  6 +++
>  7 files changed, 125 insertions(+), 4 deletions(-)
>  create mode 100644 target/i386/kvm/tdx-stub.c
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 1732fa1adecd..4a1c9950894c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -536,8 +536,15 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>  
>      trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  
> +    /*
> +     * tdx_pre_create_vcpu() may call cpu_x86_cpuid(). It in turn may call
> +     * kvm_vm_ioctl(). Set cpu->kvm_state in advance to avoid NULL pointer
> +     * dereference.
> +     */
> +    cpu->kvm_state = s;
>      ret = kvm_arch_pre_create_vcpu(cpu, errp);
>      if (ret < 0) {
> +        cpu->kvm_state = NULL;
>          goto err;
>      }
>  
> @@ -546,6 +553,7 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>          error_setg_errno(errp, -ret,
>                           "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
>                           kvm_arch_vcpu_id(cpu));
> +        cpu->kvm_state = NULL;
>          goto err;
>      }
>  
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index afbf67a7fdaa..db676c1336ab 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -38,6 +38,7 @@
>  #include "kvm_i386.h"
>  #include "../confidential-guest.h"
>  #include "sev.h"
> +#include "tdx.h"
>  #include "xen-emu.h"
>  #include "hyperv.h"
>  #include "hyperv-proto.h"
> @@ -1824,9 +1825,8 @@ static void kvm_init_nested_state(CPUX86State *env)
>      }
>  }
>  
> -static uint32_t kvm_x86_build_cpuid(CPUX86State *env,
> -                                    struct kvm_cpuid_entry2 *entries,
> -                                    uint32_t cpuid_i)
> +uint32_t kvm_x86_build_cpuid(CPUX86State *env, struct kvm_cpuid_entry2 *entries,
> +                             uint32_t cpuid_i)
>  {
>      uint32_t limit, i, j;
>      uint32_t unused;
> @@ -2358,6 +2358,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      return r;
>  }
>  
> +int kvm_arch_pre_create_vcpu(CPUState *cpu, Error **errp)
> +{
> +    if (is_tdx_vm()) {
> +        return tdx_pre_create_vcpu(cpu, errp);
> +    }
> +
> +    return 0;
> +}
> +
>  int kvm_arch_destroy_vcpu(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
> index efb0883bd968..b1baf9e7f910 100644
> --- a/target/i386/kvm/kvm_i386.h
> +++ b/target/i386/kvm/kvm_i386.h
> @@ -24,6 +24,9 @@
>  #define kvm_ioapic_in_kernel() \
>      (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split())
>  
> +uint32_t kvm_x86_build_cpuid(CPUX86State *env, struct kvm_cpuid_entry2 *entries,
> +                             uint32_t cpuid_i);
> +
>  #else
>  
>  #define kvm_pit_in_kernel()      0
> diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
> index 466bccb9cb17..3f44cdedb758 100644
> --- a/target/i386/kvm/meson.build
> +++ b/target/i386/kvm/meson.build
> @@ -8,7 +8,7 @@ i386_kvm_ss.add(files(
>  
>  i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files('xen-emu.c'))
>  
> -i386_kvm_ss.add(when: 'CONFIG_TDX', if_true: files('tdx.c'))
> +i386_kvm_ss.add(when: 'CONFIG_TDX', if_true: files('tdx.c'), if_false: files('tdx-stub.c'))
>  
>  i386_system_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
>  
> diff --git a/target/i386/kvm/tdx-stub.c b/target/i386/kvm/tdx-stub.c
> new file mode 100644
> index 000000000000..b614b46d3f4a
> --- /dev/null
> +++ b/target/i386/kvm/tdx-stub.c
> @@ -0,0 +1,8 @@
> +#include "qemu/osdep.h"
> +
> +#include "tdx.h"
> +
> +int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
> +{
> +    return -EINVAL;
> +}
> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> index ff3ef9bd8657..1b7894e43c6f 100644
> --- a/target/i386/kvm/tdx.c
> +++ b/target/i386/kvm/tdx.c
> @@ -137,6 +137,91 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg)
>      return KVM_X86_TDX_VM;
>  }
>  
> +static int setup_td_xfam(X86CPU *x86cpu, Error **errp)
> +{
> +    CPUX86State *env = &x86cpu->env;
> +    uint64_t xfam;
> +
> +    xfam = env->features[FEAT_XSAVE_XCR0_LO] |
> +           env->features[FEAT_XSAVE_XCR0_HI] |
> +           env->features[FEAT_XSAVE_XSS_LO] |
> +           env->features[FEAT_XSAVE_XSS_HI];
> +
> +    if (xfam & ~tdx_caps->supported_xfam) {
> +        error_setg(errp, "Invalid XFAM 0x%lx for TDX VM (supported: 0x%llx))",
> +                   xfam, tdx_caps->supported_xfam);
> +        return -1;
> +    }
> +
> +    tdx_guest->xfam = xfam;
> +    return 0;
> +}
> +
> +static void tdx_filter_cpuid(struct kvm_cpuid2 *cpuids)
> +{
> +    int i, dest_cnt = 0;
> +    struct kvm_cpuid_entry2 *src, *dest, *conf;
> +
> +    for (i = 0; i < cpuids->nent; i++) {
> +        src = cpuids->entries + i;
> +        conf = cpuid_find_entry(&tdx_caps->cpuid, src->function, src->index);
> +        if (!conf) {
> +            continue;
> +        }
> +        dest = cpuids->entries + dest_cnt;
> +
> +        dest->function = src->function;
> +        dest->index = src->index;
> +        dest->flags = src->flags;
> +        dest->eax = src->eax & conf->eax;
> +        dest->ebx = src->ebx & conf->ebx;
> +        dest->ecx = src->ecx & conf->ecx;
> +        dest->edx = src->edx & conf->edx;
> +
> +        dest_cnt++;
> +    }
> +    cpuids->nent = dest_cnt++;
> +}
> +
> +int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
> +{
> +    X86CPU *x86cpu = X86_CPU(cpu);
> +    CPUX86State *env = &x86cpu->env;
> +    g_autofree struct kvm_tdx_init_vm *init_vm = NULL;
> +    int r = 0;
> +
> +    QEMU_LOCK_GUARD(&tdx_guest->lock);
> +    if (tdx_guest->initialized) {
> +        return r;
> +    }
> +
> +    init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) +
> +                        sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
> +
> +    r = setup_td_xfam(x86cpu, errp);
> +    if (r) {
> +        return r;
> +    }
> +
> +    init_vm->cpuid.nent = kvm_x86_build_cpuid(env, init_vm->cpuid.entries, 0);
> +    tdx_filter_cpuid(&init_vm->cpuid);
> +
> +    init_vm->attributes = tdx_guest->attributes;
> +    init_vm->xfam = tdx_guest->xfam;
> +
> +    do {
> +        r = tdx_vm_ioctl(KVM_TDX_INIT_VM, 0, init_vm);
> +    } while (r == -EAGAIN);

Other calls to tdx_vm_ioctl don't loop on EAGAIN. Is the need to
do this retry specific to only KVM_TDX_INIT_VM, or should we push
the EAGAIN retry logic inside tdx_vm_ioctl_helper so all callers
benefit ?

> +    if (r < 0) {
> +        error_setg_errno(errp, -r, "KVM_TDX_INIT_VM failed");
> +        return r;
> +    }
> +
> +    tdx_guest->initialized = true;
> +
> +    return 0;
> +}

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 11/5/2024 6:34 PM, Daniel P. Berrangé wrote:
> On Tue, Nov 05, 2024 at 01:23:17AM -0500, Xiaoyao Li wrote:
>> Invoke KVM_TDX_INIT in kvm_arch_pre_create_vcpu() that KVM_TDX_INIT
>> configures global TD configurations, e.g. the canonical CPUID config,
>> and must be executed prior to creating vCPUs.
>>
>> Use kvm_x86_arch_cpuid() to setup the CPUID settings for TDX VM.
>>
>> Note, this doesn't address the fact that QEMU may change the CPUID
>> configuration when creating vCPUs, i.e. punts on refactoring QEMU to
>> provide a stable CPUID config prior to kvm_arch_init().
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>> Acked-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> Changes in v6:
>> - setup xfam explicitly to fit with new uapi;
>> - use tdx_caps->cpuid to filter the input of cpuids because now KVM only
>>    allows the leafs that reported via KVM_TDX_GET_CAPABILITIES;
>>
>> Changes in v4:
>> - mark init_vm with g_autofree() and use QEMU_LOCK_GUARD() to eliminate
>>    the goto labels; (Daniel)
>> Changes in v3:
>> - Pass @errp in tdx_pre_create_vcpu() and pass error info to it. (Daniel)
>> ---
>>   accel/kvm/kvm-all.c         |  8 ++++
>>   target/i386/kvm/kvm.c       | 15 +++++--
>>   target/i386/kvm/kvm_i386.h  |  3 ++
>>   target/i386/kvm/meson.build |  2 +-
>>   target/i386/kvm/tdx-stub.c  |  8 ++++
>>   target/i386/kvm/tdx.c       | 87 +++++++++++++++++++++++++++++++++++++
>>   target/i386/kvm/tdx.h       |  6 +++
>>   7 files changed, 125 insertions(+), 4 deletions(-)
>>   create mode 100644 target/i386/kvm/tdx-stub.c
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 1732fa1adecd..4a1c9950894c 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -536,8 +536,15 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>   
>>       trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>   
>> +    /*
>> +     * tdx_pre_create_vcpu() may call cpu_x86_cpuid(). It in turn may call
>> +     * kvm_vm_ioctl(). Set cpu->kvm_state in advance to avoid NULL pointer
>> +     * dereference.
>> +     */
>> +    cpu->kvm_state = s;
>>       ret = kvm_arch_pre_create_vcpu(cpu, errp);
>>       if (ret < 0) {
>> +        cpu->kvm_state = NULL;
>>           goto err;
>>       }
>>   
>> @@ -546,6 +553,7 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>           error_setg_errno(errp, -ret,
>>                            "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
>>                            kvm_arch_vcpu_id(cpu));
>> +        cpu->kvm_state = NULL;
>>           goto err;
>>       }
>>   
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index afbf67a7fdaa..db676c1336ab 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -38,6 +38,7 @@
>>   #include "kvm_i386.h"
>>   #include "../confidential-guest.h"
>>   #include "sev.h"
>> +#include "tdx.h"
>>   #include "xen-emu.h"
>>   #include "hyperv.h"
>>   #include "hyperv-proto.h"
>> @@ -1824,9 +1825,8 @@ static void kvm_init_nested_state(CPUX86State *env)
>>       }
>>   }
>>   
>> -static uint32_t kvm_x86_build_cpuid(CPUX86State *env,
>> -                                    struct kvm_cpuid_entry2 *entries,
>> -                                    uint32_t cpuid_i)
>> +uint32_t kvm_x86_build_cpuid(CPUX86State *env, struct kvm_cpuid_entry2 *entries,
>> +                             uint32_t cpuid_i)
>>   {
>>       uint32_t limit, i, j;
>>       uint32_t unused;
>> @@ -2358,6 +2358,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>       return r;
>>   }
>>   
>> +int kvm_arch_pre_create_vcpu(CPUState *cpu, Error **errp)
>> +{
>> +    if (is_tdx_vm()) {
>> +        return tdx_pre_create_vcpu(cpu, errp);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   int kvm_arch_destroy_vcpu(CPUState *cs)
>>   {
>>       X86CPU *cpu = X86_CPU(cs);
>> diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
>> index efb0883bd968..b1baf9e7f910 100644
>> --- a/target/i386/kvm/kvm_i386.h
>> +++ b/target/i386/kvm/kvm_i386.h
>> @@ -24,6 +24,9 @@
>>   #define kvm_ioapic_in_kernel() \
>>       (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split())
>>   
>> +uint32_t kvm_x86_build_cpuid(CPUX86State *env, struct kvm_cpuid_entry2 *entries,
>> +                             uint32_t cpuid_i);
>> +
>>   #else
>>   
>>   #define kvm_pit_in_kernel()      0
>> diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
>> index 466bccb9cb17..3f44cdedb758 100644
>> --- a/target/i386/kvm/meson.build
>> +++ b/target/i386/kvm/meson.build
>> @@ -8,7 +8,7 @@ i386_kvm_ss.add(files(
>>   
>>   i386_kvm_ss.add(when: 'CONFIG_XEN_EMU', if_true: files('xen-emu.c'))
>>   
>> -i386_kvm_ss.add(when: 'CONFIG_TDX', if_true: files('tdx.c'))
>> +i386_kvm_ss.add(when: 'CONFIG_TDX', if_true: files('tdx.c'), if_false: files('tdx-stub.c'))
>>   
>>   i386_system_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
>>   
>> diff --git a/target/i386/kvm/tdx-stub.c b/target/i386/kvm/tdx-stub.c
>> new file mode 100644
>> index 000000000000..b614b46d3f4a
>> --- /dev/null
>> +++ b/target/i386/kvm/tdx-stub.c
>> @@ -0,0 +1,8 @@
>> +#include "qemu/osdep.h"
>> +
>> +#include "tdx.h"
>> +
>> +int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
>> +{
>> +    return -EINVAL;
>> +}
>> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
>> index ff3ef9bd8657..1b7894e43c6f 100644
>> --- a/target/i386/kvm/tdx.c
>> +++ b/target/i386/kvm/tdx.c
>> @@ -137,6 +137,91 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg)
>>       return KVM_X86_TDX_VM;
>>   }
>>   
>> +static int setup_td_xfam(X86CPU *x86cpu, Error **errp)
>> +{
>> +    CPUX86State *env = &x86cpu->env;
>> +    uint64_t xfam;
>> +
>> +    xfam = env->features[FEAT_XSAVE_XCR0_LO] |
>> +           env->features[FEAT_XSAVE_XCR0_HI] |
>> +           env->features[FEAT_XSAVE_XSS_LO] |
>> +           env->features[FEAT_XSAVE_XSS_HI];
>> +
>> +    if (xfam & ~tdx_caps->supported_xfam) {
>> +        error_setg(errp, "Invalid XFAM 0x%lx for TDX VM (supported: 0x%llx))",
>> +                   xfam, tdx_caps->supported_xfam);
>> +        return -1;
>> +    }
>> +
>> +    tdx_guest->xfam = xfam;
>> +    return 0;
>> +}
>> +
>> +static void tdx_filter_cpuid(struct kvm_cpuid2 *cpuids)
>> +{
>> +    int i, dest_cnt = 0;
>> +    struct kvm_cpuid_entry2 *src, *dest, *conf;
>> +
>> +    for (i = 0; i < cpuids->nent; i++) {
>> +        src = cpuids->entries + i;
>> +        conf = cpuid_find_entry(&tdx_caps->cpuid, src->function, src->index);
>> +        if (!conf) {
>> +            continue;
>> +        }
>> +        dest = cpuids->entries + dest_cnt;
>> +
>> +        dest->function = src->function;
>> +        dest->index = src->index;
>> +        dest->flags = src->flags;
>> +        dest->eax = src->eax & conf->eax;
>> +        dest->ebx = src->ebx & conf->ebx;
>> +        dest->ecx = src->ecx & conf->ecx;
>> +        dest->edx = src->edx & conf->edx;
>> +
>> +        dest_cnt++;
>> +    }
>> +    cpuids->nent = dest_cnt++;
>> +}
>> +
>> +int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
>> +{
>> +    X86CPU *x86cpu = X86_CPU(cpu);
>> +    CPUX86State *env = &x86cpu->env;
>> +    g_autofree struct kvm_tdx_init_vm *init_vm = NULL;
>> +    int r = 0;
>> +
>> +    QEMU_LOCK_GUARD(&tdx_guest->lock);
>> +    if (tdx_guest->initialized) {
>> +        return r;
>> +    }
>> +
>> +    init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) +
>> +                        sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
>> +
>> +    r = setup_td_xfam(x86cpu, errp);
>> +    if (r) {
>> +        return r;
>> +    }
>> +
>> +    init_vm->cpuid.nent = kvm_x86_build_cpuid(env, init_vm->cpuid.entries, 0);
>> +    tdx_filter_cpuid(&init_vm->cpuid);
>> +
>> +    init_vm->attributes = tdx_guest->attributes;
>> +    init_vm->xfam = tdx_guest->xfam;
>> +
>> +    do {
>> +        r = tdx_vm_ioctl(KVM_TDX_INIT_VM, 0, init_vm);
>> +    } while (r == -EAGAIN);
> 
> Other calls to tdx_vm_ioctl don't loop on EAGAIN. Is the need to
> do this retry specific to only KVM_TDX_INIT_VM, or should we push
> the EAGAIN retry logic inside tdx_vm_ioctl_helper so all callers
> benefit ?

So far, only KVM_TDX_INIT_VM can get -EAGAIN due to KVM side 
TDH_MNG_CREATE gets TDX_RND_NO_ENTROPY because Random number generation 
(e.g., RDRAND or RDSEED) failed and in this case it should retry.

I think adding a commment to explain why it can get -EAGAIN and needs to 
retry should suffice?

>> +    if (r < 0) {
>> +        error_setg_errno(errp, -r, "KVM_TDX_INIT_VM failed");
>> +        return r;
>> +    }
>> +
>> +    tdx_guest->initialized = true;
>> +
>> +    return 0;
>> +}
> 
> With regards,
> Daniel

On Tue, Nov 05, 2024 at 07:51:53PM +0800, Xiaoyao Li wrote:
> On 11/5/2024 6:34 PM, Daniel P. Berrangé wrote:
> > On Tue, Nov 05, 2024 at 01:23:17AM -0500, Xiaoyao Li wrote:
> > > Invoke KVM_TDX_INIT in kvm_arch_pre_create_vcpu() that KVM_TDX_INIT
> > > configures global TD configurations, e.g. the canonical CPUID config,
> > > and must be executed prior to creating vCPUs.
> > > 
> > > Use kvm_x86_arch_cpuid() to setup the CPUID settings for TDX VM.
> > > 
> > > Note, this doesn't address the fact that QEMU may change the CPUID
> > > configuration when creating vCPUs, i.e. punts on refactoring QEMU to
> > > provide a stable CPUID config prior to kvm_arch_init().
> > > 
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> > > Acked-by: Markus Armbruster <armbru@redhat.com>
> > > ---
> > > Changes in v6:
> > > - setup xfam explicitly to fit with new uapi;
> > > - use tdx_caps->cpuid to filter the input of cpuids because now KVM only
> > >    allows the leafs that reported via KVM_TDX_GET_CAPABILITIES;
> > > 
> > > Changes in v4:
> > > - mark init_vm with g_autofree() and use QEMU_LOCK_GUARD() to eliminate
> > >    the goto labels; (Daniel)
> > > Changes in v3:
> > > - Pass @errp in tdx_pre_create_vcpu() and pass error info to it. (Daniel)
> > > ---
> > >   accel/kvm/kvm-all.c         |  8 ++++
> > >   target/i386/kvm/kvm.c       | 15 +++++--
> > >   target/i386/kvm/kvm_i386.h  |  3 ++
> > >   target/i386/kvm/meson.build |  2 +-
> > >   target/i386/kvm/tdx-stub.c  |  8 ++++
> > >   target/i386/kvm/tdx.c       | 87 +++++++++++++++++++++++++++++++++++++
> > >   target/i386/kvm/tdx.h       |  6 +++
> > >   7 files changed, 125 insertions(+), 4 deletions(-)
> > >   create mode 100644 target/i386/kvm/tdx-stub.c

> > > +int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
> > > +{
> > > +    X86CPU *x86cpu = X86_CPU(cpu);
> > > +    CPUX86State *env = &x86cpu->env;
> > > +    g_autofree struct kvm_tdx_init_vm *init_vm = NULL;
> > > +    int r = 0;
> > > +
> > > +    QEMU_LOCK_GUARD(&tdx_guest->lock);
> > > +    if (tdx_guest->initialized) {
> > > +        return r;
> > > +    }
> > > +
> > > +    init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) +
> > > +                        sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
> > > +
> > > +    r = setup_td_xfam(x86cpu, errp);
> > > +    if (r) {
> > > +        return r;
> > > +    }
> > > +
> > > +    init_vm->cpuid.nent = kvm_x86_build_cpuid(env, init_vm->cpuid.entries, 0);
> > > +    tdx_filter_cpuid(&init_vm->cpuid);
> > > +
> > > +    init_vm->attributes = tdx_guest->attributes;
> > > +    init_vm->xfam = tdx_guest->xfam;
> > > +
> > > +    do {
> > > +        r = tdx_vm_ioctl(KVM_TDX_INIT_VM, 0, init_vm);
> > > +    } while (r == -EAGAIN);
> > 
> > Other calls to tdx_vm_ioctl don't loop on EAGAIN. Is the need to
> > do this retry specific to only KVM_TDX_INIT_VM, or should we push
> > the EAGAIN retry logic inside tdx_vm_ioctl_helper so all callers
> > benefit ?
> 
> So far, only KVM_TDX_INIT_VM can get -EAGAIN due to KVM side TDH_MNG_CREATE
> gets TDX_RND_NO_ENTROPY because Random number generation (e.g., RDRAND or
> RDSEED) failed and in this case it should retry.

Ok, no problem.

> I think adding a commment to explain why it can get -EAGAIN and needs to
> retry should suffice?

Sure, a comment is useful.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|