s390x: TOD refactoring + TCG CPU hotplug support

Big values for the TOD/ns clock can result in some overflows that can be avoided. Not all overflows can be handled however, as the conversion either multiplies by 4.096 or divided by 4.096.

Apply the trick used in the Linux kernel in arch/s390/include/asm/timex.h for tod_to_ns() and use the same trick also for the conversion in the other direction.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/internal.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

target/s390x/internal.h +2 -1

index e392a02d12..6cf63340bf 100644
@@ -243,13 +243,14 @@ enum cc_op {
 /* Converts ns to s390's clock format */
 static inline uint64_t time2tod(uint64_t ns)
 {
-    return (ns << 9) / 125;
+    return (ns << 9) / 125 + (((ns & 0xff10000000000000ull) / 125) << 9);
+

target/s390x/cpu.c +0 -0

         }
     }
     /* Fixme TCG */

target/s390x/cpu.c int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)

Quoted 18 lines · On Mon, 25 Jun 2018 13:53:45 +0200 David Hildenbrand <[email protected]> wrote:

We are going to factor out the TOD into a separate device and use const pointers for device class functions where possible. We are passing right now ordinary pointers that should never be touched when setting the TOD. Let's just pass the values directly.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu.c       |  4 ++--
 target/s390x/kvm-stub.c  |  4 ++--
 target/s390x/kvm.c       | 12 ++++++------
 target/s390x/kvm_s390x.h |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

target/s390x/cpu.c +0 -0

index c268065887..68512e3e54 100644
@@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)

Any reason why you keep the pointers here?

target/s390x/cpu.c int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low) · line 418

Quoted 8 lines

target/s390x/cpu.c +2 -2

     int r = 0;
 
     if (kvm_enabled()) {
-        r = kvm_s390_set_clock_ext(tod_high, tod_low);
+        r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
         if (r == -ENXIO) {
-            return kvm_s390_set_clock(tod_high, tod_low);
+            return kvm_s390_set_clock(*tod_high, *tod_low);

Especially as it would be more clean to check for !NULL before dereferencing...

Quoted 4 lines

target/s390x/cpu.c +0 -0

         }
     }
     /* Fixme TCG */

Quoted 33 lines · Cornelia Huck

target/s390x/cpu.c int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)

Quoted 18 lines · On Mon, 25 Jun 2018 13:53:45 +0200 David Hildenbrand <[email protected]> wrote:

We are going to factor out the TOD into a separate device and use const pointers for device class functions where possible. We are passing right now ordinary pointers that should never be touched when setting the TOD. Let's just pass the values directly.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu.c       |  4 ++--
 target/s390x/kvm-stub.c  |  4 ++--
 target/s390x/kvm.c       | 12 ++++++------
 target/s390x/kvm_s390x.h |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

target/s390x/cpu.c +0 -0

index c268065887..68512e3e54 100644
@@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)  

Any reason why you keep the pointers here?

Quoted 8 lines

target/s390x/cpu.c +2 -2

     int r = 0;
 
     if (kvm_enabled()) {
-        r = kvm_s390_set_clock_ext(tod_high, tod_low);
+        r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
         if (r == -ENXIO) {
-            return kvm_s390_set_clock(tod_high, tod_low);
+            return kvm_s390_set_clock(*tod_high, *tod_low);  

Quoted 2 lines

Especially as it would be more clean to check for !NULL before dereferencing...

See the next patch :)

(I assume that refactoring code in order to rip it out does not make sense)

Quoted 37 lines · On Mon, 25 Jun 2018 17:54:42 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 35 lines · Cornelia Huck

target/s390x/cpu.c int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)

Quoted 18 lines · On Mon, 25 Jun 2018 13:53:45 +0200 David Hildenbrand <[email protected]> wrote:

We are going to factor out the TOD into a separate device and use const pointers for device class functions where possible. We are passing right now ordinary pointers that should never be touched when setting the TOD. Let's just pass the values directly.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu.c       |  4 ++--
 target/s390x/kvm-stub.c  |  4 ++--
 target/s390x/kvm.c       | 12 ++++++------
 target/s390x/kvm_s390x.h |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

target/s390x/cpu.c +0 -0

