[Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[Christian Borntraeger]

This is the qemu part of kernel series "Let user space control the
cpu states"

Christian Borntraeger (1):
  update linux headers with with cpustate changes

David Hildenbrand (4):
  s390x/kvm: introduce proper states for s390 cpus
  s390x/kvm: proper use of the cpu states OPERATING and STOPPED
  s390x/kvm: test whether a cpu is STOPPED when checking "has_work"
  s390x/kvm: propagate s390 cpu state to kvm

 hw/s390x/ipl.c            |   2 +-
 hw/s390x/s390-virtio.c    |  32 --------------
 linux-headers/linux/kvm.h |   7 ++-
 target-s390x/cpu.c        | 106 +++++++++++++++++++++++++++++++++++++++-------
 target-s390x/cpu.h        |  33 +++++++++++++--
 target-s390x/helper.c     |  11 ++---
 target-s390x/kvm.c        |  49 ++++++++++++++++++---
 trace-events              |   6 +++
 8 files changed, 179 insertions(+), 67 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [PATCH/RFC 1/5] update linux headers with with cpustate changes

[Christian Borntraeger]

Will do a proper headers sync when ready

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 linux-headers/linux/kvm.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index f5d2c38..9b584da 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -399,13 +399,18 @@ struct kvm_vapic_addr {
 	__u64 vapic_addr;
 };
 
-/* for KVM_SET_MPSTATE */
+/* for KVM_SET_MP_STATE */
 
+/* not all states are valid on all architectures */
 #define KVM_MP_STATE_RUNNABLE          0
 #define KVM_MP_STATE_UNINITIALIZED     1
 #define KVM_MP_STATE_INIT_RECEIVED     2
 #define KVM_MP_STATE_HALTED            3
 #define KVM_MP_STATE_SIPI_RECEIVED     4
+#define KVM_MP_STATE_STOPPED           5
+#define KVM_MP_STATE_CHECK_STOP        6
+#define KVM_MP_STATE_OPERATING         7
+#define KVM_MP_STATE_LOAD              8
 
 struct kvm_mp_state {
 	__u32 mp_state;
-- 
1.8.4.2

[Qemu-devel] [PATCH/RFC 2/5] s390x/kvm: introduce proper states for s390 cpus

[Christian Borntraeger]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

Until now, when a s390 cpu was stopped or halted, the number of running
CPUs was tracked in a global variable. This was problematic for migration,
so Jason came up with a per-cpu running state.
As it turns out, we want to track the full logical state of a target vcpu,
so we need real s390 cpu states.

This patch is based on an initial patch by Jason Herne, but was heavily
rewritten when adding the cpu states STOPPED and OPERATING. On the way we
move add_del_running to cpu.c (the declaration is already in cpu.h) and
modify the users where appropriate.

Please note that the cpu is still set to be stopped when it is
halted, which is wrong. This will be fixed in the next patch. The LOAD and
CHECK-STOP state will not be used in the first step.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[folded Jason's patch into David's patch to avoid add/remove same lines]
---
 hw/s390x/s390-virtio.c | 32 --------------------------------
 target-s390x/cpu.c     | 43 +++++++++++++++++++++++++++++++++++++++++++
 target-s390x/cpu.h     | 14 ++++++++++++++
 3 files changed, 57 insertions(+), 32 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 93c7ace..91baa18 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -124,38 +124,6 @@ static void s390_virtio_register_hcalls(void)
                                    s390_virtio_hcall_set_status);
 }
 
-/*
- * 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(S390CPU *cpu)
-{
-    CPUState *cs = CPU(cpu);
-
-    if (cs->halted) {
-        s390_running_cpus++;
-        cs->halted = 0;
-        cs->exception_index = -1;
-    }
-}
-
-unsigned s390_del_running_cpu(S390CPU *cpu)
-{
-    CPUState *cs = CPU(cpu);
-
-    if (cs->halted == 0) {
-        assert(s390_running_cpus >= 1);
-        s390_running_cpus--;
-        cs->halted = 1;
-        cs->exception_index = EXCP_HLT;
-    }
-    return s390_running_cpus;
-}
-
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
                        const char *initrd_filename,
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index c3082b7..a6edd07 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -224,6 +224,49 @@ static void s390_cpu_finalize(Object *obj)
 #endif
 }
 
+#if !defined(CONFIG_USER_ONLY)
+static unsigned s390_count_running_cpus(void)
+{
+    CPUState *cpu;
+    int nr_running = 0;
+
+    CPU_FOREACH(cpu) {
+        uint8_t state = S390_CPU(cpu)->env.cpu_state;
+        if (state == CPU_STATE_OPERATING ||
+            state == CPU_STATE_LOAD) {
+            nr_running++;
+        }
+    }
+
+    return nr_running;
+}
+
+void s390_add_running_cpu(S390CPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+
+    if (cs->halted) {
+        cpu->env.cpu_state = CPU_STATE_OPERATING;
+        cs->halted = 0;
+        cs->exception_index = -1;
+    }
+}
+
+unsigned s390_del_running_cpu(S390CPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+
+    if (cs->halted == 0) {
+        assert(s390_count_running_cpus() >= 1);
+        cpu->env.cpu_state = CPU_STATE_STOPPED;
+        cs->halted = 1;
+        cs->exception_index = EXCP_HLT;
+    }
+
+    return s390_count_running_cpus();
+}
+#endif
+
 static const VMStateDescription vmstate_s390_cpu = {
     .name = "cpu",
     .unmigratable = 1,
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index b13761d..af22ba6 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -141,6 +141,20 @@ typedef struct CPUS390XState {
     QEMUTimer *tod_timer;
 
     QEMUTimer *cpu_timer;
+
+    /*
+     * The cpu state represents the logical state of a cpu. In contrast to other
+     * architectures, there is a difference between a halt and a stop on s390.
+     * If all cpus are either stopped (including check stop) or in the disabled
+     * wait state, the vm can be shut down.
+     */
+#define CPU_STATE_UNINITIALIZED        0x00
+#define CPU_STATE_STOPPED              0x01
+#define CPU_STATE_CHECK_STOP           0x02
+#define CPU_STATE_OPERATING            0x03
+#define CPU_STATE_LOAD                 0x04
+    uint8_t cpu_state;
+
 } CPUS390XState;
 
 #include "cpu-qom.h"
-- 
1.8.4.2

[Qemu-devel] [PATCH/RFC 3/5] s390x/kvm: proper use of the cpu states OPERATING and STOPPED

[Christian Borntraeger]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

This patch makes sure that halting a cpu and stopping a cpu are two different
things. Stopping a cpu will also set the cpu halted - this is needed for common
infrastructure to work (note that the stop and stopped flag cannot be used for
our purpose because they are already used by other mechanisms).

A cpu can be halted ("waiting") when it is operating. If interrupts are
disabled, this is called a "disabled wait", as it can't be woken up anymore. A
stopped cpu is treated like a "disabled wait" cpu, but in order to prepare for a
proper cpu state synchronization with the kvm part, we need to track the real
logical state of a cpu.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/ipl.c        |  2 +-
 target-s390x/cpu.c    | 78 +++++++++++++++++++++++++++++++++------------------
 target-s390x/cpu.h    | 14 ++++++---
 target-s390x/helper.c | 11 ++------
 target-s390x/kvm.c    | 11 ++++----
 trace-events          |  5 ++++
 6 files changed, 75 insertions(+), 46 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 4fa9cff..3b77c9a 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -176,7 +176,7 @@ static void s390_ipl_reset(DeviceState *dev)
         }
     }
 
