Patchwork [uq/master,v3,1/2] kvm: reset state from the CPU's reset method

login
register
mail settings
Submitter Paolo Bonzini
Date April 15, 2013, 12:05 p.m.
Message ID <1366027532-1120-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/236573/
State New
Headers show

Comments

Paolo Bonzini - April 15, 2013, 12:05 p.m.
Now that we have a CPU object with a reset method, it is better to
keep the KVM reset close to the CPU reset.  Using qemu_register_reset
as we do now keeps them far apart.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v2->v3: add stub kvm_arch_reset_vcpu for s390

 include/sysemu/kvm.h   |  2 --
 kvm-all.c              | 11 -----------
 target-arm/kvm.c       |  4 ----
 target-i386/cpu.c      |  5 +++++
 target-i386/kvm_i386.h |  1 +
 target-ppc/kvm.c       |  4 ----
 target-s390x/cpu.c     |  4 ++++
 target-s390x/cpu.h     |  5 +++++
 8 files changed, 15 insertions(+), 21 deletions(-)
Andreas Färber - April 15, 2013, 12:20 p.m.
Am 15.04.2013 14:05, schrieb Paolo Bonzini:
> Now that we have a CPU object with a reset method, it is better to
> keep the KVM reset close to the CPU reset.  Using qemu_register_reset
> as we do now keeps them far apart.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	v2->v3: add stub kvm_arch_reset_vcpu for s390
> 
>  include/sysemu/kvm.h   |  2 --
>  kvm-all.c              | 11 -----------
>  target-arm/kvm.c       |  4 ----
>  target-i386/cpu.c      |  5 +++++
>  target-i386/kvm_i386.h |  1 +
>  target-ppc/kvm.c       |  4 ----
>  target-s390x/cpu.c     |  4 ++++
>  target-s390x/cpu.h     |  5 +++++
>  8 files changed, 15 insertions(+), 21 deletions(-)

Fine with me.

Have you tested whether i386 needs a similar stub on non-x86? If so then

Reviewed-by: Andreas Färber <afaerber@suse.de>

For anyone else wondering, arm and ppc are no longer calling the
kvm_arch_ function, so it can get removed there, while i386 and s390x
call it from X86CPU/S390CPU reset handler now. Would be more clear if
the commit message was more verbose ;) and would be nice to clean this
up as follow-up by renaming to kvm_x86_... and kvm_s390_... or so
respectively.

Andreas
Paolo Bonzini - April 15, 2013, 12:55 p.m.
Il 15/04/2013 14:20, Andreas Färber ha scritto:
> Fine with me.
> 
> Have you tested whether i386 needs a similar stub on non-x86? If so then
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>

It doesn't need it, because the prototype is declared always in kvm_i386.h.

I didn't test non-x86, but I tested an alternative patches that places
s390's kvm_arch_reset_vcpu prototype outside "#ifdef CONFIG_KVM".  That
also compiled fine on non-s390, and it's what will happen when compiling
on non-x86.

I figured that the dummy inline definition was closer in style to the
existing s390 code, so that's what I chose for submission.

