[Qemu-devel] s390: Fix cpu shutdown for KVM

[Qemu-devel] s390: Fix cpu shutdown for KVM

[Christian Borntraeger]

qemu/kvm on s390 currently hangs on panic (doesnt exit on disabled
wait) and also shuts down on cpu hot-unplug (SIGP stop).
This patch tries to fix these simple cases.
On s390 a shutdown is the state of all CPUs being either stopped
or disabled (for interrupts) waiting. We have to track this number
to call the shutdown sequence accordingly. This patch implements
the counting and shutdown handling for the kvm path in qemu.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390-virtio.c   |    4 ++++
 target-s390x/kvm.c |   29 +++++++++++++++++++++++++----
 2 files changed, 29 insertions(+), 4 deletions(-)

Index: b/hw/s390-virtio.c
===================================================================
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -130,6 +130,9 @@ int s390_virtio_hypercall(CPUState *env,
     return r;
 }
 
+/* defined in target-s390x/kvm.c */
+extern int s390_running_cpus;
+
 /* PC hardware initialisation */
 static void s390_init(ram_addr_t my_ram_size,
                       const char *boot_device,
@@ -189,6 +192,7 @@ static void s390_init(ram_addr_t my_ram_
 
     env->halted = 0;
     env->exception_index = 0;
+    s390_running_cpus = 1;
 
     if (kernel_filename) {
         kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
Index: b/target-s390x/kvm.c
===================================================================
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -63,6 +63,14 @@
 #define SCLP_CMDW_READ_SCP_INFO         0x00020001
 #define SCLP_CMDW_READ_SCP_INFO_FORCED  0x00120001
 
+/*
+ * The number of running CPUs. On s390 a shutdown is the state of all CPUs
+ * being either stopped or disabled (for interrupts) waiting. We have to
+ * track this number to call the shutdown sequence accordingly. This
+ * number is modified either on startup or while holding the big qemu lock.
+ */
+int s390_running_cpus;
+
 const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
     KVM_CAP_LAST_INFO
 };
@@ -185,6 +193,12 @@ void kvm_s390_interrupt_internal(CPUStat
         return;
     }
 
+    /*
+     * We can only deliver interrupts to (interrupt) enabled CPUs.
+     * We dont modify s390_running_cpus here, since CPUs in enabled wait
+     * will wait inside the kernel (no exit). Therefore, the targeted
+     * CPUs was neither disabled waiting or stopped for qemu.
+     */
     env->halted = 0;
     env->exception_index = -1;
     qemu_cpu_kick(env);
@@ -301,6 +315,7 @@ static int s390_cpu_restart(CPUState *en
     kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
     env->halted = 0;
     env->exception_index = -1;
+    s390_running_cpus++;
     qemu_cpu_kick(env);
     dprintf("DONE: SIGP cpu restart: %p\n", env);
     return 0;
@@ -425,16 +440,24 @@ static int handle_intercept(CPUState *en
             r = handle_instruction(env, run);
             break;
         case ICPT_WAITPSW:
-            /* XXX What to do on system shutdown? */
+            if (--s390_running_cpus == 0) {
+                qemu_system_shutdown_request();
+            }
             env->halted = 1;
             env->exception_index = EXCP_HLT;
+            r = EXCP_HALTED;
             break;
         case ICPT_SOFT_INTERCEPT:
             fprintf(stderr, "KVM unimplemented icpt SOFT\n");
             exit(1);
             break;
         case ICPT_CPU_STOP:
-            qemu_system_shutdown_request();
+            if (--s390_running_cpus == 0) {
+                qemu_system_shutdown_request();
+            }
+            env->halted = 1;
+            env->exception_index = EXCP_HLT;
+            r = EXCP_HALTED;
             break;
         case ICPT_IO:
             fprintf(stderr, "KVM unimplemented icpt IO\n");
@@ -468,8 +491,6 @@ int kvm_arch_handle_exit(CPUState *env, 
 
     if (ret == 0) {
         ret = EXCP_INTERRUPT;
-    } else if (ret > 0) {
-        ret = 0;
     }
     return ret;
 }

Re: [Qemu-devel] s390: Fix cpu shutdown for KVM

[Alexander Graf]

On 10/04/2011 01:28 PM, Christian Borntraeger wrote:
> qemu/kvm on s390 currently hangs on panic (doesnt exit on disabled
> wait) and also shuts down on cpu hot-unplug (SIGP stop).
> This patch tries to fix these simple cases.
> On s390 a shutdown is the state of all CPUs being either stopped
> or disabled (for interrupts) waiting. We have to track this number
> to call the shutdown sequence accordingly. This patch implements
> the counting and shutdown handling for the kvm path in qemu.
>
> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
> ---
>   hw/s390-virtio.c   |    4 ++++
>   target-s390x/kvm.c |   29 +++++++++++++++++++++++++----
>   2 files changed, 29 insertions(+), 4 deletions(-)
>
> Index: b/hw/s390-virtio.c
> ===================================================================
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -130,6 +130,9 @@ int s390_virtio_hypercall(CPUState *env,
>       return r;
>   }
>
> +/* defined in target-s390x/kvm.c */
> +extern int s390_running_cpus;

This breaks non-kvm builds. Please put it into s390-virtio.c and export 
functions that get called from target-s390x/kvm.c which add/remove 
running CPUs from that counter.


Alex

> +
>   /* PC hardware initialisation */
>   static void s390_init(ram_addr_t my_ram_size,
>                         const char *boot_device,
> @@ -189,6 +192,7 @@ static void s390_init(ram_addr_t my_ram_
>
>       env->halted = 0;
>       env->exception_index = 0;
> +    s390_running_cpus = 1;
>
>       if (kernel_filename) {
>           kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
> Index: b/target-s390x/kvm.c
> ===================================================================
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -63,6 +63,14 @@
>   #define SCLP_CMDW_READ_SCP_INFO         0x00020001
>   #define SCLP_CMDW_READ_SCP_INFO_FORCED  0x00120001
>
> +/*
> + * The number of running CPUs. On s390 a shutdown is the state of all CPUs
> + * being either stopped or disabled (for interrupts) waiting. We have to
> + * track this number to call the shutdown sequence accordingly. This
> + * number is modified either on startup or while holding the big qemu lock.
> + */
> +int s390_running_cpus;
> +
>   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>       KVM_CAP_LAST_INFO
>   };
> @@ -185,6 +193,12 @@ void kvm_s390_interrupt_internal(CPUStat
>           return;
>       }
>
> +    /*
> +     * We can only deliver interrupts to (interrupt) enabled CPUs.
> +     * We dont modify s390_running_cpus here, since CPUs in enabled wait
> +     * will wait inside the kernel (no exit). Therefore, the targeted
> +     * CPUs was neither disabled waiting or stopped for qemu.
> +     */
>       env->halted = 0;
>       env->exception_index = -1;
>       qemu_cpu_kick(env);
> @@ -301,6 +315,7 @@ static int s390_cpu_restart(CPUState *en
>       kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
>       env->halted = 0;
>       env->exception_index = -1;
> +    s390_running_cpus++;
>       qemu_cpu_kick(env);
>       dprintf("DONE: SIGP cpu restart: %p\n", env);
>       return 0;
> @@ -425,16 +440,24 @@ static int handle_intercept(CPUState *en
>               r = handle_instruction(env, run);
>               break;
>           case ICPT_WAITPSW:
> -            /* XXX What to do on system shutdown? */
> +            if (--s390_running_cpus == 0) {
> +                qemu_system_shutdown_request();
> +            }
>               env->halted = 1;
>               env->exception_index = EXCP_HLT;
> +            r = EXCP_HALTED;
>               break;
>           case ICPT_SOFT_INTERCEPT:
>               fprintf(stderr, "KVM unimplemented icpt SOFT\n");
>               exit(1);
>               break;
>           case ICPT_CPU_STOP:
> -            qemu_system_shutdown_request();
> +            if (--s390_running_cpus == 0) {
> +                qemu_system_shutdown_request();
> +            }
> +            env->halted = 1;
> +            env->exception_index = EXCP_HLT;
> +            r = EXCP_HALTED;
>               break;
>           case ICPT_IO:
>               fprintf(stderr, "KVM unimplemented icpt IO\n");
> @@ -468,8 +491,6 @@ int kvm_arch_handle_exit(CPUState *env,
>
>       if (ret == 0) {
>           ret = EXCP_INTERRUPT;
> -    } else if (ret>  0) {
> -        ret = 0;
>       }
>       return ret;
>   }

Re: [Qemu-devel] s390: Fix cpu shutdown for KVM

[Christian Borntraeger]

Something like the following? 

s390: Fix cpu shutdown for KVM

On s390 a shutdown is the state of all CPUs being either stopped
or disabled (for interrupts) waiting. We have to track this number
to call the shutdown sequence accordingly. This patch implements
the counting and shutdown handling for the kvm path in qemu.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390-virtio.c   |   21 +++++++++++++++++++++
 target-s390x/cpu.h |   12 ++++++++++++
 target-s390x/kvm.c |   21 +++++++++++++++++----
 3 files changed, 50 insertions(+), 4 deletions(-)

Index: b/hw/s390-virtio.c
===================================================================
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -130,6 +130,26 @@ int s390_virtio_hypercall(CPUState *env,
     return r;
 }
 
+/*
+ * The number of running CPUs. On s390 a shutdown is the state of all CPUs
+ * being either stopped or disabled (for interrupts) waiting. We have to
+ * track this number to call the shutdown sequence accordingly. This
+ * number is modified either on startup or while holding the big qemu lock.
+ */
+static unsigned s390_running_cpus;
+
+void kvm_s390_add_running_cpu(CPUState *env)
+{
+    assert(env->halted == 0);
+    s390_running_cpus++;
+}
+
+unsigned kvm_s390_del_running_cpu(CPUState *env)
+{
+    assert(s390_running_cpus >= 1);
+    return --s390_running_cpus;
+}
+
 /* PC hardware initialisation */
 static void s390_init(ram_addr_t my_ram_size,
                       const char *boot_device,
@@ -189,6 +209,7 @@ static void s390_init(ram_addr_t my_ram_
 
     env->halted = 0;
     env->exception_index = 0;
+    kvm_s390_add_running_cpu(env);
 
     if (kernel_filename) {
         kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
Index: b/target-s390x/cpu.h
===================================================================
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -292,6 +292,8 @@ void kvm_s390_interrupt(CPUState *env, i
 void kvm_s390_virtio_irq(CPUState *env, int config_change, uint64_t token);
 void kvm_s390_interrupt_internal(CPUState *env, int type, uint32_t parm,
                                  uint64_t parm64, int vm);
+void kvm_s390_add_running_cpu(CPUState *env);
+unsigned kvm_s390_del_running_cpu(CPUState *env);
 #else
 static inline void kvm_s390_interrupt(CPUState *env, int type, uint32_t code)
 {
@@ -307,6 +309,16 @@ static inline void kvm_s390_interrupt_in
                                                int vm)
 {
 }
+
+static inline void kvm_s390_add_running_cpu(CPUState *env)
+{
+}
+
+static inline unsigned kvm_s390_del_running_cpu(CPUState *env)
+{
+    return 0;
+}
+
 #endif
 CPUState *s390_cpu_addr2state(uint16_t cpu_addr);
 
Index: b/target-s390x/kvm.c
===================================================================
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -185,6 +185,12 @@ void kvm_s390_interrupt_internal(CPUStat
         return;
     }
 
+    /*
+     * We can only deliver interrupts to (interrupt) enabled CPUs.
+     * We dont call kvm_s390_add_running_cpu here, since CPUs in enabled wait
+     * will wait inside the kernel (no exit). Therefore, the targeted
+     * CPUs was neither disabled waiting or stopped for qemu.
+     */
     env->halted = 0;
     env->exception_index = -1;
     qemu_cpu_kick(env);
@@ -301,6 +307,7 @@ static int s390_cpu_restart(CPUState *en
     kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
     env->halted = 0;
     env->exception_index = -1;
+    kvm_s390_add_running_cpu(env);
     qemu_cpu_kick(env);
     dprintf("DONE: SIGP cpu restart: %p\n", env);
     return 0;
@@ -425,16 +432,24 @@ static int handle_intercept(CPUState *en
             r = handle_instruction(env, run);
             break;
         case ICPT_WAITPSW:
-            /* XXX What to do on system shutdown? */
+            if (kvm_s390_del_running_cpu(env) == 0) {
+                qemu_system_shutdown_request();
+            }
             env->halted = 1;
             env->exception_index = EXCP_HLT;
+            r = EXCP_HALTED;
             break;
         case ICPT_SOFT_INTERCEPT:
             fprintf(stderr, "KVM unimplemented icpt SOFT\n");
             exit(1);
             break;
         case ICPT_CPU_STOP:
-            qemu_system_shutdown_request();
+            if (kvm_s390_del_running_cpu(env) == 0) {
+                qemu_system_shutdown_request();
+            }
+            env->halted = 1;
+            env->exception_index = EXCP_HLT;
+            r = EXCP_HALTED;
             break;
         case ICPT_IO:
             fprintf(stderr, "KVM unimplemented icpt IO\n");
@@ -468,8 +483,6 @@ int kvm_arch_handle_exit(CPUState *env, 
 
     if (ret == 0) {
         ret = EXCP_INTERRUPT;
-    } else if (ret > 0) {
-        ret = 0;
     }
     return ret;
 }

Re: [Qemu-devel] s390: Fix cpu shutdown for KVM

[Peter Maydell]

On 4 October 2011 14:47, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> +     * We can only deliver interrupts to (interrupt) enabled CPUs.
> +     * We dont call kvm_s390_add_running_cpu here, since CPUs in enabled wait
> +     * will wait inside the kernel (no exit). Therefore, the targeted
> +     * CPUs was neither disabled waiting or stopped for qemu.

Grammar nits, since I'm commenting anyway:
 "don't"
 "neither disabled waiting nor stopped"

> @@ -425,16 +432,24 @@ static int handle_intercept(CPUState *en
>             r = handle_instruction(env, run);
>             break;
>         case ICPT_WAITPSW:
> -            /* XXX What to do on system shutdown? */
> +            if (kvm_s390_del_running_cpu(env) == 0) {
> +                qemu_system_shutdown_request();
> +            }
>             env->halted = 1;
>             env->exception_index = EXCP_HLT;
> +            r = EXCP_HALTED;
>             break;
>         case ICPT_SOFT_INTERCEPT:
>             fprintf(stderr, "KVM unimplemented icpt SOFT\n");
>             exit(1);
>             break;
>         case ICPT_CPU_STOP:
> -            qemu_system_shutdown_request();
> +            if (kvm_s390_del_running_cpu(env) == 0) {
> +                qemu_system_shutdown_request();
> +            }
> +            env->halted = 1;
> +            env->exception_index = EXCP_HLT;
> +            r = EXCP_HALTED;
>             break;
>         case ICPT_IO:
>             fprintf(stderr, "KVM unimplemented icpt IO\n");

This makes the ICPT_CPU_STOP and ICPT_WAITPSW cases identical,
right? You should just fold them together.

-- PMM

Re: [Qemu-devel] s390: Fix cpu shutdown for KVM

[Christian Borntraeger]

On 04/10/11 15:56, Peter Maydell wrote:
> On 4 October 2011 14:47, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>> +     * We can only deliver interrupts to (interrupt) enabled CPUs.
>> +     * We dont call kvm_s390_add_running_cpu here, since CPUs in enabled wait
>> +     * will wait inside the kernel (no exit). Therefore, the targeted
>> +     * CPUs was neither disabled waiting or stopped for qemu.
> 
> Grammar nits, since I'm commenting anyway:
>  "don't"
>  "neither disabled waiting nor stopped"

Ok. Alex can you fix that up or do you want a new patch?

> This makes the ICPT_CPU_STOP and ICPT_WAITPSW cases identical,
> right? You should just fold them together.

Yes, at the moment they are identical.
I am still thinking about some additional changes that will make them separate again
due to their usage in Linux:
- disabled wait usually indicates a kernel panic
- stop is called during cpu hot unplug and during shutdown for the last cpu

So on disabled wait we might want to perform extra logging etc, but I dont know yet.
Should I merge them anyway?

Christian

Re: [Qemu-devel] s390: Fix cpu shutdown for KVM

[Alexander Graf]

On 10/04/2011 03:47 PM, Christian Borntraeger wrote:
> Something like the following?
>
> s390: Fix cpu shutdown for KVM
>
> On s390 a shutdown is the state of all CPUs being either stopped
> or disabled (for interrupts) waiting. We have to track this number
> to call the shutdown sequence accordingly. This patch implements
> the counting and shutdown handling for the kvm path in qemu.
>
> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
> ---
>   hw/s390-virtio.c   |   21 +++++++++++++++++++++
>   target-s390x/cpu.h |   12 ++++++++++++
>   target-s390x/kvm.c |   21 +++++++++++++++++----
>   3 files changed, 50 insertions(+), 4 deletions(-)
>
> Index: b/hw/s390-virtio.c
> ===================================================================
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -130,6 +130,26 @@ int s390_virtio_hypercall(CPUState *env,
>       return r;
>   }
>
> +/*
> + * The number of running CPUs. On s390 a shutdown is the state of all CPUs
> + * being either stopped or disabled (for interrupts) waiting. We have to
> + * track this number to call the shutdown sequence accordingly. This
> + * number is modified either on startup or while holding the big qemu lock.
> + */
> +static unsigned s390_running_cpus;
> +
> +void kvm_s390_add_running_cpu(CPUState *env)
> +{
> +    assert(env->halted == 0);
> +    s390_running_cpus++;
> +}
> +
> +unsigned kvm_s390_del_running_cpu(CPUState *env)
> +{
> +    assert(s390_running_cpus>= 1);
> +    return --s390_running_cpus;
> +}

Almost. It has nothing to do with KVM though. Please keep the 
abstraction intact!

> +
>   /* PC hardware initialisation */
>   static void s390_init(ram_addr_t my_ram_size,
>                         const char *boot_device,
> @@ -189,6 +209,7 @@ static void s390_init(ram_addr_t my_ram_
>
>       env->halted = 0;
>       env->exception_index = 0;
> +    kvm_s390_add_running_cpu(env);
>
>       if (kernel_filename) {
>           kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
> Index: b/target-s390x/cpu.h
> ===================================================================
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -292,6 +292,8 @@ void kvm_s390_interrupt(CPUState *env, i
>   void kvm_s390_virtio_irq(CPUState *env, int config_change, uint64_t token);
>   void kvm_s390_interrupt_internal(CPUState *env, int type, uint32_t parm,
>                                    uint64_t parm64, int vm);
> +void kvm_s390_add_running_cpu(CPUState *env);
> +unsigned kvm_s390_del_running_cpu(CPUState *env);
>   #else
>   static inline void kvm_s390_interrupt(CPUState *env, int type, uint32_t code)
>   {
> @@ -307,6 +309,16 @@ static inline void kvm_s390_interrupt_in
>                                                  int vm)
>   {
>   }
> +
> +static inline void kvm_s390_add_running_cpu(CPUState *env)
> +{
> +}
> +
> +static inline unsigned kvm_s390_del_running_cpu(CPUState *env)
> +{
> +    return 0;
> +}

The functions are defined in s390-virtio.c and thus are available 
anywhere now. No need to special-case KVM anywhere.

> +
>   #endif
>   CPUState *s390_cpu_addr2state(uint16_t cpu_addr);
>
> Index: b/target-s390x/kvm.c
> ===================================================================
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -185,6 +185,12 @@ void kvm_s390_interrupt_internal(CPUStat
>           return;
>       }
>
> +    /*
> +     * We can only deliver interrupts to (interrupt) enabled CPUs.
> +     * We dont call kvm_s390_add_running_cpu here, since CPUs in enabled wait
> +     * will wait inside the kernel (no exit). Therefore, the targeted
> +     * CPUs was neither disabled waiting or stopped for qemu.
> +     */
>       env->halted = 0;
>       env->exception_index = -1;
>       qemu_cpu_kick(env);
> @@ -301,6 +307,7 @@ static int s390_cpu_restart(CPUState *en
>       kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
>       env->halted = 0;
>       env->exception_index = -1;
> +    kvm_s390_add_running_cpu(env);

Hrm. Is add_running correct here? Maybe we should add a wrapper around 
the halted = 0 parts in s390-virtio.c and do the increase of the counter 
there? That way we can make sure that transitioning from 0 -> 0 doesn't 
increase the counter.


Alex

[Qemu-devel] [PATCHv3] s390: Fix cpu shutdown for KVM

[Christian Borntraeger]

On s390 a shutdown is the state of all CPUs being either stopped
or disabled (for interrupts) waiting. We have to track the overall
number of running CPUs to call the shutdown sequence accordingly.
This patch implements the counting and shutdown handling for the 
kvm path in qemu.
Lets also wrap changes to env->halted and env->exception_index.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390-virtio.c   |   32 ++++++++++++++++++++++++++++++--
 target-s390x/cpu.h |    2 ++
 target-s390x/kvm.c |   19 +++++++------------
 3 files changed, 39 insertions(+), 14 deletions(-)

Index: b/hw/s390-virtio.c
===================================================================
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -130,6 +130,34 @@ int s390_virtio_hypercall(CPUState *env,
     return r;
 }
 
+/*
+ * The number of running CPUs. On s390 a shutdown is the state of all CPUs
+ * being either stopped or disabled (for interrupts) waiting. We have to
+ * track this number to call the shutdown sequence accordingly. This
+ * number is modified either on startup or while holding the big qemu lock.
+ */
+static unsigned s390_running_cpus;
+
+void s390_add_running_cpu(CPUState *env)
+{
+    if (env->halted) {
+        s390_running_cpus++;
+        env->halted = 0;
+        env->exception_index = -1;
+    }
+}
+
+unsigned s390_del_running_cpu(CPUState *env)
+{
+    if (env->halted == 0) {
+        assert(s390_running_cpus >= 1);
+        s390_running_cpus--;
+        env->halted = 1;
+        env->exception_index = EXCP_HLT;
+    }
+    return s390_running_cpus;
+}
+
 /* PC hardware initialisation */
 static void s390_init(ram_addr_t my_ram_size,
                       const char *boot_device,
@@ -187,8 +215,8 @@ static void s390_init(ram_addr_t my_ram_
         tmp_env->storage_keys = storage_keys;
     }
 
-    env->halted = 0;
-    env->exception_index = 0;
+    /* One CPU has to run */
+    s390_add_running_cpu(env);
 
     if (kernel_filename) {
         kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
Index: b/target-s390x/cpu.h
===================================================================
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -309,6 +309,8 @@ static inline void kvm_s390_interrupt_in
 }
 #endif
 CPUState *s390_cpu_addr2state(uint16_t cpu_addr);
+void s390_add_running_cpu(CPUState *env);
+unsigned s390_del_running_cpu(CPUState *env);
 
 /* from s390-virtio-bus */
 extern const target_phys_addr_t virtio_size;
Index: b/target-s390x/kvm.c
===================================================================
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -185,8 +185,7 @@ void kvm_s390_interrupt_internal(CPUStat
         return;
     }
 
-    env->halted = 0;
-    env->exception_index = -1;
+    s390_add_running_cpu(env);
     qemu_cpu_kick(env);
 
     kvmint.type = type;
@@ -299,8 +298,7 @@ static int handle_diag(CPUState *env, st
 static int s390_cpu_restart(CPUState *env)
 {
     kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
-    env->halted = 0;
-    env->exception_index = -1;
+    s390_add_running_cpu(env);
     qemu_cpu_kick(env);
     dprintf("DONE: SIGP cpu restart: %p\n", env);
     return 0;
@@ -425,17 +423,16 @@ static int handle_intercept(CPUState *en
             r = handle_instruction(env, run);
             break;
         case ICPT_WAITPSW:
-            /* XXX What to do on system shutdown? */
-            env->halted = 1;
-            env->exception_index = EXCP_HLT;
+        case ICPT_CPU_STOP:
+            if (s390_del_running_cpu(env) == 0) {
+                qemu_system_shutdown_request();
+            }
+            r = EXCP_HALTED;
             break;
         case ICPT_SOFT_INTERCEPT:
             fprintf(stderr, "KVM unimplemented icpt SOFT\n");
             exit(1);
             break;
-        case ICPT_CPU_STOP:
-            qemu_system_shutdown_request();
-            break;
         case ICPT_IO:
             fprintf(stderr, "KVM unimplemented icpt IO\n");
             exit(1);
@@ -468,8 +465,6 @@ int kvm_arch_handle_exit(CPUState *env, 
 
     if (ret == 0) {
         ret = EXCP_INTERRUPT;
-    } else if (ret > 0) {
-        ret = 0;
     }
     return ret;
 }

Re: [Qemu-devel] [PATCHv3] s390: Fix cpu shutdown for KVM

[Alexander Graf]

On 10/04/2011 05:20 PM, Christian Borntraeger wrote:
> On s390 a shutdown is the state of all CPUs being either stopped
> or disabled (for interrupts) waiting. We have to track the overall
> number of running CPUs to call the shutdown sequence accordingly.
> This patch implements the counting and shutdown handling for the
> kvm path in qemu.
> Lets also wrap changes to env->halted and env->exception_index.
>
> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
> ---
>   hw/s390-virtio.c   |   32 ++++++++++++++++++++++++++++++--
>   target-s390x/cpu.h |    2 ++
>   target-s390x/kvm.c |   19 +++++++------------
>   3 files changed, 39 insertions(+), 14 deletions(-)
>
> Index: b/hw/s390-virtio.c
> ===================================================================
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -130,6 +130,34 @@ int s390_virtio_hypercall(CPUState *env,
>       return r;
>   }
>
> +/*
> + * The number of running CPUs. On s390 a shutdown is the state of all CPUs
> + * being either stopped or disabled (for interrupts) waiting. We have to
> + * track this number to call the shutdown sequence accordingly. This
> + * number is modified either on startup or while holding the big qemu lock.
> + */
> +static unsigned s390_running_cpus;
> +
> +void s390_add_running_cpu(CPUState *env)
> +{
> +    if (env->halted) {
> +        s390_running_cpus++;
> +        env->halted = 0;
> +        env->exception_index = -1;
> +    }
> +}
> +
> +unsigned s390_del_running_cpu(CPUState *env)
> +{
> +    if (env->halted == 0) {
> +        assert(s390_running_cpus>= 1);
> +        s390_running_cpus--;
> +        env->halted = 1;
> +        env->exception_index = EXCP_HLT;
> +    }
> +    return s390_running_cpus;
> +}
> +
>   /* PC hardware initialisation */
>   static void s390_init(ram_addr_t my_ram_size,
>                         const char *boot_device,
> @@ -187,8 +215,8 @@ static void s390_init(ram_addr_t my_ram_
>           tmp_env->storage_keys = storage_keys;
>       }
>
> -    env->halted = 0;
> -    env->exception_index = 0;
> +    /* One CPU has to run */
> +    s390_add_running_cpu(env);
>
>       if (kernel_filename) {
>           kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
> Index: b/target-s390x/cpu.h
> ===================================================================
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -309,6 +309,8 @@ static inline void kvm_s390_interrupt_in
>   }
>   #endif
>   CPUState *s390_cpu_addr2state(uint16_t cpu_addr);
> +void s390_add_running_cpu(CPUState *env);
> +unsigned s390_del_running_cpu(CPUState *env);
>
>   /* from s390-virtio-bus */
>   extern const target_phys_addr_t virtio_size;
> Index: b/target-s390x/kvm.c
> ===================================================================
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -185,8 +185,7 @@ void kvm_s390_interrupt_internal(CPUStat
>           return;
>       }
>
> -    env->halted = 0;
> -    env->exception_index = -1;
> +    s390_add_running_cpu(env);
>       qemu_cpu_kick(env);
>
>       kvmint.type = type;
> @@ -299,8 +298,7 @@ static int handle_diag(CPUState *env, st
>   static int s390_cpu_restart(CPUState *env)
>   {
>       kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
> -    env->halted = 0;
> -    env->exception_index = -1;
> +    s390_add_running_cpu(env);
>       qemu_cpu_kick(env);
>       dprintf("DONE: SIGP cpu restart: %p\n", env);
>       return 0;
> @@ -425,17 +423,16 @@ static int handle_intercept(CPUState *en
>               r = handle_instruction(env, run);
>               break;
>           case ICPT_WAITPSW:
> -            /* XXX What to do on system shutdown? */
> -            env->halted = 1;
> -            env->exception_index = EXCP_HLT;
> +        case ICPT_CPU_STOP:
> +            if (s390_del_running_cpu(env) == 0) {
> +                qemu_system_shutdown_request();
> +            }
> +            r = EXCP_HALTED;
>               break;
>           case ICPT_SOFT_INTERCEPT:
>               fprintf(stderr, "KVM unimplemented icpt SOFT\n");
>               exit(1);
>               break;
> -        case ICPT_CPU_STOP:
> -            qemu_system_shutdown_request();
> -            break;
>           case ICPT_IO:
>               fprintf(stderr, "KVM unimplemented icpt IO\n");
>               exit(1);
> @@ -468,8 +465,6 @@ int kvm_arch_handle_exit(CPUState *env,
>
>       if (ret == 0) {
>           ret = EXCP_INTERRUPT;
> -    } else if (ret>  0) {
> -        ret = 0;

Hrm. Are you sure that this doesn't break anything? The rest looks good.


Alex

Re: [Qemu-devel] [PATCHv3] s390: Fix cpu shutdown for KVM

[Christian Borntraeger]

> Hrm. Are you sure that this doesn't break anything? The rest looks good.

Works on my kvm box. Unfortunately the non-kvm mode doesnt work (with and 
without the patch) so I cant test that one.

Christian

Re: [Qemu-devel] [PATCHv3] s390: Fix cpu shutdown for KVM

[Alexander Graf]

On 04.10.2011, at 17:20, Christian Borntraeger wrote:

> On s390 a shutdown is the state of all CPUs being either stopped
> or disabled (for interrupts) waiting. We have to track the overall
> number of running CPUs to call the shutdown sequence accordingly.
> This patch implements the counting and shutdown handling for the 
> kvm path in qemu.
> Lets also wrap changes to env->halted and env->exception_index.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks, applied to s390-next.

Alex