Patchwork [05/12] target-i386: push hot-plugged VCPU state to KVM and unstop it

login
register
mail settings
Submitter Igor Mammedov
Date March 27, 2013, 1:27 p.m.
Message ID <20130327142752.416c3828@nial.usersys.redhat.com>
Download mbox | patch
Permalink /patch/231687/
State New
Headers show

Comments

Igor Mammedov - March 27, 2013, 1:27 p.m.
On Wed, 27 Mar 2013 13:17:45 +0100
Andreas Färber <afaerber@suse.de> wrote:
> > Then I should move following parts to superclass:
> > 
> >     if (dev->hotplugged) {
> >         cpu_synchronize_post_init(env);
> >         resume_vcpu(CPU(cpu));
> >     }
> > 
> > because in case of KVM we should make sure that CPU in sane state before
> > allowing CPU to become run-able.
> 
> That's not possible until we change cpu_synchronize_post_init() argument
> to CPUState, which is somewhere down my TODO list. Currently I have
> mostly flushed my refactorings out, so if you wanted to dive into that,
> that would be appreciated. :)
> 
> Andreas

Would something like this be acceptable?
---
From 15502168009b7b5ae2b46d854c461e5ad031cdc6 Mon Sep 17 00:00:00 2001
From: Igor Mammedov <imammedo@redhat.com>
Date: Wed, 27 Mar 2013 14:21:20 +0100
Subject: [PATCH] cpu: Pass CPUState to *cpu_synchronize_post*()

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 cpus.c               |  4 ++--
 include/sysemu/kvm.h | 12 ++++++------
 kvm-all.c            |  8 ++------
 kvm-stub.c           |  4 ++--
 4 files changed, 12 insertions(+), 16 deletions(-)
Andreas Färber - March 27, 2013, 2:30 p.m.
Am 27.03.2013 14:27, schrieb Igor Mammedov:
> On Wed, 27 Mar 2013 13:17:45 +0100
> Andreas Färber <afaerber@suse.de> wrote:
>>> Then I should move following parts to superclass:
>>>
>>>     if (dev->hotplugged) {
>>>         cpu_synchronize_post_init(env);
>>>         resume_vcpu(CPU(cpu));
>>>     }
>>>
>>> because in case of KVM we should make sure that CPU in sane state before
>>> allowing CPU to become run-able.
>>
>> That's not possible until we change cpu_synchronize_post_init() argument
>> to CPUState, which is somewhere down my TODO list. Currently I have
>> mostly flushed my refactorings out, so if you wanted to dive into that,
>> that would be appreciated. :)
> 
> Would something like this be acceptable?
> ---
> From 15502168009b7b5ae2b46d854c461e5ad031cdc6 Mon Sep 17 00:00:00 2001
> From: Igor Mammedov <imammedo@redhat.com>
> Date: Wed, 27 Mar 2013 14:21:20 +0100
> Subject: [PATCH] cpu: Pass CPUState to *cpu_synchronize_post*()
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  cpus.c               |  4 ++--
>  include/sysemu/kvm.h | 12 ++++++------
>  kvm-all.c            |  8 ++------
>  kvm-stub.c           |  4 ++--
>  4 files changed, 12 insertions(+), 16 deletions(-)

Looks good so far, did you grep for further occurrences?

