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

Xiaoyao Li
2023-12-04T08:28:42Z

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@intel.com>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>> 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!