-    s390_add_running_cpu(cpu);
+    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
 }
 
 static void s390_ipl_class_init(ObjectClass *klass, void *data)
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index a6edd07..c5ab98f 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -26,7 +26,9 @@
 #include "cpu.h"
 #include "qemu-common.h"
 #include "qemu/timer.h"
+#include "qemu/error-report.h"
 #include "hw/hw.h"
+#include "trace.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
@@ -81,7 +83,7 @@ static void s390_cpu_load_normal(CPUState *s)
     S390CPU *cpu = S390_CPU(s);
     cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
     cpu->env.psw.mask = PSW_MASK_32 | PSW_MASK_64;
-    s390_add_running_cpu(cpu);
+    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
 }
 #endif
 
@@ -93,11 +95,8 @@ static void s390_cpu_reset(CPUState *s)
     CPUS390XState *env = &cpu->env;
 
     env->pfault_token = -1UL;
-    s390_del_running_cpu(cpu);
     scc->parent_reset(s);
-#if !defined(CONFIG_USER_ONLY)
-    s->halted = 1;
-#endif
+    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
     tlb_flush(s, 1);
 }
 
@@ -135,9 +134,8 @@ static void s390_cpu_full_reset(CPUState *s)
     S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
     CPUS390XState *env = &cpu->env;
 
-    s390_del_running_cpu(cpu);
-
     scc->parent_reset(s);
+    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
 
     memset(env, 0, offsetof(CPUS390XState, cpu_num));
 
@@ -147,12 +145,7 @@ static void s390_cpu_full_reset(CPUState *s)
 
     env->pfault_token = -1UL;
 
-    /* set halted to 1 to make sure we can add the cpu in
-     * s390_ipl_cpu code, where CPUState::halted is set back to 0
-     * after incrementing the cpu counter */
 #if !defined(CONFIG_USER_ONLY)
-    s->halted = 1;
-
     if (kvm_enabled()) {
         kvm_s390_reset_vcpu(cpu);
     }
@@ -201,10 +194,7 @@ static void s390_cpu_initfn(Object *obj)
     env->tod_basetime = 0;
     env->tod_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
     env->cpu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
-    /* set CPUState::halted state to 1 to avoid decrementing the running
-     * cpu counter in s390_cpu_reset to a negative number at
-     * initial ipl */
-    cs->halted = 1;
+    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
 #endif
     env->cpu_num = cpu_num++;
     env->ext_index = -1;
@@ -225,6 +215,12 @@ static void s390_cpu_finalize(Object *obj)
 }
 
 #if !defined(CONFIG_USER_ONLY)
+static bool disabled_wait(CPUState *cpu)
+{
+    return cpu->halted && !(S390_CPU(cpu)->env.psw.mask &
+                            (PSW_MASK_IO | PSW_MASK_EXT | PSW_MASK_MCHECK));
+}
+
 static unsigned s390_count_running_cpus(void)
 {
     CPUState *cpu;
@@ -234,34 +230,60 @@ static unsigned s390_count_running_cpus(void)
         uint8_t state = S390_CPU(cpu)->env.cpu_state;
         if (state == CPU_STATE_OPERATING ||
             state == CPU_STATE_LOAD) {
-            nr_running++;
+            if (!disabled_wait(cpu)) {
+                nr_running++;
+            }
         }
     }
 
     return nr_running;
 }
 
