Re: [PATCH v3 26/70] i386/tdx: Initialize TDX before creating TD vcpus

[Xiaoyao Li <xiaoyao.li@xxxxxxxxx>]

On 11/15/2023 7:01 PM, Daniel P. Berrangé wrote:
> On Wed, Nov 15, 2023 at 02:14:35AM -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@xxxxxxxxx>
> > Acked-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - Pass @errp in tdx_pre_create_vcpu() and pass error info to it. (Daniel)
> > ---
> >   accel/kvm/kvm-all.c        |  9 +++++++-
> >   target/i386/kvm/kvm.c      |  9 ++++++++
> >   target/i386/kvm/tdx-stub.c |  5 +++++
> >   target/i386/kvm/tdx.c      | 45 ++++++++++++++++++++++++++++++++++++++
> >   target/i386/kvm/tdx.h      |  4 ++++
> >   5 files changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 6b5f4d62f961..a92fff471b58 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -441,8 +441,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;
> >       }
> >   
> > @@ -450,11 +457,11 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
> >       if (ret < 0) {
> >           error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)",
> >                            kvm_arch_vcpu_id(cpu));
> > +        cpu->kvm_state = NULL;
> >           goto err;
> >       }
> >   
> >       cpu->kvm_fd = ret;
> > -    cpu->kvm_state = s;
> >       cpu->vcpu_dirty = true;
> >       cpu->dirty_pages = 0;
> >       cpu->throttle_us_per_full = 0;
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index dafe4d262977..fc840653ceb6 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -2268,6 +2268,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/tdx-stub.c b/target/i386/kvm/tdx-stub.c
> > index 1d866d5496bf..3877d432a397 100644
> > --- a/target/i386/kvm/tdx-stub.c
> > +++ b/target/i386/kvm/tdx-stub.c
> > @@ -6,3 +6,8 @@ int tdx_kvm_init(MachineState *ms, Error **errp)
> >   {
> >       return -EINVAL;
> >   }
> > +
> > +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 1f5d8117d1a9..122a37c93de3 100644
> > --- a/target/i386/kvm/tdx.c
> > +++ b/target/i386/kvm/tdx.c
> > @@ -467,6 +467,49 @@ int tdx_kvm_init(MachineState *ms, Error **errp)
> >       return 0;
> >   }
> >   
> > +int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    X86CPU *x86cpu = X86_CPU(cpu);
> > +    CPUX86State *env = &x86cpu->env;
> > +    struct kvm_tdx_init_vm *init_vm;
>
> Mark this as auto-free to avoid the g_free() requirement
>
>    g_autofree  struct kvm_tdx_init_vm *init_vm = NULL;
>
> > +    int r = 0;
> > +
> > +    qemu_mutex_lock(&tdx_guest->lock);
>
>     QEMU_LOCK_GUARD(&tdx_guest->lock);
>
> to eliminate the mutex_unlock requirement, thus eliminating all
> 'goto' jumps and label targets, in favour of a plain 'return -1'
> everywhere.

Learned!

thanks!