Paolo
Gleb Natapov - April 18, 2013, 5:37 a.m.
On Mon, Apr 15, 2013 at 02:05:31PM +0200, Paolo Bonzini wrote:
> Now that we have a CPU object with a reset method, it is better to
> keep the KVM reset close to the CPU reset.  Using qemu_register_reset
> as we do now keeps them far apart.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	v2->v3: add stub kvm_arch_reset_vcpu for s390
> 
>  include/sysemu/kvm.h   |  2 --
>  kvm-all.c              | 11 -----------
>  target-arm/kvm.c       |  4 ----
>  target-i386/cpu.c      |  5 +++++
>  target-i386/kvm_i386.h |  1 +
>  target-ppc/kvm.c       |  4 ----
>  target-s390x/cpu.c     |  4 ++++
>  target-s390x/cpu.h     |  5 +++++
>  8 files changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index f2d97b5..50072c5 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -199,8 +199,6 @@ int kvm_arch_init_vcpu(CPUState *cpu);
>  /* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
>  unsigned long kvm_arch_vcpu_id(CPUState *cpu);
>  
> -void kvm_arch_reset_vcpu(CPUState *cpu);
> -
>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>  int kvm_arch_on_sigbus(int code, void *addr);
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index 9b433d3..57616ef 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -207,13 +207,6 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
>      return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
>  }
>  
> -static void kvm_reset_vcpu(void *opaque)
> -{
> -    CPUState *cpu = opaque;
> -
> -    kvm_arch_reset_vcpu(cpu);
> -}
> -
>  int kvm_init_vcpu(CPUState *cpu)
>  {
>      KVMState *s = kvm_state;
> @@ -253,10 +246,6 @@ int kvm_init_vcpu(CPUState *cpu)
>      }
>  
>      ret = kvm_arch_init_vcpu(cpu);
> -    if (ret == 0) {
> -        qemu_register_reset(kvm_reset_vcpu, cpu);
> -        kvm_arch_reset_vcpu(cpu);
> -    }
>  err:
>      return ret;
>  }
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 6bfb103..f45a63d 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -430,10 +430,6 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>      return 0;
>  }
>  
> -void kvm_arch_reset_vcpu(CPUState *cs)
> -{
> -}
> -
>  bool kvm_arch_stop_on_emulation_error(CPUState *cs)
>  {
>      return true;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 6dd993f..1754225 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -24,6 +24,7 @@
>  #include "cpu.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/cpus.h"
> +#include "kvm_i386.h"
>  #include "topology.h"
>  
>  #include "qemu/option.h"
> @@ -2015,6 +2016,10 @@ static void x86_cpu_reset(CPUState *s)
>      }
>  
>      s->halted = !cpu_is_bsp(cpu);
> +
> +    if (kvm_enabled()) {
> +        kvm_arch_reset_vcpu(s);
> +    }
>  #endif
>  }
>  
> diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
> index 4392ab4..3accc2d 100644
> --- a/target-i386/kvm_i386.h
> +++ b/target-i386/kvm_i386.h
> @@ -14,6 +14,7 @@
>  #include "sysemu/kvm.h"
>  
>  bool kvm_allows_irq0_override(void);
> +void kvm_arch_reset_vcpu(CPUState *cs);
>  
>  int kvm_device_pci_assign(KVMState *s, PCIHostDeviceAddress *dev_addr,
>                            uint32_t flags, uint32_t *dev_id);
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 5e9dddb..1e8b01d 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -425,10 +425,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      return ret;
>  }
>  
> -void kvm_arch_reset_vcpu(CPUState *cpu)
> -{
> -}
> -
>  static void kvm_sw_tlb_put(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 23fe51f..6321384 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -84,6 +84,10 @@ static void s390_cpu_reset(CPUState *s)
>       * after incrementing the cpu counter */
>  #if !defined(CONFIG_USER_ONLY)
>      s->halted = 1;
> +
> +    if (kvm_enabled()) {
> +        kvm_arch_reset_vcpu(s);
> +    }
>  #endif
>      tlb_flush(env, 1);
>  }
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index e351005..cc51de5 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -352,11 +352,16 @@ void s390x_cpu_timer(void *opaque);
>  int s390_virtio_hypercall(CPUS390XState *env);
>  
>  #ifdef CONFIG_KVM
> +void kvm_arch_reset_vcpu(CPUState *cs);
>  void kvm_s390_interrupt(S390CPU *cpu, int type, uint32_t code);
>  void kvm_s390_virtio_irq(S390CPU *cpu, int config_change, uint64_t token);
>  void kvm_s390_interrupt_internal(S390CPU *cpu, int type, uint32_t parm,
>                                   uint64_t parm64, int vm);
>  #else
> +static inline void kvm_arch_reset_vcpu(CPUState *cs)
> +{
> +}
> +
You added stub to s390, but not to x86. It will compile either way with
current enough gcc, but I wonder why the inconsistency?

>  static inline void kvm_s390_interrupt(S390CPU *cpu, int type, uint32_t code)
>  {
>  }
> -- 
> 1.8.1.4
> 

--
			Gleb.
Paolo Bonzini - April 18, 2013, 7:37 a.m.
Il 18/04/2013 07:37, Gleb Natapov ha scritto:
>> > +static inline void kvm_arch_reset_vcpu(CPUState *cs)
>> > +{
>> > +}
>> > +
> You added stub to s390, but not to x86. It will compile either way with
> current enough gcc, but I wonder why the inconsistency?
> 