-void s390_add_running_cpu(S390CPU *cpu)
+unsigned int s390_cpu_halt(S390CPU *cpu)
 {
     CPUState *cs = CPU(cpu);
+    trace_cpu_halt(cs->cpu_index);
 
-    if (cs->halted) {
-        cpu->env.cpu_state = CPU_STATE_OPERATING;
-        cs->halted = 0;
-        cs->exception_index = -1;
+    if (!cs->halted) {
+        cs->halted = 1;
+        cs->exception_index = EXCP_HLT;
     }
+
+    return s390_count_running_cpus();
 }
 
-unsigned s390_del_running_cpu(S390CPU *cpu)
+void s390_cpu_unhalt(S390CPU *cpu)
 {
     CPUState *cs = CPU(cpu);
+    trace_cpu_unhalt(cs->cpu_index);
 
-    if (cs->halted == 0) {
-        assert(s390_count_running_cpus() >= 1);
-        cpu->env.cpu_state = CPU_STATE_STOPPED;
-        cs->halted = 1;
-        cs->exception_index = EXCP_HLT;
+    if (cs->halted) {
+        cs->halted = 0;
+        cs->exception_index = -1;
+    }
+}
+
+unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
+ {
+    trace_cpu_set_state(CPU(cpu)->cpu_index, cpu_state);
+
+    switch (cpu_state) {
+    case CPU_STATE_STOPPED:
+    case CPU_STATE_CHECK_STOP:
+        /* halt the cpu for common infrastructure */
+        s390_cpu_halt(cpu);
+        break;
+    case CPU_STATE_OPERATING:
+    case CPU_STATE_LOAD:
+        /* unhalt the cpu for common infrastructure */
+        s390_cpu_unhalt(cpu);
+        break;
+    default:
+        error_report("Requested CPU state is not a valid S390 CPU state: %u",
+                     cpu_state);
+        exit(1);
     }
+    cpu->env.cpu_state = cpu_state;
 
     return s390_count_running_cpus();
 }
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index af22ba6..892726c 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -392,8 +392,9 @@ static inline void kvm_s390_service_interrupt(uint32_t parm)
 }
 #endif
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
-void s390_add_running_cpu(S390CPU *cpu);
-unsigned s390_del_running_cpu(S390CPU *cpu);
+unsigned int s390_cpu_halt(S390CPU *cpu);
+void s390_cpu_unhalt(S390CPU *cpu);
+unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
 
 /* service interrupts are floating therefore we must not pass an cpustate */
 void s390_sclp_extint(uint32_t parm);
@@ -402,11 +403,16 @@ void s390_sclp_extint(uint32_t parm);
 extern const hwaddr virtio_size;
 
 #else
-static inline void s390_add_running_cpu(S390CPU *cpu)
+static inline unsigned int s390_cpu_halt(S390CPU *cpu)
+{
+    return 0;
+}
+
+static inline void s390_cpu_unhalt(S390CPU *cpu)
 {
 }
 
-static inline unsigned s390_del_running_cpu(S390CPU *cpu)
+static inline unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
 {
     return 0;
 }
diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 67ab106..cd33500 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -506,16 +506,11 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
 {
     if (mask & PSW_MASK_WAIT) {
         S390CPU *cpu = s390_env_get_cpu(env);
-        CPUState *cs = CPU(cpu);
-        if (!(mask & (PSW_MASK_IO | PSW_MASK_EXT | PSW_MASK_MCHECK))) {
-            if (s390_del_running_cpu(cpu) == 0) {
+        if (s390_cpu_halt(cpu) == 0) {
 #ifndef CONFIG_USER_ONLY
-                qemu_system_shutdown_request();
+            qemu_system_shutdown_request();
 #endif
-            }
         }
-        cs->halted = 1;
-        cs->exception_index = EXCP_HLT;
     }
 
     env->psw.addr = addr;
@@ -818,7 +813,7 @@ void s390_cpu_do_interrupt(CPUState *cs)
     qemu_log_mask(CPU_LOG_INT, "%s: %d at pc=%" PRIx64 "\n",
                   __func__, cs->exception_index, env->psw.addr);
 
-    s390_add_running_cpu(cpu);
+    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
     /* handle machine checks */
     if ((env->psw.mask & PSW_MASK_MCHECK) &&
         (cs->exception_index == -1)) {
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index a6e587b..db2e42c 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -895,7 +895,7 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
 
 static int kvm_s390_cpu_start(S390CPU *cpu)
 {
-    s390_add_running_cpu(cpu);
+    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
     qemu_cpu_kick(CPU(cpu));
     DPRINTF("DONE: KVM cpu start: %p\n", &cpu->env);
     return 0;
@@ -908,7 +908,7 @@ int kvm_s390_cpu_restart(S390CPU *cpu)
     };
 
     kvm_s390_vcpu_interrupt(cpu, &irq);
-    s390_add_running_cpu(cpu);
+    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
     qemu_cpu_kick(CPU(cpu));
     DPRINTF("DONE: KVM cpu restart: %p\n", &cpu->env);
     return 0;
@@ -1042,7 +1042,7 @@ static void unmanageable_intercept(S390CPU *cpu, const char *str, int pswoffset)
     error_report("Unmanageable %s! CPU%i new PSW: 0x%016lx:%016lx",
                  str, cs->cpu_index, ldq_phys(cs->as, cpu->env.psa + pswoffset),
                  ldq_phys(cs->as, cpu->env.psa + pswoffset + 8));
-    s390_del_running_cpu(cpu);
+    s390_cpu_halt(cpu);
     guest_panicked();
 }
 
@@ -1071,7 +1071,8 @@ static int handle_intercept(S390CPU *cpu)
             break;
         case ICPT_WAITPSW:
             /* disabled wait, since enabled wait is handled in kernel */
-            if (s390_del_running_cpu(cpu) == 0) {
+            cpu_synchronize_state(cs);
+            if (s390_cpu_halt(cpu) == 0) {
                 if (is_special_wait_psw(cs)) {
                     qemu_system_shutdown_request();
                 } else {
@@ -1081,7 +1082,7 @@ static int handle_intercept(S390CPU *cpu)
             r = EXCP_HALTED;
             break;
         case ICPT_CPU_STOP:
-            if (s390_del_running_cpu(cpu) == 0) {
+            if (s390_cpu_set_state(CPU_STATE_STOPPED, cpu) == 0) {
                 qemu_system_shutdown_request();
             }
             r = EXCP_HALTED;
diff --git a/trace-events b/trace-events
index 11a17a8..c652606 100644
--- a/trace-events
+++ b/trace-events
@@ -1301,3 +1301,8 @@ mhp_pc_dimm_assigned_address(uint64_t addr) "0x%"PRIx64
 # target-s390x/kvm.c
 kvm_enable_cmma(int rc) "CMMA: enabling with result code %d"
 kvm_clear_cmma(int rc) "CMMA: clearing with result code %d"
+
+# target-s390x/cpu.c
+cpu_set_state(int cpu_index, uint8_t state) "setting cpu %d state to %" PRIu8
+cpu_halt(int cpu_index) "halting cpu %d"
+cpu_unhalt(int cpu_index) "unhalting cpu %d"
-- 
1.8.4.2

[Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[Christian Borntraeger]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

If a cpu is stopped, it must never be allowed to run and no interrupt may wake it
up. A cpu also has to be unhalted if it is halted and has work to do - this
scenario wasn't hit in kvm case yet, as only "disabled wait" is processed within
QEMU.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-s390x/cpu.c | 6 ++++++
 target-s390x/kvm.c | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index c5ab98f..1d32f5a 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -72,6 +72,12 @@ static bool s390_cpu_has_work(CPUState *cs)
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
 
+    /* stopped cpus can never run */
+    if (env->cpu_state == CPU_STATE_STOPPED ||
+        env->cpu_state == CPU_STATE_CHECK_STOP) {
+        return false;
+    }
+
     return (cs->interrupt_request & CPU_INTERRUPT_HARD) &&
            (env->psw.mask & PSW_MASK_EXT);
 }
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index db2e42c..00125f1 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -553,6 +553,11 @@ void kvm_arch_post_run(CPUState *cpu, struct kvm_run *run)
 
 int kvm_arch_process_async_events(CPUState *cs)
 {
+    if (cs->halted && CPU_GET_CLASS(cs)->has_work(cs)) {
+        /* has_work will take care of stopped cpus */
+        s390_cpu_unhalt(S390_CPU(cs));
+    }
+
     return cs->halted;
 }
 
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[Alexander Graf]

On 10.07.14 15:10, Christian Borntraeger wrote:
> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>
> If a cpu is stopped, it must never be allowed to run and no interrupt may wake it
> up. A cpu also has to be unhalted if it is halted and has work to do - this
> scenario wasn't hit in kvm case yet, as only "disabled wait" is processed within
> QEMU.
>
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

This looks like it's something that generic infrastructure should take 
care of, no? How does this work for the other archs? They always get an 
interrupt on the transition between !has_work -> has_work. Why don't we 
get one for s390x?


Alex

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[David Hildenbrand]

> 
> On 10.07.14 15:10, Christian Borntraeger wrote:
> > From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >
> > If a cpu is stopped, it must never be allowed to run and no interrupt may wake it
> > up. A cpu also has to be unhalted if it is halted and has work to do - this
> > scenario wasn't hit in kvm case yet, as only "disabled wait" is processed within
> > QEMU.
> >
> > Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> This looks like it's something that generic infrastructure should take 
> care of, no? How does this work for the other archs? They always get an 
> interrupt on the transition between !has_work -> has_work. Why don't we 
> get one for s390x?
> 
> 
> Alex
> 
> 

Well, we have the special case on s390 as a CPU that is in the STOPPED or the
CHECK STOP state may never run - even if there is an interrupt. It's
basically like this CPU has been switched off.

Imagine that it is tried to inject an interrupt into a stopped vcpu. It
will kick the stopped vcpu and thus lead to a call to
"kvm_arch_process_async_events()". We have to deny that this vcpu will ever
run as long as it is stopped. It's like a way to "suppress" the
interrupt for such a transition you mentioned.

Later, another vcpu might decide to turn that vcpu back on (by e.g. sending a
SIGP START to that vcpu).

I am not sure if such a mechanism/scenario is applicable to any other arch. They
all seem to reset the cs->halted flag if they know they are able to run (e.g.
due to an interrupt) - they have no such thing as "stopped cpus", only
"halted/waiting cpus".

David

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[Paolo Bonzini]

Il 28/07/2014 16:16, David Hildenbrand ha scritto:
> Later, another vcpu might decide to turn that vcpu back on (by e.g. sending a
> SIGP START to that vcpu).
> 
> I am not sure if such a mechanism/scenario is applicable to any other arch. They
> all seem to reset the cs->halted flag if they know they are able to run (e.g.
> due to an interrupt) - they have no such thing as "stopped cpus", only
> "halted/waiting cpus".

On x86, INIT_RECEIVED is pretty much a stopped CPU.  It can only run
(and receive interrupts) after getting a special startup interrupt ("SIPI").

Paolo

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[Alexander Graf]

On 28.07.2014, at 16:16, David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:

>> 
>> On 10.07.14 15:10, Christian Borntraeger wrote:
>>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>> 
>>> If a cpu is stopped, it must never be allowed to run and no interrupt may wake it
>>> up. A cpu also has to be unhalted if it is halted and has work to do - this
>>> scenario wasn't hit in kvm case yet, as only "disabled wait" is processed within
>>> QEMU.
>>> 
>>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> 
>> This looks like it's something that generic infrastructure should take 
>> care of, no? How does this work for the other archs? They always get an 
>> interrupt on the transition between !has_work -> has_work. Why don't we 
>> get one for s390x?
>> 
>> 
>> Alex
>> 
>> 
> 
> Well, we have the special case on s390 as a CPU that is in the STOPPED or the
> CHECK STOP state may never run - even if there is an interrupt. It's
> basically like this CPU has been switched off.
> 
> Imagine that it is tried to inject an interrupt into a stopped vcpu. It
> will kick the stopped vcpu and thus lead to a call to
> "kvm_arch_process_async_events()". We have to deny that this vcpu will ever
> run as long as it is stopped. It's like a way to "suppress" the
> interrupt for such a transition you mentioned.

An interrupt kick usually just means we go back into the main loop. From there we check the interrupt bitmap which interrupt to handle. Check out the handling code here:

  http://git.qemu.org/?p=qemu.git;a=blob;f=cpu-exec.c;h=38e5f02a307523d99134f4e2e6c51683bb10b45b;hb=HEAD#l580

If you just check for the stopped state in here, do_interrupt() will never get called and thus the CPU shouldn't ever get executed. Unless I'm heavily mistaken :).

> 
> Later, another vcpu might decide to turn that vcpu back on (by e.g. sending a
> SIGP START to that vcpu).

Yes, in that case that other CPU generates a signal (a different bit in interrupt_request) and the first CPU would see that it has to wake up and wake up.

> I am not sure if such a mechanism/scenario is applicable to any other arch. They
> all seem to reset the cs->halted flag if they know they are able to run (e.g.
> due to an interrupt) - they have no such thing as "stopped cpus", only
> "halted/waiting cpus".

There's not really much difference between the two. The only difference from a software point of view is that a "stopped" CPU has its external interrupt bits masked off, no?


Alex

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[David Hildenbrand]

> 
> On 28.07.2014, at 16:16, David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:
> 
> >> 
> >> On 10.07.14 15:10, Christian Borntraeger wrote:
> >>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >>> 
> >>> If a cpu is stopped, it must never be allowed to run and no interrupt may wake it
> >>> up. A cpu also has to be unhalted if it is halted and has work to do - this
> >>> scenario wasn't hit in kvm case yet, as only "disabled wait" is processed within
> >>> QEMU.
> >>> 
> >>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> 
> >> This looks like it's something that generic infrastructure should take 
> >> care of, no? How does this work for the other archs? They always get an 
> >> interrupt on the transition between !has_work -> has_work. Why don't we 
> >> get one for s390x?
> >> 
> >> 
> >> Alex
> >> 
> >> 
> > 
> > Well, we have the special case on s390 as a CPU that is in the STOPPED or the
> > CHECK STOP state may never run - even if there is an interrupt. It's
> > basically like this CPU has been switched off.
> > 
> > Imagine that it is tried to inject an interrupt into a stopped vcpu. It
> > will kick the stopped vcpu and thus lead to a call to
> > "kvm_arch_process_async_events()". We have to deny that this vcpu will ever
> > run as long as it is stopped. It's like a way to "suppress" the
> > interrupt for such a transition you mentioned.
> 
> An interrupt kick usually just means we go back into the main loop. From there we check the interrupt bitmap which interrupt to handle. Check out the handling code here:
> 
>   http://git.qemu.org/?p=qemu.git;a=blob;f=cpu-exec.c;h=38e5f02a307523d99134f4e2e6c51683bb10b45b;hb=HEAD#l580
> 
> If you just check for the stopped state in here, do_interrupt() will never get called and thus the CPU shouldn't ever get executed. Unless I'm heavily mistaken :).

So you would rather move the check out of has_work() into the main loop in
cpu-exec.c and directly into kvm_arch_process_async_events()?

This would on the other hand lead to an unhalt of the vcpu in cpu_exec() on any
CPU_INTERRUPT_HARD. A VCPU might thus be unhalted although it is not able to run. Is okay?

Looking at cpu.c:cpu_thread_is_idle(), we would maybe return false, although we
are idle (because we are idle when we are stopped)?

My qemu kvm knowledge is way better than the qemu emulation knowledge, so I
appreciate any insights :)

> 
> > 
> > Later, another vcpu might decide to turn that vcpu back on (by e.g. sending a
> > SIGP START to that vcpu).
> 
> Yes, in that case that other CPU generates a signal (a different bit in interrupt_request) and the first CPU would see that it has to wake up and wake up.
> 
> > I am not sure if such a mechanism/scenario is applicable to any other arch. They
> > all seem to reset the cs->halted flag if they know they are able to run (e.g.
> > due to an interrupt) - they have no such thing as "stopped cpus", only
> > "halted/waiting cpus".
> 
> There's not really much difference between the two. The only difference from a software point of view is that a "stopped" CPU has its external interrupt bits masked off, no?

Well the difference is, that a STOPPED vcpu can be woken up by non-interrupt
like things (SIGP START) AND a special interrupt (SIGP RESTART - which is like
a "SIPI"++ as it performs a psw exchange - "NMI"). So we basically have two
paths that can lead to a state change. All interrupt bits may be in any
combination (SIGP RESTART interrupts can't be masked out, nor can SIGP START be
denied).

The other thing may be that on s390, each vcpu (including itself) can put
another vcpu into the STOPPED state - I assume that this is different for x86 "
INIT_RECEIVED". For this reason we have to watch out for bad race conditions
(e.g. multiple vcpus working on another vcpu)...

David

> 
> 
> Alex
> 

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[David Hildenbrand]

> > 
> > On 28.07.2014, at 16:16, David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:
> > 
> > >> 
> > >> On 10.07.14 15:10, Christian Borntraeger wrote:
> > >>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > >>> 
> > >>> If a cpu is stopped, it must never be allowed to run and no interrupt may wake it
> > >>> up. A cpu also has to be unhalted if it is halted and has work to do - this
> > >>> scenario wasn't hit in kvm case yet, as only "disabled wait" is processed within
> > >>> QEMU.
> > >>> 
> > >>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > >>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > >>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > >> 
> > >> This looks like it's something that generic infrastructure should take 
> > >> care of, no? How does this work for the other archs? They always get an 
> > >> interrupt on the transition between !has_work -> has_work. Why don't we 
> > >> get one for s390x?
> > >> 
> > >> 
> > >> Alex
> > >> 
> > >> 
> > > 
> > > Well, we have the special case on s390 as a CPU that is in the STOPPED or the
> > > CHECK STOP state may never run - even if there is an interrupt. It's
> > > basically like this CPU has been switched off.
> > > 
> > > Imagine that it is tried to inject an interrupt into a stopped vcpu. It
> > > will kick the stopped vcpu and thus lead to a call to
> > > "kvm_arch_process_async_events()". We have to deny that this vcpu will ever
> > > run as long as it is stopped. It's like a way to "suppress" the
> > > interrupt for such a transition you mentioned.
> > 
> > An interrupt kick usually just means we go back into the main loop. From there we check the interrupt bitmap which interrupt to handle. Check out the handling code here:
> > 
> >   http://git.qemu.org/?p=qemu.git;a=blob;f=cpu-exec.c;h=38e5f02a307523d99134f4e2e6c51683bb10b45b;hb=HEAD#l580
> > 
> > If you just check for the stopped state in here, do_interrupt() will never get called and thus the CPU shouldn't ever get executed. Unless I'm heavily mistaken :).
> 
> So you would rather move the check out of has_work() into the main loop in
> cpu-exec.c and directly into kvm_arch_process_async_events()?
> 
> This would on the other hand lead to an unhalt of the vcpu in cpu_exec() on any
> CPU_INTERRUPT_HARD. A VCPU might thus be unhalted although it is not able to run. Is okay?
> 
> Looking at cpu.c:cpu_thread_is_idle(), we would maybe return false, although we
> are idle (because we are idle when we are stopped)?
> 
> My qemu kvm knowledge is way better than the qemu emulation knowledge, so I
> appreciate any insights :)
> 
> > 
> > > 
> > > Later, another vcpu might decide to turn that vcpu back on (by e.g. sending a
> > > SIGP START to that vcpu).
> > 
> > Yes, in that case that other CPU generates a signal (a different bit in interrupt_request) and the first CPU would see that it has to wake up and wake up.
> > 
> > > I am not sure if such a mechanism/scenario is applicable to any other arch. They
> > > all seem to reset the cs->halted flag if they know they are able to run (e.g.
> > > due to an interrupt) - they have no such thing as "stopped cpus", only
> > > "halted/waiting cpus".
> > 
> > There's not really much difference between the two. The only difference from a software point of view is that a "stopped" CPU has its external interrupt bits masked off, no?
> 
> Well the difference is, that a STOPPED vcpu can be woken up by non-interrupt
> like things (SIGP START) AND a special interrupt (SIGP RESTART - which is like
> a "SIPI"++ as it performs a psw exchange - "NMI"). So we basically have two
> paths that can lead to a state change. All interrupt bits may be in any
> combination (SIGP RESTART interrupts can't be masked out, nor can SIGP START be
> denied).
> 
> The other thing may be that on s390, each vcpu (including itself) can put
> another vcpu into the STOPPED state - I assume that this is different for x86 "
> INIT_RECEIVED". For this reason we have to watch out for bad race conditions
> (e.g. multiple vcpus working on another vcpu)...

Ah, sorry, just to clearify, a vcpu always sets itself to STOPPED, its the other
vcpus that trigger it (= interrupt-like).

David

> 
> David
> 
> > 
> > 
> > Alex
> > 
> 

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[Alexander Graf]

On 28.07.14 17:03, David Hildenbrand wrote:
>> On 28.07.2014, at 16:16, David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:
>>
>>>> On 10.07.14 15:10, Christian Borntraeger wrote:
>>>>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>>>>
>>>>> If a cpu is stopped, it must never be allowed to run and no interrupt may wake it
>>>>> up. A cpu also has to be unhalted if it is halted and has work to do - this
>>>>> scenario wasn't hit in kvm case yet, as only "disabled wait" is processed within
>>>>> QEMU.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>>>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> This looks like it's something that generic infrastructure should take
>>>> care of, no? How does this work for the other archs? They always get an
>>>> interrupt on the transition between !has_work -> has_work. Why don't we
>>>> get one for s390x?
>>>>
>>>>
>>>> Alex
>>>>
>>>>
>>> Well, we have the special case on s390 as a CPU that is in the STOPPED or the
>>> CHECK STOP state may never run - even if there is an interrupt. It's
>>> basically like this CPU has been switched off.
>>>
>>> Imagine that it is tried to inject an interrupt into a stopped vcpu. It
>>> will kick the stopped vcpu and thus lead to a call to
>>> "kvm_arch_process_async_events()". We have to deny that this vcpu will ever
>>> run as long as it is stopped. It's like a way to "suppress" the
>>> interrupt for such a transition you mentioned.
>> An interrupt kick usually just means we go back into the main loop. From there we check the interrupt bitmap which interrupt to handle. Check out the handling code here:
>>
>>    http://git.qemu.org/?p=qemu.git;a=blob;f=cpu-exec.c;h=38e5f02a307523d99134f4e2e6c51683bb10b45b;hb=HEAD#l580
>>
>> If you just check for the stopped state in here, do_interrupt() will never get called and thus the CPU shouldn't ever get executed. Unless I'm heavily mistaken :).
> So you would rather move the check out of has_work() into the main loop in
> cpu-exec.c and directly into kvm_arch_process_async_events()?
>
> This would on the other hand lead to an unhalt of the vcpu in cpu_exec() on any
> CPU_INTERRUPT_HARD. A VCPU might thus be unhalted although it is not able to run. Is okay?

Not really I think. We could create a new interrupt_request bit called 
CPU_INTERRUPT_STOPPED that doesn't get unset automatically and simply 
sets cpu->halted = 1 (similar to CPU_INTERRUPT_HALT).

>
> Looking at cpu.c:cpu_thread_is_idle(), we would maybe return false, although we
> are idle (because we are idle when we are stopped)?
>
> My qemu kvm knowledge is way better than the qemu emulation knowledge, so I
> appreciate any insights :)
>
>>> Later, another vcpu might decide to turn that vcpu back on (by e.g. sending a
>>> SIGP START to that vcpu).
>> Yes, in that case that other CPU generates a signal (a different bit in interrupt_request) and the first CPU would see that it has to wake up and wake up.
>>
>>> I am not sure if such a mechanism/scenario is applicable to any other arch. They
>>> all seem to reset the cs->halted flag if they know they are able to run (e.g.
>>> due to an interrupt) - they have no such thing as "stopped cpus", only
>>> "halted/waiting cpus".
>> There's not really much difference between the two. The only difference from a software point of view is that a "stopped" CPU has its external interrupt bits masked off, no?
> Well the difference is, that a STOPPED vcpu can be woken up by non-interrupt
> like things (SIGP START) AND a special interrupt (SIGP RESTART - which is like
> a "SIPI"++ as it performs a psw exchange - "NMI"). So we basically have two
> paths that can lead to a state change. All interrupt bits may be in any
> combination (SIGP RESTART interrupts can't be masked out, nor can SIGP START be
> denied).