Andreas
Igor Mammedov - March 27, 2013, 3:16 p.m.
On Wed, 27 Mar 2013 15:30:30 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 27.03.2013 14:27, schrieb Igor Mammedov:
> > On Wed, 27 Mar 2013 13:17:45 +0100
> > Andreas Färber <afaerber@suse.de> wrote:
> >>> Then I should move following parts to superclass:
> >>>
> >>>     if (dev->hotplugged) {
> >>>         cpu_synchronize_post_init(env);
> >>>         resume_vcpu(CPU(cpu));
> >>>     }
> >>>
> >>> because in case of KVM we should make sure that CPU in sane state before
> >>> allowing CPU to become run-able.
> >>
> >> That's not possible until we change cpu_synchronize_post_init() argument
> >> to CPUState, which is somewhere down my TODO list. Currently I have
> >> mostly flushed my refactorings out, so if you wanted to dive into that,
> >> that would be appreciated. :)
> > 
> > Would something like this be acceptable?
> > ---
> > From 15502168009b7b5ae2b46d854c461e5ad031cdc6 Mon Sep 17 00:00:00 2001
> > From: Igor Mammedov <imammedo@redhat.com>
> > Date: Wed, 27 Mar 2013 14:21:20 +0100
> > Subject: [PATCH] cpu: Pass CPUState to *cpu_synchronize_post*()
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  cpus.c               |  4 ++--
> >  include/sysemu/kvm.h | 12 ++++++------
> >  kvm-all.c            |  8 ++------
> >  kvm-stub.c           |  4 ++--
> >  4 files changed, 12 insertions(+), 16 deletions(-)
> 
> Looks good so far, did you grep for further occurrences?
yep, I re-factored every *cpu_synchronize_post*() call,

but considering an intention to call cpu_synchronize_post_init() from
qom/cpu.c this patch won't work nice since it will pull with itself
kvm-stub.o to *-user target.

Due to qom/cpu.c is built only once for both softmmu and *-user targets, I
consider to move cpu_synchronize_post_init() & cpu_synchronize_post_reset()
from include/sysemu/kvm.h into include/sysemu/cpus.h with definition moved
into cpus.c + stubs for cpu_synchronize_post_init() &resume_vcpu() in
libqemustub for *-user target.
Adding stubs to libqemustub could be avoided if resume_vcpu() and
cpu_synchronize_post_init() are called from x86_cpu_realizefn()
at the cost of some ifdeffenery in include/sysemu/cpus.h though.

But moving resume_vcpu() & cpu_synchronize_post_init() into qom/cpu.c looks
like good candidate for being reused by other targets. 

Paolo,
  would it be acceptable to add resume_vcpu() & cpu_synchronize_post_init()
  stubs into libqemustub? 


> Andreas
>
Paolo Bonzini - March 27, 2013, 3:20 p.m.
Il 27/03/2013 16:16, Igor Mammedov ha scritto:
> yep, I re-factored every *cpu_synchronize_post*() call,
> 
> but considering an intention to call cpu_synchronize_post_init() from
> qom/cpu.c this patch won't work nice since it will pull with itself
> kvm-stub.o to *-user target.
> 
> Due to qom/cpu.c is built only once for both softmmu and *-user targets, I
> consider to move cpu_synchronize_post_init() & cpu_synchronize_post_reset()
> from include/sysemu/kvm.h into include/sysemu/cpus.h with definition moved
> into cpus.c + stubs for cpu_synchronize_post_init() &resume_vcpu() in
> libqemustub for *-user target.
> Adding stubs to libqemustub could be avoided if resume_vcpu() and
> cpu_synchronize_post_init() are called from x86_cpu_realizefn()
> at the cost of some ifdeffenery in include/sysemu/cpus.h though.
> 
> But moving resume_vcpu() & cpu_synchronize_post_init() into qom/cpu.c looks
> like good candidate for being reused by other targets. 
> 
> Paolo,
>   would it be acceptable to add resume_vcpu() & cpu_synchronize_post_init()
>   stubs into libqemustub? 

Can you instead add all of kvm-stub.c?