index c268065887..68512e3e54 100644
@@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)  

Any reason why you keep the pointers here?

target/s390x/cpu.c int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low) · line 418

Quoted 8 lines

target/s390x/cpu.c +2 -2

     int r = 0;
 
     if (kvm_enabled()) {
-        r = kvm_s390_set_clock_ext(tod_high, tod_low);
+        r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
         if (r == -ENXIO) {
-            return kvm_s390_set_clock(tod_high, tod_low);
+            return kvm_s390_set_clock(*tod_high, *tod_low);  

Especially as it would be more clean to check for !NULL before dereferencing...

Quoted 3 lines

See the next patch :)

(I assume that refactoring code in order to rip it out does not make sense)

Add a comment in the commit message?

"Note that s390_set_clock() will be removed in a follow-on patch and therefore its calling convention is not changed."

Quoted 44 lines · Cornelia Huck

Quoted 40 lines · On Mon, 25 Jun 2018 17:54:42 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 33 lines · Cornelia Huck

target/s390x/cpu.c int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)

Quoted 18 lines · On Mon, 25 Jun 2018 13:53:45 +0200 David Hildenbrand <[email protected]> wrote:

We are going to factor out the TOD into a separate device and use const pointers for device class functions where possible. We are passing right now ordinary pointers that should never be touched when setting the TOD. Let's just pass the values directly.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu.c       |  4 ++--
 target/s390x/kvm-stub.c  |  4 ++--
 target/s390x/kvm.c       | 12 ++++++------
 target/s390x/kvm_s390x.h |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

target/s390x/cpu.c +0 -0

index c268065887..68512e3e54 100644
@@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)    

Any reason why you keep the pointers here?

target/s390x/cpu.c int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low) · line 418

Quoted 8 lines

target/s390x/cpu.c +2 -2

     int r = 0;
 
     if (kvm_enabled()) {
-        r = kvm_s390_set_clock_ext(tod_high, tod_low);
+        r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
         if (r == -ENXIO) {
-            return kvm_s390_set_clock(tod_high, tod_low);
+            return kvm_s390_set_clock(*tod_high, *tod_low);    

Quoted 2 lines

Especially as it would be more clean to check for !NULL before dereferencing...

See the next patch :)

(I assume that refactoring code in order to rip it out does not make sense)

Quoted 5 lines

Add a comment in the commit message?

"Note that s390_set_clock() will be removed in a follow-on patch and therefore its calling convention is not changed."

Sure I can do that. Thanks!

Right now, each CPU has its own TOD. Especially, the TOD will differ based on creation time of a CPU - e.g. when hotplugging a CPU the times will differ quite a lot, resulting in stall warnings in the guest.

Let's use a single TOD by implementing our new TOD device. Prepare it for TOD-clock epoch extension.

Most importantly, whenever we set the TOD, we have to update the CKC timer.

Introduce "tcg_s390x.h" just like "kvm_s390x.h" for tcg specific function declarations that should not go into cpu.h.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/tod-qemu.c        | 46 ++++++++++++++++++++++++++++++++++----
 hw/s390x/tod.c             | 11 +++++++++
 include/hw/s390x/tod.h     | 19 ++++++++++++++++
 target/s390x/cpu.c         |  7 ------
 target/s390x/cpu.h         |  1 -
 target/s390x/internal.h    | 16 -------------
 target/s390x/misc_helper.c | 30 ++++++++++++++++++++-----
 target/s390x/tcg_s390x.h   | 18 +++++++++++++++
 8 files changed, 114 insertions(+), 34 deletions(-)
 create mode 100644 target/s390x/tcg_s390x.h

hw/s390x/tod-qemu.c +10 -3

index 03ea1ce4e8..941e84693e 100644
@@ -11,19 +11,43 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/s390x/tod.h"
+#include "qemu/timer.h"
+#include "qemu/cutils.h"
+#include "cpu.h"
+#include "tcg_s390x.h"
 
 static void qemu_s390_tod_get(const S390TODState *td, S390TOD *tod,
                               Error **errp)
 {
-    /* FIXME */
-    tod->high = 0;
-    tod->low = 0;
+    *tod = td->base;
+
+    tod->low += time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+    if (tod->low < td->base.low) {
+        tod->high++;
+    }
 }
 

hw/s390x/tod-qemu.c +14 -0

+        td->base.high--;
+    }
+
+    /*
+     * The TOD has been changed and we have to recalculate the CKC values
+     * for all CPUs. We do this asynchronously, as "SET CLOCK should be
+     * issued only while all other activity on all CPUs .. has been
+     * suspended".
+     */
+    CPU_FOREACH(cpu) {
+        async_run_on_cpu(cpu, tcg_s390_tod_updated, RUN_ON_CPU_NULL);
+    }
 }
 
 static void qemu_s390_tod_class_init(ObjectClass *oc, void *data)