That's perfectly normal behavior. Just make it two different interrupt 
types:

if (interrupt_request & CPU_INTERRUPT_STOPPED) {
   /* Go back to halted state */
   ...
} else if (interrupt_request & CPU_INTERRUPT_SIGP) {
   env->interrupt_request &= ~CPU_INTERRUPT_STOPPED;
   /* swap PSW */
   ...
} else if ((interrupt_request & CPU_INTERRUPT_HARD) &&
(env->psw.mask & PSW_MASK_EXT)) {
   ...
}

>
> The other thing may be that on s390, each vcpu (including itself) can put
> another vcpu into the STOPPED state - I assume that this is different for x86 "
> INIT_RECEIVED". For this reason we have to watch out for bad race conditions
> (e.g. multiple vcpus working on another vcpu)...

TCG is single-threaded :). And if you stick to the interrupt logic above 
all should be good.


Alex

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[Paolo Bonzini]

Il 28/07/2014 17:03, David Hildenbrand ha scritto:
> Well the difference is, that a STOPPED vcpu can be woken up by non-interrupt
> like things (SIGP START) AND a special interrupt (SIGP RESTART - which is like
> a "SIPI"++ as it performs a psw exchange - "NMI"). So we basically have two
> paths that can lead to a state change.  All interrupt bits may be in any
> combination (SIGP RESTART interrupts can't be masked out, nor can SIGP START be
> denied).
> 
> The other thing may be that on s390, each vcpu (including itself) can put
> another vcpu into the STOPPED state - I assume that this is different for x86 "
> INIT_RECEIVED". For this reason we have to watch out for bad race conditions
> (e.g. multiple vcpus working on another vcpu)...

