| Message ID | 20130327142752.416c3828@nial.usersys.redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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.
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) { }