@@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data)
     tdc->set = qemu_s390_tod_set;
 }
 
+static void qemu_s390_tod_init(Object *obj)
+{

hw/s390x/tod-qemu.c +1 -0

 static TypeInfo qemu_s390_tod_info = {
     .name = TYPE_QEMU_S390_TOD,
     .parent = TYPE_S390_TOD,
     .instance_size = sizeof(S390TODState),
+    .instance_init = qemu_s390_tod_init,
     .class_init = qemu_s390_tod_class_init,
     .class_size = sizeof(S390TODClass),
 };

target/s390x/misc_helper.c +10 -5

index dd5273949b..be341b5295 100644
@@ -28,6 +28,8 @@
 #include "qemu/timer.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
+#include "qapi/error.h"
+#include "tcg_s390x.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "sysemu/cpus.h"

@@ -39,6 +41,7 @@
 #include "hw/s390x/ioinst.h"
 #include "hw/s390x/s390-pci-inst.h"
 #include "hw/boards.h"
+#include "hw/s390x/tod.h"
 #endif
 
 /* #define DEBUG_HELPER */

@@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1)
 /* Store Clock */
 uint64_t HELPER(stck)(CPUS390XState *env)
 {
-    uint64_t time;
+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod;
 
-    time = env->tod_offset +
-        time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
-
-    return time;
+    tdc->get(td, &tod, &error_abort);
+    return tod.low;
 }
 
 /* Set Clock Comparator */
 void HELPER(sckc)(CPUS390XState *env, uint64_t time)
 {
+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);

hw/s390x/tod-qemu.c line 38

Quoted 4 lines

hw/s390x/tod-qemu.c +4 -0

+    td->base = *tod;
+
+    td->base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+    if (tod->low < td->base.low) {

Just a matter of taste, but I'd rather use "td->base.low > tod->low" here instead (since the operations before and after the if-statement are related to td->base).

Quoted 20 lines

hw/s390x/tod-qemu.c +12 -0

+        td->base.high--;
+    }
+
+    /*
+     * The TOD has been changed and we have to recalculate the CKC values
+     * for all CPUs. We do this asynchronously, as "SET CLOCK should be
+     * issued only while all other activity on all CPUs .. has been
+     * suspended".
+     */
+    CPU_FOREACH(cpu) {
+        async_run_on_cpu(cpu, tcg_s390_tod_updated, RUN_ON_CPU_NULL);
+    }
 }
 
 static void qemu_s390_tod_class_init(ObjectClass *oc, void *data)

@@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data)
     tdc->set = qemu_s390_tod_set;
 }
 

hw/s390x/tod-qemu.c static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) · line 72

Quoted 12 lines

hw/s390x/tod-qemu.c +12 -0

+static void qemu_s390_tod_init(Object *obj)
+{
+    S390TODState *td = S390_TOD(obj);
+    struct tm tm;
+
+    qemu_get_timedate(&tm, 0);
+    td->base.high = 0;
+    td->base.low = TOD_UNIX_EPOCH + (time2tod(mktimegm(&tm)) * 1000000000ULL);
+    if (td->base.low < TOD_UNIX_EPOCH) {
+        td->base.high += 1;
+    }
+}

Nit: It would be sufficient to do this in the realize() function instead.

Quoted 9 lines