You can do that in x86 by sending an INIT inter-processor interrupt.  A
SIPI is ignored if the CPU is not in INIT_RECEIVED state.

Commit 66450a21f99636af4fafac2afd33f1a40631bc3a introduced the current
implementation.

- an INIT cancels a previous SIPI;

- if both INIT and SIPI are sent, on real hardware you need to have a
few hundred microseconds between them, but KVM will reliably process
INIT before SIPI.

See commit 299018f44ac553dce3caf84df1d14c4764faa279 for an example of
the races that can happen.

Note that x86 has KVM_MP_STATE_SIPI_RECEIVED state but it is obsolete,
we go straight from KVM_MP_STATE_INIT_RECEIVED to KVM_MP_STATE_RUNNABLE.

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[David Hildenbrand]

> Il 28/07/2014 17:03, David Hildenbrand ha scritto:
> > Well the difference is, that a STOPPED vcpu can be woken up by non-interrupt
> > like things (SIGP START) AND a special interrupt (SIGP RESTART - which is like
> > a "SIPI"++ as it performs a psw exchange - "NMI"). So we basically have two
> > paths that can lead to a state change.  All interrupt bits may be in any
> > combination (SIGP RESTART interrupts can't be masked out, nor can SIGP START be
> > denied).
> > 
> > The other thing may be that on s390, each vcpu (including itself) can put
> > another vcpu into the STOPPED state - I assume that this is different for x86 "
> > INIT_RECEIVED". For this reason we have to watch out for bad race conditions
> > (e.g. multiple vcpus working on another vcpu)...
> 
> You can do that in x86 by sending an INIT inter-processor interrupt.  A
> SIPI is ignored if the CPU is not in INIT_RECEIVED state.
> 
> Commit 66450a21f99636af4fafac2afd33f1a40631bc3a introduced the current
> implementation.
> 
> - an INIT cancels a previous SIPI;
> 
> - if both INIT and SIPI are sent, on real hardware you need to have a
> few hundred microseconds between them, but KVM will reliably process
> INIT before SIPI.
> 
> See commit 299018f44ac553dce3caf84df1d14c4764faa279 for an example of
> the races that can happen.
> 
> Note that x86 has KVM_MP_STATE_SIPI_RECEIVED state but it is obsolete,
> we go straight from KVM_MP_STATE_INIT_RECEIVED to KVM_MP_STATE_RUNNABLE.
> 