Because I kept it consistent within the file.  s390 prefers stubs, x86
prefers no stubs.

Paolo

Patch

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index f2d97b5..50072c5 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -199,8 +199,6 @@  int kvm_arch_init_vcpu(CPUState *cpu);
 /* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
 unsigned long kvm_arch_vcpu_id(CPUState *cpu);
 
-void kvm_arch_reset_vcpu(CPUState *cpu);
-
 int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
 int kvm_arch_on_sigbus(int code, void *addr);
 
diff --git a/kvm-all.c b/kvm-all.c
index 9b433d3..57616ef 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -207,13 +207,6 @@  static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
     return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
 }
 
-static void kvm_reset_vcpu(void *opaque)
-{
-    CPUState *cpu = opaque;
-
-    kvm_arch_reset_vcpu(cpu);
-}
-
 int kvm_init_vcpu(CPUState *cpu)
 {
     KVMState *s = kvm_state;
@@ -253,10 +246,6 @@  int kvm_init_vcpu(CPUState *cpu)
     }
 
     ret = kvm_arch_init_vcpu(cpu);
-    if (ret == 0) {
-        qemu_register_reset(kvm_reset_vcpu, cpu);
-        kvm_arch_reset_vcpu(cpu);
-    }
 err:
     return ret;
 }
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 6bfb103..f45a63d 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -430,10 +430,6 @@  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
     return 0;
 }
 
-void kvm_arch_reset_vcpu(CPUState *cs)
-{
-}
-
 bool kvm_arch_stop_on_emulation_error(CPUState *cs)
 {
     return true;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6dd993f..1754225 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -24,6 +24,7 @@ 
 #include "cpu.h"
 #include "sysemu/kvm.h"
 #include "sysemu/cpus.h"
+#include "kvm_i386.h"
 #include "topology.h"
 
 #include "qemu/option.h"
@@ -2015,6 +2016,10 @@  static void x86_cpu_reset(CPUState *s)
     }
 
     s->halted = !cpu_is_bsp(cpu);
+
+    if (kvm_enabled()) {
+        kvm_arch_reset_vcpu(s);
+    }
 #endif
 }
 
diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
index 4392ab4..3accc2d 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -14,6 +14,7 @@ 
 #include "sysemu/kvm.h"
 
 bool kvm_allows_irq0_override(void);
+void kvm_arch_reset_vcpu(CPUState *cs);
 
 int kvm_device_pci_assign(KVMState *s, PCIHostDeviceAddress *dev_addr,
                           uint32_t flags, uint32_t *dev_id);
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 5e9dddb..1e8b01d 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -425,10 +425,6 @@  int kvm_arch_init_vcpu(CPUState *cs)
     return ret;
 }
 
-void kvm_arch_reset_vcpu(CPUState *cpu)
-{
-}
-
 static void kvm_sw_tlb_put(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 23fe51f..6321384 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -84,6 +84,10 @@  static void s390_cpu_reset(CPUState *s)
      * after incrementing the cpu counter */
 #if !defined(CONFIG_USER_ONLY)
     s->halted = 1;
+
+    if (kvm_enabled()) {
+        kvm_arch_reset_vcpu(s);
+    }
 #endif
     tlb_flush(env, 1);
 }
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index e351005..cc51de5 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -352,11 +352,16 @@  void s390x_cpu_timer(void *opaque);
 int s390_virtio_hypercall(CPUS390XState *env);
 
 #ifdef CONFIG_KVM
+void kvm_arch_reset_vcpu(CPUState *cs);
 void kvm_s390_interrupt(S390CPU *cpu, int type, uint32_t code);
 void kvm_s390_virtio_irq(S390CPU *cpu, int config_change, uint64_t token);
 void kvm_s390_interrupt_internal(S390CPU *cpu, int type, uint32_t parm,
                                  uint64_t parm64, int vm);
 #else
+static inline void kvm_arch_reset_vcpu(CPUState *cs)
+{
+}
+
 static inline void kvm_s390_interrupt(S390CPU *cpu, int type, uint32_t code)
 {
 }