hw/s390x/tod-qemu.c +1 -0

 static TypeInfo qemu_s390_tod_info = {
     .name = TYPE_QEMU_S390_TOD,
     .parent = TYPE_S390_TOD,
     .instance_size = sizeof(S390TODState),
+    .instance_init = qemu_s390_tod_init,
     .class_init = qemu_s390_tod_class_init,
     .class_size = sizeof(S390TODClass),
 };

[...]

Quoted 49 lines

target/s390x/misc_helper.c +12 -5

index dd5273949b..be341b5295 100644
@@ -28,6 +28,8 @@
 #include "qemu/timer.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
+#include "qapi/error.h"
+#include "tcg_s390x.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "sysemu/cpus.h"

@@ -39,6 +41,7 @@
 #include "hw/s390x/ioinst.h"
 #include "hw/s390x/s390-pci-inst.h"
 #include "hw/boards.h"
+#include "hw/s390x/tod.h"
 #endif
 
 /* #define DEBUG_HELPER */

@@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1)
 /* Store Clock */
 uint64_t HELPER(stck)(CPUS390XState *env)
 {
-    uint64_t time;
+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod;
 
-    time = env->tod_offset +
-        time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
-
-    return time;
+    tdc->get(td, &tod, &error_abort);
+    return tod.low;
 }
 
 /* Set Clock Comparator */
 void HELPER(sckc)(CPUS390XState *env, uint64_t time)
 {
+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod_base;
+
     if (time == -1ULL) {
         return;
     }
 

target/s390x/misc_helper.c void HELPER(spx)(CPUS390XState *env, uint64_t a1) · line 166

Quoted 4 lines

target/s390x/misc_helper.c +2 -0

     env->ckc = time;
 
+    tdc->get(td, &tod_base, &error_abort);
+    tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));

So the tdc->get first adds the time2tod, and then you subtract it here again? Can't you simply use td->base.low directly instead?

target/s390x/misc_helper.c void HELPER(sckc)(CPUS390XState *env, uint64_t time) · line 175

Quoted 10 lines

target/s390x/misc_helper.c +1 -1

     /* difference between origins */
-    time -= env->tod_offset;
+    time -= tod_base.low;
 
     /* nanoseconds */
     time = tod2time(time);

@@ -164,6 +174,14 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     timer_mod(env->tod_timer, time);
 }

... I found only nits, so with or without my suggested modifications:

Reviewed-by Thomas Huth <[email protected]>

Quoted 4 lines

Quoted 4 lines

hw/s390x/tod-qemu.c +0 -0

@@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data)
     tdc->set = qemu_s390_tod_set;
 }
 

Quoted 14 lines

hw/s390x/tod-qemu.c static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) · line 72

Quoted 12 lines

hw/s390x/tod-qemu.c +12 -0

+static void qemu_s390_tod_init(Object *obj)
+{
+    S390TODState *td = S390_TOD(obj);
+    struct tm tm;
+
+    qemu_get_timedate(&tm, 0);
+    td->base.high = 0;
+    td->base.low = TOD_UNIX_EPOCH + (time2tod(mktimegm(&tm)) * 1000000000ULL);
+    if (td->base.low < TOD_UNIX_EPOCH) {
+        td->base.high += 1;
+    }
+}  

Nit: It would be sufficient to do this in the realize() function instead.

Then I'll have to overwrite the realize function and store the parent_realize function - something that I want to avoid if not really necessary.

(for now it was also done in the cpu initfn, so that should be fine)

Quoted 59 lines

Quoted 9 lines

hw/s390x/tod-qemu.c +1 -0

 static TypeInfo qemu_s390_tod_info = {
     .name = TYPE_QEMU_S390_TOD,
     .parent = TYPE_S390_TOD,
     .instance_size = sizeof(S390TODState),
+    .instance_init = qemu_s390_tod_init,
     .class_init = qemu_s390_tod_class_init,
     .class_size = sizeof(S390TODClass),
 };  

[...]

Quoted 49 lines

target/s390x/misc_helper.c +12 -5

index dd5273949b..be341b5295 100644
@@ -28,6 +28,8 @@
 #include "qemu/timer.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
+#include "qapi/error.h"
+#include "tcg_s390x.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "sysemu/cpus.h"