Thanks for the explanation Paolo!

Looks like from an interrupt point of view, the states have a lot in common.
The major thing that differs on s390 is probably the way these interrupts are
generated and what else they influence (all the power of the SIGP facility :)
+ special check-stop state that can't be left by an interrupt, only by SIGP
  CPU resets).

David

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[Christian Borntraeger]

On 28/07/14 16:22, Alexander Graf wrote:
> 
> On 28.07.2014, at 16:16, David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:
> 
>>>
>>> On 10.07.14 15:10, Christian Borntraeger wrote:
>>>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>>>
>>>> If a cpu is stopped, it must never be allowed to run and no interrupt may wake it
>>>> up. A cpu also has to be unhalted if it is halted and has work to do - this
>>>> scenario wasn't hit in kvm case yet, as only "disabled wait" is processed within
>>>> QEMU.
>>>>
>>>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>
>>> This looks like it's something that generic infrastructure should take 
>>> care of, no? How does this work for the other archs? They always get an 
>>> interrupt on the transition between !has_work -> has_work. Why don't we 
>>> get one for s390x?
>>>
>>>
>>> Alex
>>>
>>>
>>
>> Well, we have the special case on s390 as a CPU that is in the STOPPED or the
>> CHECK STOP state may never run - even if there is an interrupt. It's
>> basically like this CPU has been switched off.
>>
>> Imagine that it is tried to inject an interrupt into a stopped vcpu. It
>> will kick the stopped vcpu and thus lead to a call to
>> "kvm_arch_process_async_events()". We have to deny that this vcpu will ever
>> run as long as it is stopped. It's like a way to "suppress" the
>> interrupt for such a transition you mentioned.
> 
> An interrupt kick usually just means we go back into the main loop. From there we check the interrupt bitmap which interrupt to handle. Check out the handling code here:
> 
>   http://git.qemu.org/?p=qemu.git;a=blob;f=cpu-exec.c;h=38e5f02a307523d99134f4e2e6c51683bb10b45b;hb=HEAD#l580
> 
> If you just check for the stopped state in here, do_interrupt() will never get called and thus the CPU shouldn't ever get executed. Unless I'm heavily mistaken :).
> 
>>
>> Later, another vcpu might decide to turn that vcpu back on (by e.g. sending a
>> SIGP START to that vcpu).
> 
> Yes, in that case that other CPU generates a signal (a different bit in interrupt_request) and the first CPU would see that it has to wake up and wake up.
> 
>> I am not sure if such a mechanism/scenario is applicable to any other arch. They
>> all seem to reset the cs->halted flag if they know they are able to run (e.g.
>> due to an interrupt) - they have no such thing as "stopped cpus", only
>> "halted/waiting cpus".
> 
> There's not really much difference between the two. The only difference from a software point of view is that a "stopped" CPU has its external interrupt bits masked off, no?

We have
- wait (wait bit in PSW)
- disabled wait (wait bit and interrupt fencing in PSW)
- STOPPED (not related to PSW, state change usually handled via service processor or hypervisor)

I think we have to differentiate between KVM/TCG. On KVM we always do in kernel halt and qemu sees a halted only for STOPPED or disabled wait. TCG has to take care of the normal wait as well.

>From a first glimpse, a disabled wait and STOPPED look similar, but there are (important) differences, e.g. other CPUs get a different a different result from a SIGP SENSE. This makes a big difference, e.g. for Linux guests, that send a SIGP STOP, followed by a SIGP SENSE loop until the CPU is down on hotplug (and shutdown, kexec..) So I think we agree, that handling the cpu states natively makes sense.

The question is now only how to model it correctly without breaking TCG/KVM and reuse as much common code as possible. Correct?

Do I understand you correctly, that your collapsing of stopped and halted is only in the qemu coding sense, IOW maybe we could just modify kvm_arch_process_async_events to consider the STOPPED state, as TCGs sigp implementation does not support SMP anyway?
David would that work?

Christian

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[Alexander Graf]