Paolo
Igor Mammedov - March 27, 2013, 7:46 p.m.
On Wed, 27 Mar 2013 16:20:15 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 27/03/2013 16:16, Igor Mammedov ha scritto:
> > yep, I re-factored every *cpu_synchronize_post*() call,
> > 
> > but considering an intention to call cpu_synchronize_post_init() from
> > qom/cpu.c this patch won't work nice since it will pull with itself
> > kvm-stub.o to *-user target.
> > 
> > Due to qom/cpu.c is built only once for both softmmu and *-user targets, I
> > consider to move cpu_synchronize_post_init() & cpu_synchronize_post_reset()
> > from include/sysemu/kvm.h into include/sysemu/cpus.h with definition moved
> > into cpus.c + stubs for cpu_synchronize_post_init() &resume_vcpu() in
> > libqemustub for *-user target.
> > Adding stubs to libqemustub could be avoided if resume_vcpu() and
> > cpu_synchronize_post_init() are called from x86_cpu_realizefn()
> > at the cost of some ifdeffenery in include/sysemu/cpus.h though.
> > 
> > But moving resume_vcpu() & cpu_synchronize_post_init() into qom/cpu.c looks
> > like good candidate for being reused by other targets. 
> > 
> > Paolo,
> >   would it be acceptable to add resume_vcpu() & cpu_synchronize_post_init()
> >   stubs into libqemustub? 
> 
> Can you instead add all of kvm-stub.c?
> 
> Paolo
> 
It's possible but not all of it, I'll post 3 patches that replace 5/12, linked
to this thread.

Patch

diff --git a/cpus.c b/cpus.c
index e919dd7..9b9a32f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -419,7 +419,7 @@  void cpu_synchronize_all_post_reset(void)
     CPUArchState *cpu;
 
     for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
-        cpu_synchronize_post_reset(cpu);
+        cpu_synchronize_post_reset(ENV_GET_CPU(cpu));
     }
 }
 
@@ -428,7 +428,7 @@  void cpu_synchronize_all_post_init(void)
     CPUArchState *cpu;
 
     for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
-        cpu_synchronize_post_init(cpu);
+        cpu_synchronize_post_init(ENV_GET_CPU(cpu));
     }
 }
 
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index f2d97b5..495e6f8 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -250,8 +250,8 @@  int kvm_check_extension(KVMState *s, unsigned int extension);
 uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
 void kvm_cpu_synchronize_state(CPUArchState *env);
-void kvm_cpu_synchronize_post_reset(CPUArchState *env);
-void kvm_cpu_synchronize_post_init(CPUArchState *env);
+void kvm_cpu_synchronize_post_reset(CPUState *cpu);
+void kvm_cpu_synchronize_post_init(CPUState *cpu);
 
 /* generic hooks - to be moved/refactored once there are more users */
 
@@ -262,17 +262,17 @@  static inline void cpu_synchronize_state(CPUArchState *env)
     }
 }
 
-static inline void cpu_synchronize_post_reset(CPUArchState *env)
+static inline void cpu_synchronize_post_reset(CPUState *cpu)
 {
     if (kvm_enabled()) {
-        kvm_cpu_synchronize_post_reset(env);
+        kvm_cpu_synchronize_post_reset(cpu);
     }
 }
 
-static inline void cpu_synchronize_post_init(CPUArchState *env)
+static inline void cpu_synchronize_post_init(CPUState *cpu)
 {
     if (kvm_enabled()) {
-        kvm_cpu_synchronize_post_init(env);
+        kvm_cpu_synchronize_post_init(cpu);
     }
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 9b433d3..fc4e17c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1510,18 +1510,14 @@  void kvm_cpu_synchronize_state(CPUArchState *env)
     }
 }
 
-void kvm_cpu_synchronize_post_reset(CPUArchState *env)
+void kvm_cpu_synchronize_post_reset(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env);
-
     kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE);
     cpu->kvm_vcpu_dirty = false;
 }
 
-void kvm_cpu_synchronize_post_init(CPUArchState *env)
+void kvm_cpu_synchronize_post_init(CPUState *cpu)
 {
-    CPUState *cpu = ENV_GET_CPU(env);
-
     kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
     cpu->kvm_vcpu_dirty = false;
 }
diff --git a/kvm-stub.c b/kvm-stub.c
index 760aadc..82875dd 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -42,11 +42,11 @@  void kvm_cpu_synchronize_state(CPUArchState *env)
 {
 }
 
-void kvm_cpu_synchronize_post_reset(CPUArchState *env)
+void kvm_cpu_synchronize_post_reset(CPUState *env)
 {
 }
 
-void kvm_cpu_synchronize_post_init(CPUArchState *env)
+void kvm_cpu_synchronize_post_init(CPUState *cpu)
 {
 }