@@ -39,6 +41,7 @@
 #include "hw/s390x/ioinst.h"
 #include "hw/s390x/s390-pci-inst.h"
 #include "hw/boards.h"
+#include "hw/s390x/tod.h"
 #endif
 
 /* #define DEBUG_HELPER */

@@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1)
 /* Store Clock */
 uint64_t HELPER(stck)(CPUS390XState *env)
 {
-    uint64_t time;
+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod;
 
-    time = env->tod_offset +
-        time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
-
-    return time;
+    tdc->get(td, &tod, &error_abort);
+    return tod.low;
 }
 
 /* Set Clock Comparator */
 void HELPER(sckc)(CPUS390XState *env, uint64_t time)
 {
+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod_base;
+
     if (time == -1ULL) {
         return;
     }
 

Quoted 7 lines

target/s390x/misc_helper.c void HELPER(spx)(CPUS390XState *env, uint64_t a1) · line 166

Quoted 4 lines

target/s390x/misc_helper.c +2 -0

     env->ckc = time;
 
+    tdc->get(td, &tod_base, &error_abort);
+    tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));  

So the tdc->get first adds the time2tod, and then you subtract it here again? Can't you simply use td->base.low directly instead?

That might be a good idea, previously I had tdc->get_base(), but dropped it because it cannot be implemented for KVM.

Simply accessing the member here should be fine. Thanks!

Quoted 16 lines

target/s390x/misc_helper.c void HELPER(sckc)(CPUS390XState *env, uint64_t time) · line 175

Quoted 11 lines

target/s390x/misc_helper.c +1 -1

     /* difference between origins */
-    time -= env->tod_offset;
+    time -= tod_base.low;
 
     /* nanoseconds */
     time = tod2time(time);

@@ -164,6 +174,14 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     timer_mod(env->tod_timer, time);
 }  

... I found only nits, so with or without my suggested modifications:

Reviewed-by Thomas Huth <[email protected]>

Quoted 19 lines · On Tue, 26 Jun 2018 14:06:00 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 4 lines

Quoted 4 lines

hw/s390x/tod-qemu.c +0 -0

@@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data)
     tdc->set = qemu_s390_tod_set;
 }
 

Quoted 14 lines

hw/s390x/tod-qemu.c static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) · line 72

Quoted 12 lines

hw/s390x/tod-qemu.c +12 -0

+static void qemu_s390_tod_init(Object *obj)
+{
+    S390TODState *td = S390_TOD(obj);
+    struct tm tm;
+
+    qemu_get_timedate(&tm, 0);
+    td->base.high = 0;
+    td->base.low = TOD_UNIX_EPOCH + (time2tod(mktimegm(&tm)) * 1000000000ULL);
+    if (td->base.low < TOD_UNIX_EPOCH) {
+        td->base.high += 1;
+    }
+}  

Nit: It would be sufficient to do this in the realize() function instead.

Quoted 3 lines

Then I'll have to overwrite the realize function and store the parent_realize function - something that I want to avoid if not really necessary.

Agreed. I'd just leave it as it is now.

Quoted 70 lines

(for now it was also done in the cpu initfn, so that should be fine)

Quoted 66 lines

Quoted 9 lines

hw/s390x/tod-qemu.c +1 -0

 static TypeInfo qemu_s390_tod_info = {
     .name = TYPE_QEMU_S390_TOD,
     .parent = TYPE_S390_TOD,
     .instance_size = sizeof(S390TODState),
+    .instance_init = qemu_s390_tod_init,
     .class_init = qemu_s390_tod_class_init,
     .class_size = sizeof(S390TODClass),
 };  

[...]

Quoted 43 lines

target/s390x/misc_helper.c +10 -5

index dd5273949b..be341b5295 100644
@@ -28,6 +28,8 @@
 #include "qemu/timer.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
+#include "qapi/error.h"
+#include "tcg_s390x.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "sysemu/cpus.h"

@@ -39,6 +41,7 @@
 #include "hw/s390x/ioinst.h"
 #include "hw/s390x/s390-pci-inst.h"
 #include "hw/boards.h"
+#include "hw/s390x/tod.h"
 #endif
 
 /* #define DEBUG_HELPER */