On 29.07.14 13:44, Christian Borntraeger wrote:
> On 28/07/14 16:22, Alexander Graf wrote:
>> On 28.07.2014, at 16:16, David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:
>>
>>>> On 10.07.14 15:10, Christian Borntraeger wrote:
>>>>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>>>>
>>>>> If a cpu is stopped, it must never be allowed to run and no interrupt may wake it
>>>>> up. A cpu also has to be unhalted if it is halted and has work to do - this
>>>>> scenario wasn't hit in kvm case yet, as only "disabled wait" is processed within
>>>>> QEMU.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>>>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> This looks like it's something that generic infrastructure should take
>>>> care of, no? How does this work for the other archs? They always get an
>>>> interrupt on the transition between !has_work -> has_work. Why don't we
>>>> get one for s390x?
>>>>
>>>>
>>>> Alex
>>>>
>>>>
>>> Well, we have the special case on s390 as a CPU that is in the STOPPED or the
>>> CHECK STOP state may never run - even if there is an interrupt. It's
>>> basically like this CPU has been switched off.
>>>
>>> Imagine that it is tried to inject an interrupt into a stopped vcpu. It
>>> will kick the stopped vcpu and thus lead to a call to
>>> "kvm_arch_process_async_events()". We have to deny that this vcpu will ever
>>> run as long as it is stopped. It's like a way to "suppress" the
>>> interrupt for such a transition you mentioned.
>> An interrupt kick usually just means we go back into the main loop. From there we check the interrupt bitmap which interrupt to handle. Check out the handling code here:
>>
>>    http://git.qemu.org/?p=qemu.git;a=blob;f=cpu-exec.c;h=38e5f02a307523d99134f4e2e6c51683bb10b45b;hb=HEAD#l580
>>
>> If you just check for the stopped state in here, do_interrupt() will never get called and thus the CPU shouldn't ever get executed. Unless I'm heavily mistaken :).
>>
>>> Later, another vcpu might decide to turn that vcpu back on (by e.g. sending a
>>> SIGP START to that vcpu).
>> Yes, in that case that other CPU generates a signal (a different bit in interrupt_request) and the first CPU would see that it has to wake up and wake up.
>>
>>> I am not sure if such a mechanism/scenario is applicable to any other arch. They
>>> all seem to reset the cs->halted flag if they know they are able to run (e.g.
>>> due to an interrupt) - they have no such thing as "stopped cpus", only
>>> "halted/waiting cpus".
>> There's not really much difference between the two. The only difference from a software point of view is that a "stopped" CPU has its external interrupt bits masked off, no?
> We have
> - wait (wait bit in PSW)
> - disabled wait (wait bit and interrupt fencing in PSW)
> - STOPPED (not related to PSW, state change usually handled via service processor or hypervisor)
>
> I think we have to differentiate between KVM/TCG. On KVM we always do in kernel halt and qemu sees a halted only for STOPPED or disabled wait. TCG has to take care of the normal wait as well.
>
>  From a first glimpse, a disabled wait and STOPPED look similar, but there are (important) differences, e.g. other CPUs get a different a different result from a SIGP SENSE. This makes a big difference, e.g. for Linux guests, that send a SIGP STOP, followed by a SIGP SENSE loop until the CPU is down on hotplug (and shutdown, kexec..) So I think we agree, that handling the cpu states natively makes sense.
>
> The question is now only how to model it correctly without breaking TCG/KVM and reuse as much common code as possible. Correct?
>
> Do I understand you correctly, that your collapsing of stopped and halted is only in the qemu coding sense, IOW maybe we could just modify kvm_arch_process_async_events to consider the STOPPED state, as TCGs sigp implementation does not support SMP anyway?

That works for me, yes.


Alex

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking "has_work"

[David Hildenbrand]

> > We have
> > - wait (wait bit in PSW)
> > - disabled wait (wait bit and interrupt fencing in PSW)
> > - STOPPED (not related to PSW, state change usually handled via service processor or hypervisor)
> >
> > I think we have to differentiate between KVM/TCG. On KVM we always do in kernel halt and qemu sees a halted only for STOPPED or disabled wait. TCG has to take care of the normal wait as well.
> >
> >  From a first glimpse, a disabled wait and STOPPED look similar, but there are (important) differences, e.g. other CPUs get a different a different result from a SIGP SENSE. This makes a big difference, e.g. for Linux guests, that send a SIGP STOP, followed by a SIGP SENSE loop until the CPU is down on hotplug (and shutdown, kexec..) So I think we agree, that handling the cpu states natively makes sense.
> >
> > The question is now only how to model it correctly without breaking TCG/KVM and reuse as much common code as possible. Correct?
> >
> > Do I understand you correctly, that your collapsing of stopped and halted is only in the qemu coding sense, IOW maybe we could just modify kvm_arch_process_async_events to consider the STOPPED state, as TCGs sigp implementation does not support SMP anyway?
> 
> That works for me, yes.
> 
> 
> Alex
> 

I had a look at it yesterday and it seems like we can totally drop this patch:

1. TCG doesn't support multiple CPUs and the TCG SIGP implementation isn't
ready for proper STOP/START/SENSE. Testing for STOPPED cpus in cpu_has_work()
can be dropped. To be able to support TCG was the main reason for this patch -
as we don't want to do so for now, we can leave it as is. We can still decide
to support the cpu states later using a mechanism suggest by Alex
(interrupt_requests).

Even if cpu_has_work() would make cpu.c:cpu_thread_is_idle() return false,
kvm_arch_process_async_events() called by kvm-all.c:kvm_cpu_exec() would make
it go back to sleep. Therefore a stopped VCPU will never be able to run in the
KVM case (because it always has cs->halted = true).

2. The unhalt in kvm_arch_process_async_events is for a special case where a
VCPU is in disabled wait and receives e.g. a machine-check interrupt. These
might happen in the future, for now we will never see them (the only
way to get a vcpu out of disabled wait are SIGP RESTART/CPU RESET - so we
don't break anything at that point).

David

[Qemu-devel] [PATCH/RFC 5/5] s390x/kvm: propagate s390 cpu state to kvm

[Christian Borntraeger]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

Let QEMU propagate the cpu state to kvm. If kvm doesn't yet support it, it is
silently ignored as kvm will still handle the cpu state itself in that case.

The state is not synced back, thus kvm won't have a chance to actively modify
the cpu state. To do so, control has to be given back to QEMU (which is already
done so in all relevant cases).

Setting of the cpu state can fail either because kvm doesn't support the
interface yet, or because the state is invalid/not supported. Failed attempts
will be traced

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-s390x/cpu.c |  3 +++
 target-s390x/cpu.h |  5 +++++
 target-s390x/kvm.c | 33 +++++++++++++++++++++++++++++++++
 trace-events       |  1 +
 4 files changed, 42 insertions(+)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 1d32f5a..cf62ebd 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -289,6 +289,9 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
                      cpu_state);
         exit(1);
     }
+    if (kvm_enabled() && cpu->env.cpu_state != cpu_state) {
+        kvm_s390_set_cpu_state(cpu, cpu_state);
+    }
     cpu->env.cpu_state = cpu_state;
 
     return s390_count_running_cpus();
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 892726c..ce4a6ca 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -1088,6 +1088,7 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
                                     int vq, bool assign);
 int kvm_s390_cpu_restart(S390CPU *cpu);
 void kvm_s390_clear_cmma_callback(void *opaque);
+int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state);
 #else
 static inline void kvm_s390_io_interrupt(uint16_t subchannel_id,
                                         uint16_t subchannel_nr,
@@ -1114,6 +1115,10 @@ static inline int kvm_s390_cpu_restart(S390CPU *cpu)
 static inline void kvm_s390_clear_cmma_callback(void *opaque)
 {
 }
+static inline int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
+{
+    return -ENOSYS;
+}
 #endif
 
 static inline void cmma_reset(S390CPU *cpu)
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 00125f1..d8ecd0b 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -1289,3 +1289,36 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
     }
     return kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &kick);
 }
+
+int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
+{
+    struct kvm_mp_state mp_state = {};
+    int ret;
+
+    switch (cpu_state) {
+    case CPU_STATE_STOPPED:
+        mp_state.mp_state = KVM_MP_STATE_STOPPED;
+        break;
+    case CPU_STATE_CHECK_STOP:
+        mp_state.mp_state = KVM_MP_STATE_CHECK_STOP;
+        break;
+    case CPU_STATE_OPERATING:
+        mp_state.mp_state = KVM_MP_STATE_OPERATING;
+        break;
+    case CPU_STATE_LOAD:
+        mp_state.mp_state = KVM_MP_STATE_LOAD;
+        break;
+    default:
+        error_report("Requested CPU state is not a valid S390 CPU state: %u",
+                     cpu_state);
+        exit(1);
+    }
+
+    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
+    if (ret) {
+        trace_kvm_failed_cpu_state_set(CPU(cpu)->cpu_index, cpu_state,
+                                       strerror(-ret));
+    }
+
+    return ret;
+}
diff --git a/trace-events b/trace-events
index c652606..56a7dab 100644
--- a/trace-events
+++ b/trace-events
@@ -1301,6 +1301,7 @@ mhp_pc_dimm_assigned_address(uint64_t addr) "0x%"PRIx64
 # target-s390x/kvm.c
 kvm_enable_cmma(int rc) "CMMA: enabling with result code %d"
 kvm_clear_cmma(int rc) "CMMA: clearing with result code %d"
