Re: [PATCH v2 13/58] kvm: Introduce kvm_arch_pre_create_vcpu()
On Wed, Aug 30, 2023 at 09:45:58AM +0800,
Xiaoyao Li 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
> > > Acked-by: Gerd Hoffmann
> > > ---
> > > 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().
--
Isaku Yamahata
Re: [PATCH v2 13/58] kvm: Introduce kvm_arch_pre_create_vcpu()
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
Acked-by: Gerd Hoffmann
---
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);
Re: [PATCH v2 13/58] kvm: Introduce kvm_arch_pre_create_vcpu()
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
Acked-by: Gerd Hoffmann
---
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);
Re: [PATCH v2 13/58] kvm: Introduce kvm_arch_pre_create_vcpu()
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
> Acked-by: Gerd Hoffmann
> ---
> 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
--
|: 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 :|
[PATCH v2 13/58] kvm: Introduce kvm_arch_pre_create_vcpu()
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
Acked-by: Gerd Hoffmann
---
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__);
+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