@@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1)
 /* Store Clock */
 uint64_t HELPER(stck)(CPUS390XState *env)
 {
-    uint64_t time;
+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod;
 
-    time = env->tod_offset +
-        time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
-
-    return time;
+    tdc->get(td, &tod, &error_abort);
+    return tod.low;
 }
 
 /* Set Clock Comparator */
 void HELPER(sckc)(CPUS390XState *env, uint64_t time)
 {
+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);

target/s390x/misc_helper.c void HELPER(spx)(CPUS390XState *env, uint64_t a1) · line 166

Quoted 10 lines

target/s390x/misc_helper.c +4 -0

+    S390TOD tod_base;
+
     if (time == -1ULL) {
         return;
     }
 
     env->ckc = time;
 
+    tdc->get(td, &tod_base, &error_abort);
+    tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));  

So the tdc->get first adds the time2tod, and then you subtract it here again? Can't you simply use td->base.low directly instead?

Quoted 4 lines

That might be a good idea, previously I had tdc->get_base(), but dropped it because it cannot be implemented for KVM.

Simply accessing the member here should be fine. Thanks!

So, we're guaranteed to have td->base.low at the correct value? (Sorry, having a bit of a hard time following around here.)

Quoted 27 lines · Cornelia Huck

Quoted 23 lines · On Tue, 26 Jun 2018 14:06:00 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 11 lines

Quoted 11 lines

/* Set Clock Comparator */
void HELPER(sckc)(CPUS390XState *env, uint64_t time) {

target/s390x/misc_helper.c +4 -0

+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod_base;
+
     if (time == -1ULL) {
         return;
     }

Quoted 7 lines

target/s390x/misc_helper.c void HELPER(spx)(CPUS390XState *env, uint64_t a1) · line 166

Quoted 4 lines

     env->ckc = time;

target/s390x/misc_helper.c +2 -0

+    tdc->get(td, &tod_base, &error_abort);
+    tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));    

So the tdc->get first adds the time2tod, and then you subtract it here again? Can't you simply use td->base.low directly instead?

That might be a good idea, previously I had tdc->get_base(), but dropped it because it cannot be implemented for KVM.

Simply accessing the member here should be fine. Thanks!

Quoted 3 lines

So, we're guaranteed to have td->base.low at the correct value? (Sorry, having a bit of a hard time following around here.)

Yes, just verified, changed and tested. We will have the value at that place.

I'll give some more time to review the other parts, then I can resend.

Let's stop the timer and delete any pending CKC IRQ before doing anything else.

While at it, add a comment while the check for ckc == -1ULL is needed.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/misc_helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

target/s390x/misc_helper.c +7 -0

index be341b5295..4872bdd774 100644
@@ -156,6 +156,13 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     S390TODClass *tdc = S390_TOD_GET_CLASS(td);
     S390TOD tod_base;
 
+    /* stop the timer and remove pending CKC IRQs */
+    timer_del(env->tod_timer);
+    qemu_mutex_lock_iothread();
+    env->pending_int &= ~INTERRUPT_EXT_CLOCK_COMPARATOR;
+    qemu_mutex_unlock_iothread();
+
+    /* the tod has to exceed the ckc, this can never happen if ckc is all 1's */
     if (time == -1ULL) {
         return;
     }

Let's stop the timer and delete any pending CKC IRQ before doing anything else.

While at it, add a comment while the check for ckc == -1ULL is needed.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/misc_helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

target/s390x/misc_helper.c +0 -0

index be341b5295..4872bdd774 100644
@@ -156,6 +156,13 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     S390TODClass *tdc = S390_TOD_GET_CLASS(td);
     S390TOD tod_base;
 

This allows a guest to change its TOD. We already take care of updating all CKC timers from within S390TODClass.

Use MO_ALIGN to load the operand manually - this will properly trigger a SPECIFICATION exception.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/helper.h      |  1 +
 target/s390x/insn-data.def |  3 +--
 target/s390x/misc_helper.c | 16 ++++++++++++++++
 target/s390x/translate.c   |  9 +++++++++
 4 files changed, 27 insertions(+), 2 deletions(-)

target/s390x/helper.h +1 -0