+kvm_failed_cpu_state_set(int cpu_index, uint8_t state, const char *msg) "Warning: Unable to set cpu %d state %" PRIu8 " to KVM: %s"
 
 # target-s390x/cpu.c
 cpu_set_state(int cpu_index, uint8_t state) "setting cpu %d state to %" PRIu8
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[David Hildenbrand]

> This is the qemu part of kernel series "Let user space control the
> cpu states"
> 
> Christian Borntraeger (1):
>   update linux headers with with cpustate changes
> 
> David Hildenbrand (4):
>   s390x/kvm: introduce proper states for s390 cpus
>   s390x/kvm: proper use of the cpu states OPERATING and STOPPED
>   s390x/kvm: test whether a cpu is STOPPED when checking "has_work"
>   s390x/kvm: propagate s390 cpu state to kvm
> 
>  hw/s390x/ipl.c            |   2 +-
>  hw/s390x/s390-virtio.c    |  32 --------------
>  linux-headers/linux/kvm.h |   7 ++-
>  target-s390x/cpu.c        | 106 +++++++++++++++++++++++++++++++++++++++-------
>  target-s390x/cpu.h        |  33 +++++++++++++--
>  target-s390x/helper.c     |  11 ++---
>  target-s390x/kvm.c        |  49 ++++++++++++++++++---
>  trace-events              |   6 +++
>  8 files changed, 179 insertions(+), 67 deletions(-)
> 

Looks good to me.

Re: [Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[David Hildenbrand]

> This is the qemu part of kernel series "Let user space control the
> cpu states"
> 
> Christian Borntraeger (1):
>   update linux headers with with cpustate changes
> 
> David Hildenbrand (4):
>   s390x/kvm: introduce proper states for s390 cpus
>   s390x/kvm: proper use of the cpu states OPERATING and STOPPED
>   s390x/kvm: test whether a cpu is STOPPED when checking "has_work"
>   s390x/kvm: propagate s390 cpu state to kvm
> 
>  hw/s390x/ipl.c            |   2 +-
>  hw/s390x/s390-virtio.c    |  32 --------------
>  linux-headers/linux/kvm.h |   7 ++-
>  target-s390x/cpu.c        | 106 +++++++++++++++++++++++++++++++++++++++-------
>  target-s390x/cpu.h        |  33 +++++++++++++--
>  target-s390x/helper.c     |  11 ++---
>  target-s390x/kvm.c        |  49 ++++++++++++++++++---
>  trace-events              |   6 +++
>  8 files changed, 179 insertions(+), 67 deletions(-)
> 

Looks good to me

Re: [Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[David Hildenbrand]

> > This is the qemu part of kernel series "Let user space control the
> > cpu states"
> > 
> > Christian Borntraeger (1):
> >   update linux headers with with cpustate changes
> > 
> > David Hildenbrand (4):
> >   s390x/kvm: introduce proper states for s390 cpus
> >   s390x/kvm: proper use of the cpu states OPERATING and STOPPED
> >   s390x/kvm: test whether a cpu is STOPPED when checking "has_work"
> >   s390x/kvm: propagate s390 cpu state to kvm
> > 
> >  hw/s390x/ipl.c            |   2 +-
> >  hw/s390x/s390-virtio.c    |  32 --------------
> >  linux-headers/linux/kvm.h |   7 ++-
> >  target-s390x/cpu.c        | 106 +++++++++++++++++++++++++++++++++++++++-------
> >  target-s390x/cpu.h        |  33 +++++++++++++--
> >  target-s390x/helper.c     |  11 ++---
> >  target-s390x/kvm.c        |  49 ++++++++++++++++++---
> >  trace-events              |   6 +++
> >  8 files changed, 179 insertions(+), 67 deletions(-)
> > 
> 
> Looks good to me
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

@all thought it was the final internal review :)

Re: [Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[Alexander Graf]

On 10.07.14 15:27, David Hildenbrand wrote:
>>> This is the qemu part of kernel series "Let user space control the
>>> cpu states"
>>>
>>> Christian Borntraeger (1):
>>>    update linux headers with with cpustate changes
>>>
>>> David Hildenbrand (4):
>>>    s390x/kvm: introduce proper states for s390 cpus
>>>    s390x/kvm: proper use of the cpu states OPERATING and STOPPED
>>>    s390x/kvm: test whether a cpu is STOPPED when checking "has_work"
>>>    s390x/kvm: propagate s390 cpu state to kvm
>>>
>>>   hw/s390x/ipl.c            |   2 +-
>>>   hw/s390x/s390-virtio.c    |  32 --------------
>>>   linux-headers/linux/kvm.h |   7 ++-
>>>   target-s390x/cpu.c        | 106 +++++++++++++++++++++++++++++++++++++++-------
>>>   target-s390x/cpu.h        |  33 +++++++++++++--
>>>   target-s390x/helper.c     |  11 ++---
>>>   target-s390x/kvm.c        |  49 ++++++++++++++++++---
>>>   trace-events              |   6 +++
>>>   8 files changed, 179 insertions(+), 67 deletions(-)
>>>
>> Looks good to me
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> @all thought it was the final internal review :)

It's a perfectly good thing to say "looks good to me" in public too. The 
only major difference is that usually you would say "reviewed-by" ;).


Alex

Re: [Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[Alexander Graf]

On 28.07.2014, at 15:43, Alexander Graf <agraf@suse.de> wrote:

> 
> On 10.07.14 15:27, David Hildenbrand wrote:
>>>> This is the qemu part of kernel series "Let user space control the
>>>> cpu states"
>>>> 
>>>> Christian Borntraeger (1):
>>>>   update linux headers with with cpustate changes
>>>> 
>>>> David Hildenbrand (4):
>>>>   s390x/kvm: introduce proper states for s390 cpus
>>>>   s390x/kvm: proper use of the cpu states OPERATING and STOPPED
>>>>   s390x/kvm: test whether a cpu is STOPPED when checking "has_work"
>>>>   s390x/kvm: propagate s390 cpu state to kvm
>>>> 
>>>>  hw/s390x/ipl.c            |   2 +-
>>>>  hw/s390x/s390-virtio.c    |  32 --------------
>>>>  linux-headers/linux/kvm.h |   7 ++-
>>>>  target-s390x/cpu.c        | 106 +++++++++++++++++++++++++++++++++++++++-------
>>>>  target-s390x/cpu.h        |  33 +++++++++++++--
>>>>  target-s390x/helper.c     |  11 ++---
>>>>  target-s390x/kvm.c        |  49 ++++++++++++++++++---
>>>>  trace-events              |   6 +++
>>>>  8 files changed, 179 insertions(+), 67 deletions(-)
>>>> 
>>> Looks good to me
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> 
>> @all thought it was the final internal review :)
> 
> It's a perfectly good thing to say "looks good to me" in public too. The only major difference is that usually you would say "reviewed-by" ;).

Meh - only realized after I sent this that all those patches are From: you. Then of course it's not useful ;)


Alex