| Message ID | 20230818095041.1973309-14-xiaoyao.li@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | TDX QEMU support | expand |
On Fri, Aug 18, 2023 at 05:49:56AM -0400, Xiaoyao Li wrote: > Introduce kvm_arch_pre_create_vcpu(), to perform arch-dependent > work prior to create any vcpu. This is for i386 TDX because it needs > call TDX_INIT_VM before creating any vcpu. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > --- > accel/kvm/kvm-all.c | 12 ++++++++++++ > include/sysemu/kvm.h | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index c9f3aab5e587..5071af917ae0 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -422,6 +422,11 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) > return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id); > } > > +int __attribute__ ((weak)) kvm_arch_pre_create_vcpu(CPUState *cpu) > +{ > + return 0; > +} > + > int kvm_init_vcpu(CPUState *cpu, Error **errp) > { > KVMState *s = kvm_state; > @@ -430,6 +435,13 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp) > > trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu)); > > + ret = kvm_arch_pre_create_vcpu(cpu); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "%s: kvm_arch_pre_create_vcpu() failed", > + __func__); Don't report generic error messages here, when kvm_arch_pre_create_vcpu can provide a better error - pass the 'errp' into the kvm_arch_pre_create_vcpu method. > + goto err; > + } > + > ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu)); > if (ret < 0) { > error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)", > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index 49c896d8a512..d89ec87072d7 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -371,6 +371,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level); > > int kvm_arch_init(MachineState *ms, KVMState *s); > > +int kvm_arch_pre_create_vcpu(CPUState *cpu); > int kvm_arch_init_vcpu(CPUState *cpu); > int kvm_arch_destroy_vcpu(CPUState *cpu); > > -- > 2.34.1 > With regards, Daniel
On 18/8/23 11:49, Xiaoyao Li wrote: > Introduce kvm_arch_pre_create_vcpu(), to perform arch-dependent > work prior to create any vcpu. This is for i386 TDX because it needs > call TDX_INIT_VM before creating any vcpu. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > --- > accel/kvm/kvm-all.c | 12 ++++++++++++ > include/sysemu/kvm.h | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index c9f3aab5e587..5071af917ae0 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -422,6 +422,11 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) > return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id); > } > > +int __attribute__ ((weak)) kvm_arch_pre_create_vcpu(CPUState *cpu) > +{ > + return 0; > +} kvm_arch_init_vcpu() is implemented for each arch. Why not use the same approach here? > int kvm_init_vcpu(CPUState *cpu, Error **errp) > { > KVMState *s = kvm_state; > @@ -430,6 +435,13 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp) > > trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu)); > > + ret = kvm_arch_pre_create_vcpu(cpu); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "%s: kvm_arch_pre_create_vcpu() failed", > + __func__); > + goto err; > + } > + > ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu)); > if (ret < 0) { > error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)", > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index 49c896d8a512..d89ec87072d7 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -371,6 +371,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level); > > int kvm_arch_init(MachineState *ms, KVMState *s); > > +int kvm_arch_pre_create_vcpu(CPUState *cpu); > int kvm_arch_init_vcpu(CPUState *cpu); > int kvm_arch_destroy_vcpu(CPUState *cpu); >
On 8/29/2023 10:40 PM, Philippe Mathieu-Daudé wrote: > On 18/8/23 11:49, Xiaoyao Li wrote: >> Introduce kvm_arch_pre_create_vcpu(), to perform arch-dependent >> work prior to create any vcpu. This is for i386 TDX because it needs >> call TDX_INIT_VM before creating any vcpu. >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> Acked-by: Gerd Hoffmann <kraxel@redhat.com> >> --- >> accel/kvm/kvm-all.c | 12 ++++++++++++ >> include/sysemu/kvm.h | 1 + >> 2 files changed, 13 insertions(+) >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index c9f3aab5e587..5071af917ae0 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -422,6 +422,11 @@ static int kvm_get_vcpu(KVMState *s, unsigned >> long vcpu_id) >> return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id); >> } >> +int __attribute__ ((weak)) kvm_arch_pre_create_vcpu(CPUState *cpu) >> +{ >> + return 0; >> +} > > kvm_arch_init_vcpu() is implemented for each arch. Why not use the > same approach here? Because only x86 needs it currently, for TDX. Other arches don't require an implementation. If don't provide the _weak_ function, it needs to implement the empty function (justing return 0) in all the other arches just as the placeholder. If QEMU community prefers this approach, I can change to it in next version. >> int kvm_init_vcpu(CPUState *cpu, Error **errp) >> { >> KVMState *s = kvm_state; >> @@ -430,6 +435,13 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp) >> trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu)); >> + ret = kvm_arch_pre_create_vcpu(cpu); >> + if (ret < 0) { >> + error_setg_errno(errp, -ret, "%s: kvm_arch_pre_create_vcpu() >> failed", >> + __func__); >> + goto err; >> + } >> + >> ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu)); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu >> failed (%lu)", >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h >> index 49c896d8a512..d89ec87072d7 100644 >> --- a/include/sysemu/kvm.h >> +++ b/include/sysemu/kvm.h >> @@ -371,6 +371,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level); >> int kvm_arch_init(MachineState *ms, KVMState *s); >> +int kvm_arch_pre_create_vcpu(CPUState *cpu); >> int kvm_arch_init_vcpu(CPUState *cpu); >> int kvm_arch_destroy_vcpu(CPUState *cpu); >
On Wed, Aug 30, 2023 at 09:45:58AM +0800, Xiaoyao Li <xiaoyao.li@intel.com> wrote: > On 8/29/2023 10:40 PM, Philippe Mathieu-Daudé wrote: > > On 18/8/23 11:49, Xiaoyao Li wrote: > > > Introduce kvm_arch_pre_create_vcpu(), to perform arch-dependent > > > work prior to create any vcpu. This is for i386 TDX because it needs > > > call TDX_INIT_VM before creating any vcpu. > > > > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > > --- > > > accel/kvm/kvm-all.c | 12 ++++++++++++ > > > include/sysemu/kvm.h | 1 + > > > 2 files changed, 13 insertions(+) > > > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > > index c9f3aab5e587..5071af917ae0 100644 > > > --- a/accel/kvm/kvm-all.c > > > +++ b/accel/kvm/kvm-all.c > > > @@ -422,6 +422,11 @@ static int kvm_get_vcpu(KVMState *s, unsigned > > > long vcpu_id) > > > return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id); > > > } > > > +int __attribute__ ((weak)) kvm_arch_pre_create_vcpu(CPUState *cpu) > > > +{ > > > + return 0; > > > +} > > > > kvm_arch_init_vcpu() is implemented for each arch. Why not use the > > same approach here? > > Because only x86 needs it currently, for TDX. Other arches don't require an > implementation. > > If don't provide the _weak_ function, it needs to implement the empty > function (justing return 0) in all the other arches just as the placeholder. > If QEMU community prefers this approach, I can change to it in next version. Alternative is to move the hook to x86 specific function, not common kvm function. With my quick grepping, x86_cpus_init() or x86_cpu_realizefn().
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index c9f3aab5e587..5071af917ae0 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -422,6 +422,11 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id); } +int __attribute__ ((weak)) kvm_arch_pre_create_vcpu(CPUState *cpu) +{ + return 0; +} + int kvm_init_vcpu(CPUState *cpu, Error **errp) { KVMState *s = kvm_state; @@ -430,6 +435,13 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp) trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu)); + ret = kvm_arch_pre_create_vcpu(cpu); + if (ret < 0) { + error_setg_errno(errp, -ret, "%s: kvm_arch_pre_create_vcpu() failed", + __func__); + goto err; + } + ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu)); if (ret < 0) { error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)", diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index 49c896d8a512..d89ec87072d7 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -371,6 +371,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level); int kvm_arch_init(MachineState *ms, KVMState *s); +int kvm_arch_pre_create_vcpu(CPUState *cpu); int kvm_arch_init_vcpu(CPUState *cpu); int kvm_arch_destroy_vcpu(CPUState *cpu);