index 59cba86a27..97c60ca7bc 100644
@@ -127,6 +127,7 @@ DEF_HELPER_4(diag, void, env, i32, i32, i32)
 DEF_HELPER_3(load_psw, noreturn, env, i64, i64)
 DEF_HELPER_FLAGS_2(spx, TCG_CALL_NO_RWG, void, env, i64)
 DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
+DEF_HELPER_FLAGS_2(sck, TCG_CALL_NO_RWG, i32, env, i64)
 DEF_HELPER_FLAGS_2(sckc, TCG_CALL_NO_RWG, void, env, i64)
 DEF_HELPER_FLAGS_2(sckpf, TCG_CALL_NO_RWG, void, env, i64)
 DEF_HELPER_FLAGS_1(stckc, TCG_CALL_NO_RWG, i64, env)

target/s390x/insn-data.def +1 -2

index 157619403d..5c6f33ed9c 100644
@@ -997,8 +997,7 @@
 /* SET ADDRESS SPACE CONTROL FAST */
     C(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0)
 /* SET CLOCK */
-    /* ??? Not implemented - is it necessary? */
-    C(0xb204, SCK,     S,     Z,   0, 0, 0, 0, 0, 0)
+    C(0xb204, SCK,     S,     Z,   la2, 0, 0, 0, sck, 0)
 /* SET CLOCK COMPARATOR */
     C(0xb206, SCKC,    S,     Z,   0, m2_64a, 0, 0, sckc, 0)
 /* SET CLOCK PROGRAMMABLE FIELD */

target/s390x/misc_helper.c +16 -0

index 4872bdd774..a223970d1c 100644
@@ -189,6 +189,22 @@ void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque)
     helper_sckc(env, env->ckc);
 }
 
+/* Set Clock */
+uint32_t HELPER(sck)(CPUS390XState *env, uint64_t tod_low)
+{
+    S390TODState *td = s390_get_todstate();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod = {
+        .high = 0,
+        .low = tod_low,
+    };
+
+    qemu_mutex_lock_iothread();
+    tdc->set(td, &tod, &error_abort);
+    qemu_mutex_unlock_iothread();
+    return 0;
+}
+
 /* Set Tod Programmable Field */
 void HELPER(sckpf)(CPUS390XState *env, uint64_t r0)
 {

target/s390x/translate.c +2 -0

index fdfec7feba..57c03cbf58 100644
@@ -4016,6 +4016,15 @@ static DisasJumpType op_stcke(DisasContext *s, DisasOps *o)
     return DISAS_NEXT;
 }
 
+static DisasJumpType op_sck(DisasContext *s, DisasOps *o)
+{

run_on_cpu() doesn't seem to work reliably until the CPU has been fully created if the single-threaded TCG main loop is already running.

Therefore, hotplugging a CPU under single-threaded TCG does currently not work. We should use the direct call instead of going via run_on_cpu().

So let's use run_on_cpu() for KVM only - KVM requires it due to the initial CPU reset ioctl. As a nice side effect, we get rif of the ifdef.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

target/s390x/cpu.c +12 -5

index 40d6980229..271c5ce652 100644
@@ -218,11 +218,18 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 #endif
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
-#if !defined(CONFIG_USER_ONLY)
-    run_on_cpu(cs, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
-#else
-    cpu_reset(cs);
-#endif
+
+    /*
+     * KVM requires the initial CPU reset ioctl to be executed on the target
+     * CPU thread. CPU hotplug under single-threaded TCG will not work with
+     * run_on_cpu(), as run_on_cpu() will not work properly if called while
+     * the main thread is already running but the CPU hasn't been realized.
+     */
+    if (kvm_enabled()) {
+        run_on_cpu(cs, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
+    } else {
+        cpu_reset(cs);
+    }
 
     scc->parent_realize(dev, &err);
 out:

run_on_cpu() doesn't seem to work reliably until the CPU has been fully created if the single-threaded TCG main loop is already running.

Therefore, hotplugging a CPU under single-threaded TCG does currently not work. We should use the direct call instead of going via run_on_cpu().

So let's use run_on_cpu() for KVM only - KVM requires it due to the initial CPU reset ioctl. As a nice side effect, we get rif of the ifdef.