Skip to contentSkip to conversation

s390x: TOD refactoring + TCG CPU hotplug support

PATCH Superseded David Hildenbrand 4 participants 27 messages Jun 20, 2018 – Jun 21, 2018

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v2 cover s390x: TOD refactoring + TCG CPU hotplug support

The TOD in TCG is not handled correctly:
- each CPU has its own TOD based on CPU creation time vs. a system TOD
- TOD is not migrated
- TOD timer is not restarted during migration
- CKC interrupts/TOD timer is not cleared when resetting the CKC

This (and a cpu creation problem for single threaded TCG) currently made CPU hotplug under TCG not work. Now it's working

The fist patch also refactors TOD handling for KVM (moved into a new TOD device).

v1 -> v2:
- "s390x/tcg: properly implement the TOD" -- introduce "tcg_s390x.h" similar to "kvm_s390x.h" -- dropped one unnecessary include introduction
- "s390x/tcg: rearm the CKC timer during migration" -- introduce "tcg-stub.c" similar to "kvm-stub.c"

David Hildenbrand (7):
s390x/tod: factor out TOD into separate device s390x/tcg: drop tod_basetime
s390x/tcg: properly implement the TOD s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts s390x/tcg: implement SET CLOCK
s390x/tcg: rearm the CKC timer during migration s390x/tcg: fix CPU hotplug with single-threaded TCG

 hw/s390x/Makefile.objs     |   3 +
 hw/s390x/s390-virtio-ccw.c |  57 +---------------
 hw/s390x/tod-kvm.c         |  64 ++++++++++++++++++
 hw/s390x/tod-qemu.c        |  85 ++++++++++++++++++++++++
 hw/s390x/tod.c             | 131 +++++++++++++++++++++++++++++++++++++
 include/hw/s390x/tod.h     |  65 ++++++++++++++++++
 target/s390x/Makefile.objs |   1 +
 target/s390x/cpu.c         |  51 ++-------------
 target/s390x/cpu.h         |   4 --
 target/s390x/helper.h      |   1 +
 target/s390x/insn-data.def |   3 +-
 target/s390x/internal.h    |  15 -----
 target/s390x/kvm_s390x.h   |   2 +
 target/s390x/machine.c     |   6 ++
 target/s390x/misc_helper.c |  60 +++++++++++++++--
 target/s390x/tcg-stub.c    |  20 ++++++
 target/s390x/tcg_s390x.h   |  18 +++++
 target/s390x/translate.c   |   9 +++
 18 files changed, 468 insertions(+), 127 deletions(-)
 create mode 100644 hw/s390x/tod-kvm.c
 create mode 100644 hw/s390x/tod-qemu.c
 create mode 100644 hw/s390x/tod.c
 create mode 100644 include/hw/s390x/tod.h
 create mode 100644 target/s390x/tcg-stub.c
 create mode 100644 target/s390x/tcg_s390x.h
Signature · 1 line
2.17.1
#1 lore raw

PATCH v2 1/7 s390x/tod: factor out TOD into separate device

Let's treat this like a separate device. TCG will have to store the actual state/time later on.

Include cpu-qom.h in kvm_s390x.h (due to S390CPU) to compile tod-kvm.c.

---
 hw/s390x/Makefile.objs     |   3 +
 hw/s390x/s390-virtio-ccw.c |  57 +-----------------
 hw/s390x/tod-kvm.c         |  64 ++++++++++++++++++++
 hw/s390x/tod-qemu.c        |  47 +++++++++++++++
 hw/s390x/tod.c             | 120 +++++++++++++++++++++++++++++++++++++
 include/hw/s390x/tod.h     |  46 ++++++++++++++
 target/s390x/cpu.c         |  32 ----------
 target/s390x/cpu.h         |   2 -
 target/s390x/kvm_s390x.h   |   2 +
 9 files changed, 285 insertions(+), 88 deletions(-)
 create mode 100644 hw/s390x/tod-kvm.c
 create mode 100644 hw/s390x/tod-qemu.c
 create mode 100644 hw/s390x/tod.c
 create mode 100644 include/hw/s390x/tod.h
hw/s390x/Makefile.objs +3 -0
index dc704b57d6..93282f7c59 100644
@@ -14,6 +14,9 @@ obj-$(CONFIG_PCI) += s390-pci-bus.o s390-pci-inst.o
 obj-$(call lnot,$(CONFIG_PCI)) += s390-pci-stub.o
 obj-y += s390-skeys.o
 obj-y += s390-stattrib.o
+obj-y += tod.o
+obj-$(CONFIG_KVM) += tod-kvm.o
+obj-$(CONFIG_TCG) += tod-qemu.o
 obj-$(CONFIG_KVM) += s390-skeys-kvm.o
 obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
 obj-y += s390-ccw.o
hw/s390x/s390-virtio-ccw.c +3 -54

75 lines not on this page · raw message

hw/s390x/tod-kvm.c new file +64 -0

64 lines not on this page · raw message

hw/s390x/tod-qemu.c new file +47 -0

47 lines not on this page · raw message

hw/s390x/tod.c new file +120 -0

120 lines not on this page · raw message

include/hw/s390x/tod.h new file +46 -0

46 lines not on this page · raw message

target/s390x/cpu.c +0 -32
index c268065887..03ea6eafa7 100644
@@ -390,38 +390,6 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
     return s390_count_running_cpus();
 }
 
-int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
-{
-    int r = 0;
-
-    if (kvm_enabled()) {
-        r = kvm_s390_get_clock_ext(tod_high, tod_low);
-        if (r == -ENXIO) {
-            return kvm_s390_get_clock(tod_high, tod_low);
-        }
-    } else {
-        /* Fixme TCG */
-        *tod_high = 0;
-        *tod_low = 0;
-    }
-
-    return r;
-}
-
-int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
-{
-    int r = 0;
-
-    if (kvm_enabled()) {
-        r = kvm_s390_set_clock_ext(tod_high, tod_low);
-        if (r == -ENXIO) {
-            return kvm_s390_set_clock(tod_high, tod_low);
-        }
-    }
-    /* Fixme TCG */
-    return r;
-}
-
 int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
 {
     if (kvm_enabled()) {
target/s390x/cpu.h +0 -2
index 6629a533f3..ac51c17fb4 100644
@@ -714,8 +714,6 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg)
 
 
 /* cpu.c */
-int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low);
-int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low);
 void s390_crypto_reset(void);
 bool s390_get_squash_mcss(void);
 int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit);
target/s390x/kvm_s390x.h +2 -0
index c383bf4ee9..8567aa49f7 100644
@@ -10,6 +10,8 @@
 #ifndef KVM_S390X_H
 #define KVM_S390X_H
 
+#include "cpu-qom.h"
+
 struct kvm_s390_irq;
 
 void kvm_s390_floating_interrupt_legacy(struct kvm_s390_irq *irq);
Signature · 1 line
2.17.1
Reviewed-by on v2 1/7
#2 lore raw

Re: [Qemu-devel] [PATCH v2 1/7] s390x/tod: factor out TOD into separate device

Quoted 7 lines · David Hildenbrand

Let's treat this like a separate device. TCG will have to store the actual state/time later on.

Include cpu-qom.h in kvm_s390x.h (due to S390CPU) to compile tod-kvm.c.

---

[...]

hw/s390x/tod.c line 105

Quoted 12 lines
hw/s390x/tod.c new file +12 -0
+static void s390_tod_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->desc = "TOD (Time Of Day) Clock";
+    dc->realize = s390_tod_realize;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+
+    /* We only have one TOD clock in the system attached to the machine */
+    dc->user_creatable = false;
+    dc->hotpluggable = false;
+}

IIRC you don't need the hotpluggable = false if you already set user_creatable = false.

Quoted 38 lines
hw/s390x/tod.c new file +14 -0
+static TypeInfo s390_tod_info = {
+    .name = TYPE_S390_TOD,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(S390TODState),
+    .class_init = s390_tod_class_init,
+    .class_size = sizeof(S390TODClass),
+    .abstract = true,
+};
+
+static void register_types(void)
+{
+    type_register_static(&s390_tod_info);
+}
+type_init(register_types);
include/hw/s390x/tod.h new file +18 -0
new file mode 100644
index 0000000000..43ed71600f
@@ -0,0 +1,46 @@
+/*
+ * TOD (Time Of Day) clock
+ *
+ * Copyright 2018 Red Hat, Inc.
+ * Author(s): David Hildenbrand <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_S390_TOD_H
+#define HW_S390_TOD_H
+
+#include "hw/qdev.h"
+
+typedef struct S390TOD {
+    uint8_t high;
+    uint64_t low;

include/hw/s390x/tod.h line 28

Quoted 10 lines
include/hw/s390x/tod.h new file +10 -0
+} S390TOD;
+
+#define TYPE_S390_TOD "s390-tod"
+#define S390_TOD(obj) OBJECT_CHECK(S390TODState, (obj), TYPE_S390_TOD)
+#define S390_TOD_CLASS(oc) OBJECT_CLASS_CHECK(S390TODClass, (oc), \
+                                              TYPE_S390_TOD)
+#define S390_TOD_GET_CLASS(obj) OBJECT_GET_CLASS(S390TODClass, (obj), \
+                                                 TYPE_S390_TOD)
+#define TYPE_KVM_S390_TOD "kvm-" TYPE_S390_TOD
+#define TYPE_QEMU_S390_TOD "qemu-" TYPE_S390_TOD

I'd prefer if you could turn that into suffixes instead of prefixes, like it is done in include/hw/s390x/storage-keys.h and storage-attributes.h already.

Apart from that, the patch looks fine to me, so once you've changed that, feel free to add:

#3 lore raw

Re: [Qemu-devel] [PATCH v2 1/7] s390x/tod: factor out TOD into separate device

Quoted 9 lines · Thomas Huth
Quoted 7 lines · David Hildenbrand

Let's treat this like a separate device. TCG will have to store the actual state/time later on.

Include cpu-qom.h in kvm_s390x.h (due to S390CPU) to compile tod-kvm.c.

---

[...]

Quoted 15 lines

hw/s390x/tod.c line 105

Quoted 12 lines
hw/s390x/tod.c new file +12 -0
+static void s390_tod_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->desc = "TOD (Time Of Day) Clock";
+    dc->realize = s390_tod_realize;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+
+    /* We only have one TOD clock in the system attached to the machine */
+    dc->user_creatable = false;
+    dc->hotpluggable = false;
+}

IIRC you don't need the hotpluggable = false if you already set user_creatable = false.

That makes sense.

Quoted 54 lines
Quoted 39 lines
hw/s390x/tod.c new file +14 -0
+static TypeInfo s390_tod_info = {
+    .name = TYPE_S390_TOD,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(S390TODState),
+    .class_init = s390_tod_class_init,
+    .class_size = sizeof(S390TODClass),
+    .abstract = true,
+};
+
+static void register_types(void)
+{
+    type_register_static(&s390_tod_info);
+}
+type_init(register_types);
include/hw/s390x/tod.h new file +18 -0
new file mode 100644
index 0000000000..43ed71600f
@@ -0,0 +1,46 @@
+/*
+ * TOD (Time Of Day) clock
+ *
+ * Copyright 2018 Red Hat, Inc.
+ * Author(s): David Hildenbrand <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_S390_TOD_H
+#define HW_S390_TOD_H
+
+#include "hw/qdev.h"
+
+typedef struct S390TOD {
+    uint8_t high;
+    uint64_t low;

include/hw/s390x/tod.h line 28

Quoted 10 lines
include/hw/s390x/tod.h new file +10 -0
+} S390TOD;
+
+#define TYPE_S390_TOD "s390-tod"
+#define S390_TOD(obj) OBJECT_CHECK(S390TODState, (obj), TYPE_S390_TOD)
+#define S390_TOD_CLASS(oc) OBJECT_CLASS_CHECK(S390TODClass, (oc), \
+                                              TYPE_S390_TOD)
+#define S390_TOD_GET_CLASS(obj) OBJECT_GET_CLASS(S390TODClass, (obj), \
+                                                 TYPE_S390_TOD)
+#define TYPE_KVM_S390_TOD "kvm-" TYPE_S390_TOD
+#define TYPE_QEMU_S390_TOD "qemu-" TYPE_S390_TOD

I'd prefer if you could turn that into suffixes instead of prefixes, like it is done in include/hw/s390x/storage-keys.h and storage-attributes.h already.

Quoted 2 lines

Apart from that, the patch looks fine to me, so once you've changed that, feel free to add:

Sure, will change! Thanks!

Quoted 3 lines
Signature · 3 lines
Thanks,

David / dhildenb
#4 lore raw

PATCH v2 2/7 s390x/tcg: drop tod_basetime

In reply to David Hildenbrand

Never set to anything but 0.

---
 target/s390x/cpu.c         | 1 -
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 4 ++--
 3 files changed, 2 insertions(+), 4 deletions(-)
target/s390x/cpu.c +0 -1
index 03ea6eafa7..a41b3f34ab 100644
@@ -289,7 +289,6 @@ static void s390_cpu_initfn(Object *obj)
     qemu_get_timedate(&tm, 0);
     env->tod_offset = TOD_UNIX_EPOCH +
                       (time2tod(mktimegm(&tm)) * 1000000000ULL);
-    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);
     s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
target/s390x/cpu.h +0 -1
index ac51c17fb4..4abfe88a3d 100644
@@ -131,7 +131,6 @@ struct CPUS390XState {
 #endif
 
     uint64_t tod_offset;
-    uint64_t tod_basetime;
     QEMUTimer *tod_timer;
 
     QEMUTimer *cpu_timer;
target/s390x/misc_helper.c +2 -2
index de1ced2082..dd5273949b 100644
@@ -141,7 +141,7 @@ uint64_t HELPER(stck)(CPUS390XState *env)
     uint64_t time;
 
     time = env->tod_offset +
-        time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - env->tod_basetime);
+        time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
 
     return time;
 }
@@ -161,7 +161,7 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     /* nanoseconds */
     time = tod2time(time);
 
-    timer_mod(env->tod_timer, env->tod_basetime + time);
+    timer_mod(env->tod_timer, time);
 }
 
 /* Set Tod Programmable Field */
Signature · 1 line
2.17.1
#6 lore raw

PATCH v2 3/7 s390x/tcg: properly implement the TOD

In reply to David Hildenbrand

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.

---
 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    | 15 -------------
 target/s390x/misc_helper.c | 33 ++++++++++++++++++++++-----
 target/s390x/tcg_s390x.h   | 18 +++++++++++++++
 8 files changed, 117 insertions(+), 33 deletions(-)
 create mode 100644 target/s390x/tcg_s390x.h
hw/s390x/tod-qemu.c +42 -4
index 7997ba2b1a..da2b82cd51 100644
@@ -11,17 +11,41 @@
 #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(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++;
+    }
 }
 
 static void qemu_s390_tod_set(S390TODState *td, S390TOD *tod, Error **errp)
 {
-    /* FIXME */
+    CPUState *cpu;
+
+    td->base = *tod;
+
+    td->base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+    if (tod->low < td->base.low) {
+        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)
@@ -32,10 +56,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)
+{
+    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;
+    }
+}
+
 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),
 };
hw/s390x/tod.c +11 -0
index c1b3438452..7495d8c057 100644
@@ -30,6 +30,17 @@ void s390_init_tod(void)
     qdev_init_nofail(DEVICE(obj));
 }
 
+S390TODState *s390_get_tod(void)
+{
+    static S390TODState *ts;
+
+    if (!ts) {
+        ts = S390_TOD(object_resolve_path_type("", TYPE_S390_TOD, NULL));
+    }
+
+    return ts;
+}
+
 #define S390_TOD_CLOCK_VALUE_MISSING    0x00
 #define S390_TOD_CLOCK_VALUE_PRESENT    0x01
 
include/hw/s390x/tod.h +19 -0
index 43ed71600f..5491245b86 100644
@@ -30,6 +30,9 @@ typedef struct S390TOD {
 typedef struct S390TODState {
     /* private */
     DeviceState parent_obj;
+
+    /* unused by KVM implementation */
+    S390TOD base;
 } S390TODState;
 
 typedef struct S390TODClass {
@@ -41,6 +44,22 @@ typedef struct S390TODClass {
     void (*set)(S390TODState *td, S390TOD *tod, Error **errp);
 } S390TODClass;
 
+/* The value of the TOD clock for 1.1.1970. */
+#define TOD_UNIX_EPOCH 0x7d91048bca000000ULL
+
+/* Converts ns to s390's clock format */
+static inline uint64_t time2tod(uint64_t ns)
+{
+    return (ns << 9) / 125;
+}
+
+/* Converts s390's clock format to ns */
+static inline uint64_t tod2time(uint64_t t)
+{
+    return (t * 125) >> 9;
+}
+
 void s390_init_tod(void);
+S390TODState *s390_get_tod(void);
 
 #endif
target/s390x/cpu.c +0 -7
index a41b3f34ab..40d6980229 100644
@@ -30,7 +30,6 @@
 #include "kvm_s390x.h"
 #include "sysemu/kvm.h"
 #include "qemu-common.h"
-#include "qemu/cutils.h"
 #include "qemu/timer.h"
 #include "qemu/error-report.h"
 #include "trace.h"
@@ -275,9 +274,6 @@ static void s390_cpu_initfn(Object *obj)
     CPUState *cs = CPU(obj);
     S390CPU *cpu = S390_CPU(obj);
     CPUS390XState *env = &cpu->env;
-#if !defined(CONFIG_USER_ONLY)
-    struct tm tm;
-#endif
 
     cs->env_ptr = env;
     cs->halted = 1;
@@ -286,9 +282,6 @@ static void s390_cpu_initfn(Object *obj)
                         s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
     s390_cpu_model_register_props(obj);
 #if !defined(CONFIG_USER_ONLY)
-    qemu_get_timedate(&tm, 0);
-    env->tod_offset = TOD_UNIX_EPOCH +
-                      (time2tod(mktimegm(&tm)) * 1000000000ULL);
     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);
     s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
target/s390x/cpu.h +0 -1
index 4abfe88a3d..2c3dd2d189 100644
@@ -130,7 +130,6 @@ struct CPUS390XState {
     uint64_t cpuid;
 #endif
 
-    uint64_t tod_offset;
     QEMUTimer *tod_timer;
 
     QEMUTimer *cpu_timer;
target/s390x/internal.h +0 -15
index e392a02d12..f2a771e2b4 100644
@@ -237,21 +237,6 @@ enum cc_op {
     CC_OP_MAX
 };
 
-/* The value of the TOD clock for 1.1.1970. */
-#define TOD_UNIX_EPOCH 0x7d91048bca000000ULL
-
-/* Converts ns to s390's clock format */
-static inline uint64_t time2tod(uint64_t ns)
-{
-    return (ns << 9) / 125;
-}
-
-/* Converts s390's clock format to ns */
-static inline uint64_t tod2time(uint64_t t)
-{
-    return (t * 125) >> 9;
-}
-
 static inline hwaddr decode_basedisp_s(CPUS390XState *env, uint32_t ipb,
                                        uint8_t *ar)
 {
target/s390x/misc_helper.c +27 -6
index dd5273949b..6ccbe1fe9a 100644
@@ -28,6 +28,7 @@
 #include "qemu/timer.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
+#include "tcg_s390x.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "sysemu/cpus.h"
@@ -39,6 +40,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 +140,36 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1)
 /* Store Clock */
 uint64_t HELPER(stck)(CPUS390XState *env)
 {
-    uint64_t time;
+    S390TODState *td = s390_get_tod();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    Error *err = NULL;
+    S390TOD tod;
 
-    time = env->tod_offset +
-        time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
-
-    return time;
+    tdc->get(td, &tod, &err);
+    g_assert(!err);
+    return tod.low;
 }
 
 /* Set Clock Comparator */
 void HELPER(sckc)(CPUS390XState *env, uint64_t time)
 {
+    S390TODState *td = s390_get_tod();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    Error *err = NULL;
+    S390TOD tod_base;
+
     if (time == -1ULL) {
         return;
     }
 
     env->ckc = time;
 
+    tdc->get(td, &tod_base, &err);
+    g_assert(!err);
+    tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+
     /* difference between origins */
-    time -= env->tod_offset;
+    time -= tod_base.low;
 
     /* nanoseconds */
     time = tod2time(time);
@@ -164,6 +177,14 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     timer_mod(env->tod_timer, time);
 }
 
+void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque)
+{
+    S390CPU *cpu = S390_CPU(cs);
+    CPUS390XState *env = &cpu->env;
+
+    helper_sckc(env, env->ckc);
+}
+
 /* Set Tod Programmable Field */
 void HELPER(sckpf)(CPUS390XState *env, uint64_t r0)
 {
target/s390x/tcg_s390x.h new file +18 -0
new file mode 100644
index 0000000000..4e308aa0ce
@@ -0,0 +1,18 @@
+/*
+ * QEMU TCG support -- s390x specific functions.
+ *
+ * Copyright 2018 Red Hat, Inc.
+ *
+ * Authors:
+ *   David Hildenbrand <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef TCG_S390X_H
+#define TCG_S390X_H
+
+void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque);
+
+#endif /* TCG_S390X_H */
Signature · 1 line
2.17.1
#7 lore raw

Re: [Qemu-devel] [PATCH v2 3/7] s390x/tcg: properly implement the TOD

include/hw/s390x/tod.h typedef struct S390TODClass { · line 60

Quoted 11 lines · David Hildenbrand
include/hw/s390x/tod.h +11 -0
+/* Converts ns to s390's clock format */
+static inline uint64_t time2tod(uint64_t ns)
+{
+    return (ns << 9) / 125;
+}
+
+/* Converts s390's clock format to ns */
+static inline uint64_t tod2time(uint64_t t)
+{
+    return (t * 125) >> 9;
+}

How many significant bits on input here?
Do you in fact want to be using muldiv64?

r~

#8 lore raw

Re: [Qemu-devel] [PATCH v2 3/7] s390x/tcg: properly implement the TOD

Quoted 13 lines · Richard Henderson

include/hw/s390x/tod.h typedef struct S390TODClass { · line 60

Quoted 11 lines · David Hildenbrand
include/hw/s390x/tod.h +11 -0
+/* Converts ns to s390's clock format */
+static inline uint64_t time2tod(uint64_t ns)
+{
+    return (ns << 9) / 125;
+}
+
+/* Converts s390's clock format to ns */
+static inline uint64_t tod2time(uint64_t t)
+{
+    return (t * 125) >> 9;
+}

In this patch I'm only moving the code. If we find this is a problem, this should go into a separate patch.

Quoted 1 line

How many significant bits on input here?

Basically all are significant, and as it is a clock, we will reach these bits at one point.

Quoted 1 line

Do you in fact want to be using muldiv64?

Looking at linux:

arch/s390/include/asm/timex.h

They have a lengthy documentation, resulting in (a spli to avoid overflows)

return ((todval >> 9) * 125) + (((todval & 0x1ff) * 125) >> 9);

Maybe we should do the same?

Quoted 4 lines

r~

Signature · 3 lines
Thanks,

David / dhildenb
#9 lore raw

Re: [Qemu-devel] [PATCH v2 3/7] s390x/tcg: properly implement the TOD

Quoted 15 lines · David Hildenbrand
Quoted 13 lines · Richard Henderson

include/hw/s390x/tod.h typedef struct S390TODClass { · line 60

Quoted 11 lines · David Hildenbrand
include/hw/s390x/tod.h +11 -0
+/* Converts ns to s390's clock format */
+static inline uint64_t time2tod(uint64_t ns)
+{
+    return (ns << 9) / 125;
+}
+
+/* Converts s390's clock format to ns */
+static inline uint64_t tod2time(uint64_t t)
+{
+    return (t * 125) >> 9;
+}
Quoted 2 lines

In this patch I'm only moving the code. If we find this is a problem, this should go into a separate patch.

Ah, right.

Quoted 9 lines
Quoted 1 line

How many significant bits on input here?

Basically all are significant, and as it is a clock, we will reach these bits at one point.

Quoted 1 line

Do you in fact want to be using muldiv64?

Looking at linux:

Quoted 7 lines

arch/s390/include/asm/timex.h

They have a lengthy documentation, resulting in (a spli to avoid overflows)

return ((todval >> 9) * 125) + (((todval & 0x1ff) * 125) >> 9);

Maybe we should do the same?

That would work too.

r~

#10 lore raw

Re: [Qemu-devel] [PATCH v2 3/7] s390x/tcg: properly implement the TOD

In reply to David Hildenbrand
Quoted 12 lines · David Hildenbrand

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.

[...]

Quoted 8 lines
hw/s390x/tod.c +0 -0
index c1b3438452..7495d8c057 100644
@@ -30,6 +30,17 @@ void s390_init_tod(void)
     qdev_init_nofail(DEVICE(obj));
 }
 

hw/s390x/tod.c void s390_init_tod(void) · line 42

Quoted 10 lines
hw/s390x/tod.c +10 -0
+S390TODState *s390_get_tod(void)
+{
+    static S390TODState *ts;
+
+    if (!ts) {
+        ts = S390_TOD(object_resolve_path_type("", TYPE_S390_TOD, NULL));
+    }
+
+    return ts;
+}

Could you please call that function s390_get_todstate or something similar? Otherwise it sounds like it could be used to retrieve the time, and not the state object.

Thomas

#11 lore raw

Re: [Qemu-devel] [PATCH v2 3/7] s390x/tcg: properly implement the TOD

Quoted 22 lines · Thomas Huth
Quoted 12 lines · David Hildenbrand

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.

[...]

Quoted 8 lines
hw/s390x/tod.c +0 -0
index c1b3438452..7495d8c057 100644
@@ -30,6 +30,17 @@ void s390_init_tod(void)
     qdev_init_nofail(DEVICE(obj));
 }
 
Quoted 14 lines

hw/s390x/tod.c void s390_init_tod(void) · line 42

Quoted 10 lines
hw/s390x/tod.c +10 -0
+S390TODState *s390_get_tod(void)
+{
+    static S390TODState *ts;
+
+    if (!ts) {
+        ts = S390_TOD(object_resolve_path_type("", TYPE_S390_TOD, NULL));
+    }
+
+    return ts;
+}

Could you please call that function s390_get_todstate or something similar? Otherwise it sounds like it could be used to retrieve the time, and not the state object.

Yes, makes sense! Thanks!

Quoted 3 lines

Thomas

Signature · 3 lines
Thanks,

David / dhildenb
#12 lore raw

PATCH v2 4/7 s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts

In reply to David Hildenbrand

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

---
 target/s390x/misc_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)
target/s390x/misc_helper.c +6 -0
index 6ccbe1fe9a..d5f9f5e1d3 100644
@@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     Error *err = NULL;
     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();
+
     if (time == -1ULL) {
         return;
     }
Signature · 1 line
2.17.1
Reviewed-by on v2 4/7
#13 lore raw

Re: [Qemu-devel] [PATCH v2 4/7] s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts

Quoted 13 lines · David Hildenbrand

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

---
 target/s390x/misc_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)
target/s390x/misc_helper.c +0 -0
index 6ccbe1fe9a..d5f9f5e1d3 100644

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

Quoted 8 lines
target/s390x/misc_helper.c +5 -0
@@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     Error *err = NULL;
     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();

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

Quoted 1 line
target/s390x/misc_helper.c +0 -0
     if (time == -1ULL) {

I wonder whether that check is still adequate? Is there really a way to disable the clock comparator like this? At least I haven't seen it in the PoP.

Quoted 3 lines
target/s390x/misc_helper.c +0 -0
         return;
     }
#14 lore raw

Re: [Qemu-devel] [PATCH v2 4/7] s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts

Quoted 25 lines · Thomas Huth
Quoted 13 lines · David Hildenbrand

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

---
 target/s390x/misc_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)
target/s390x/misc_helper.c +0 -0
index 6ccbe1fe9a..d5f9f5e1d3 100644

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

Quoted 8 lines
target/s390x/misc_helper.c +5 -0
@@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     Error *err = NULL;
     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();
Quoted 5 lines

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

Quoted 1 line
target/s390x/misc_helper.c +0 -0
     if (time == -1ULL) {

I wonder whether that check is still adequate? Is there really a way to disable the clock comparator like this? At least I haven't seen it in the PoP.

e.g. 4-61 (Control)

4. When the clock-comparator sign control is zero,
(a) the program can set the clock comparator to all zeros to ensure that an interruption condition is immediately present, and (b) the program can set the clock comparator to a value of all binary ones to ensure that a clock-comparator interrup- tion is never recognized. [...]

Rational from 4-59 (Control):

The clock comparator provides a means of causing an interruption when the TOD-clock value exceeds a value specified by the program.

We can never exceed all binary 1s. So it is really exceeding, not "hitting"

Quoted 5 lines
Quoted 4 lines
target/s390x/misc_helper.c +0 -0
         return;
     }
Signature · 3 lines
Thanks,

David / dhildenb
#15 lore raw

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 4/7] s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts

Quoted 2 lines · David Hildenbrand
Quoted 1 line · Thomas Huth

On 20.06.2018 12:08, David Hildenbrand wrote:

[...]

Quoted 16 lines
Quoted 5 lines
Quoted 1 line
     if (time == -1ULL) {

I wonder whether that check is still adequate? Is there really a way to disable the clock comparator like this? At least I haven't seen it in the PoP.

e.g. 4-61 (Control)

4. When the clock-comparator sign control is zero,
(a) the program can set the clock comparator to all zeros to ensure that an interruption condition is immediately present, and (b) the program can set the clock comparator to a value of all binary ones to ensure that a clock-comparator interrup- tion is never recognized. [...]

Quoted 7 lines

Rational from 4-59 (Control):

The clock comparator provides a means of causing an interruption when the TOD-clock value exceeds a value specified by the program.

We can never exceed all binary 1s. So it is really exceeding, not "hitting"

Thanks! ... maybe you could add a comment to the code with a pointer to that section in the PoP while you're at it? (just if you agree that it is useful)

Thomas

#16 lore raw

Re: [Qemu-devel] [PATCH v2 4/7] s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts

In reply to David Hildenbrand
Quoted 42 lines · On Thu, 21 Jun 2018 15:54:36 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 25 lines · Thomas Huth
Quoted 13 lines · David Hildenbrand

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

---
 target/s390x/misc_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)
target/s390x/misc_helper.c +0 -0
index 6ccbe1fe9a..d5f9f5e1d3 100644

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

Quoted 8 lines
target/s390x/misc_helper.c +5 -0
@@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     Error *err = NULL;
     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();  
Quoted 5 lines

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

Quoted 1 line
target/s390x/misc_helper.c +0 -0
     if (time == -1ULL) {  

I wonder whether that check is still adequate? Is there really a way to disable the clock comparator like this? At least I haven't seen it in the PoP.

e.g. 4-61 (Control)

4. When the clock-comparator sign control is zero,
(a) the program can set the clock comparator to all zeros to ensure that an interruption condition is immediately present, and (b) the program can set the clock comparator to a value of all binary ones to ensure that a clock-comparator interrup- tion is never recognized. [...]

Quoted 7 lines

Rational from 4-59 (Control):

The clock comparator provides a means of causing an interruption when the TOD-clock value exceeds a value specified by the program.

We can never exceed all binary 1s. So it is really exceeding, not "hitting"

OK, so that actually makes sense with your change on top. Before we just jumped out. Agree about adding a comment.

#17 lore raw

Re: [Qemu-devel] [PATCH v2 4/7] s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts

In reply to Thomas Huth
Quoted 25 lines · On Thu, 21 Jun 2018 14:09:28 +0200 Thomas Huth <[email protected]> wrote:
Quoted 13 lines · David Hildenbrand

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

---
 target/s390x/misc_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)
target/s390x/misc_helper.c +0 -0
index 6ccbe1fe9a..d5f9f5e1d3 100644

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

Quoted 8 lines
target/s390x/misc_helper.c +5 -0
@@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     Error *err = NULL;
     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();  
Quoted 5 lines

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

Quoted 1 line
target/s390x/misc_helper.c +0 -0
     if (time == -1ULL) {  

I wonder whether that check is still adequate? Is there really a way to disable the clock comparator like this? At least I haven't seen it in the PoP.

Me neither. It seems to have been in the code since the beginning...

Quoted 5 lines
Quoted 4 lines
target/s390x/misc_helper.c +0 -0
         return;
     }
#18 lore raw

Re: [Qemu-devel] [PATCH v2 4/7] s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts

Quoted 28 lines · Cornelia Huck
Quoted 25 lines · On Thu, 21 Jun 2018 14:09:28 +0200 Thomas Huth <[email protected]> wrote:
Quoted 13 lines · David Hildenbrand

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

---
 target/s390x/misc_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)
target/s390x/misc_helper.c +0 -0
index 6ccbe1fe9a..d5f9f5e1d3 100644

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

Quoted 8 lines
target/s390x/misc_helper.c +5 -0
@@ -158,6 +158,12 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     Error *err = NULL;
     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();  
Quoted 7 lines
Quoted 5 lines

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

Quoted 1 line
target/s390x/misc_helper.c +0 -0
     if (time == -1ULL) {  

I wonder whether that check is still adequate? Is there really a way to disable the clock comparator like this? At least I haven't seen it in the PoP.

Me neither. It seems to have been in the code since the beginning...

I agree that this is somewhat buried deep down in the PoP and not obvious when only looking at SET CLOCK COMPARATOR documentation.

I'll add a comment like

"we can never exceed -1 and therefore never trigger an IRQ"

Signature · 3 lines
Thanks,

David / dhildenb
#19 lore raw

PATCH v2 5/7 s390x/tcg: implement SET CLOCK

In reply to David Hildenbrand

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.

---
 target/s390x/helper.h      |  1 +
 target/s390x/insn-data.def |  3 +--
 target/s390x/misc_helper.c | 19 +++++++++++++++++++
 target/s390x/translate.c   |  9 +++++++++
 4 files changed, 30 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 +19 -0
index d5f9f5e1d3..c9712b9476 100644
@@ -191,6 +191,25 @@ 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_tod();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod = {
+        .high = 0,
+        .low = tod_low,
+    };
+    Error *err = NULL;
+
+    qemu_mutex_lock_iothread();
+    tdc->set(td, &tod, &err);
+    qemu_mutex_unlock_iothread();
+    g_assert(!err);
+
+    return 0;
+}
+
 /* Set Tod Programmable Field */
 void HELPER(sckpf)(CPUS390XState *env, uint64_t r0)
 {
target/s390x/translate.c +9 -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)
+{
+    check_privileged(s);
+    tcg_gen_qemu_ld_i64(o->in1, o->addr1, get_mem_index(s), MO_TEQ | MO_ALIGN);
+    gen_helper_sck(cc_op, cpu_env, o->in1);
+    set_cc_static(s);
+    return DISAS_NEXT;
+}
+
 static DisasJumpType op_sckc(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
Signature · 1 line
2.17.1
#20 lore raw

Re: [Qemu-devel] [PATCH v2 5/7] s390x/tcg: implement SET CLOCK

Quoted 53 lines · David Hildenbrand

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.

---
 target/s390x/helper.h      |  1 +
 target/s390x/insn-data.def |  3 +--
 target/s390x/misc_helper.c | 19 +++++++++++++++++++
 target/s390x/translate.c   |  9 +++++++++
 4 files changed, 30 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 +5 -0
index d5f9f5e1d3..c9712b9476 100644
@@ -191,6 +191,25 @@ 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_tod();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);

target/s390x/misc_helper.c void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque) · line 208

Quoted 10 lines
target/s390x/misc_helper.c +10 -0
+    S390TOD tod = {
+        .high = 0,
+        .low = tod_low,
+    };
+    Error *err = NULL;
+
+    qemu_mutex_lock_iothread();
+    tdc->set(td, &tod, &err);
+    qemu_mutex_unlock_iothread();
+    g_assert(!err);

I know it currently can't happen, but still, I think it would be nicer to use CC3 to tell the guest that something went wrong with the clock, instead of abort QEMU here.

Thomas

#21 lore raw

Re: [Qemu-devel] [PATCH v2 5/7] s390x/tcg: implement SET CLOCK

Quoted 60 lines · Thomas Huth
Quoted 59 lines · David Hildenbrand

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.

---
 target/s390x/helper.h      |  1 +
 target/s390x/insn-data.def |  3 +--
 target/s390x/misc_helper.c | 19 +++++++++++++++++++
 target/s390x/translate.c   |  9 +++++++++
 4 files changed, 30 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 +11 -0
index d5f9f5e1d3..c9712b9476 100644
@@ -191,6 +191,25 @@ 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_tod();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod = {
+        .high = 0,
+        .low = tod_low,
+    };
+    Error *err = NULL;
+
Quoted 8 lines

target/s390x/misc_helper.c void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque) · line 208

Quoted 4 lines
target/s390x/misc_helper.c +4 -0
+    qemu_mutex_lock_iothread();
+    tdc->set(td, &tod, &err);
+    qemu_mutex_unlock_iothread();
+    g_assert(!err);

I know it currently can't happen, but still, I think it would be nicer to use CC3 to tell the guest that something went wrong with the clock, instead of abort QEMU here.

Hmm, I thing I should either use error_abort here or do what you suggest.

However, CC=3 means "Clock in not-operational state".

And this implies that also STORE CLOCK and friends will have to fail and that we have to present a machine check. Especially, once we would implement the TOD-clock steering facility, CC=3 would not apply anymore.

So instead of faking something that is not architecturally correct, I think we really should just quit QEMU, as we expect this to never fail.

Quoted 3 lines

Thomas

Signature · 3 lines
Thanks,

David / dhildenb
#22 lore raw

Re: [Qemu-devel] [PATCH v2 5/7] s390x/tcg: implement SET CLOCK

Quoted 25 lines · On Thu, 21 Jun 2018 16:01:22 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 13 lines · Thomas Huth
Quoted 11 lines · David Hildenbrand
target/s390x/misc_helper.c +11 -0
+/* Set Clock */
+uint32_t HELPER(sck)(CPUS390XState *env, uint64_t tod_low)
+{
+    S390TODState *td = s390_get_tod();
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    S390TOD tod = {
+        .high = 0,
+        .low = tod_low,
+    };
+    Error *err = NULL;
+
Quoted 8 lines

target/s390x/misc_helper.c void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque) · line 208

Quoted 4 lines
target/s390x/misc_helper.c +4 -0
+    qemu_mutex_lock_iothread();
+    tdc->set(td, &tod, &err);
+    qemu_mutex_unlock_iothread();
+    g_assert(!err);  

I know it currently can't happen, but still, I think it would be nicer to use CC3 to tell the guest that something went wrong with the clock, instead of abort QEMU here.

Hmm, I thing I should either use error_abort here or do what you suggest.

Quoted 8 lines

However, CC=3 means "Clock in not-operational state".

And this implies that also STORE CLOCK and friends will have to fail and that we have to present a machine check. Especially, once we would implement the TOD-clock steering facility, CC=3 would not apply anymore.

So instead of faking something that is not architecturally correct, I think we really should just quit QEMU, as we expect this to never fail.

I think error_abort is the best way to handle that, then.

#23 lore raw

PATCH v2 6/7 s390x/tcg: rearm the CKC timer during migration

In reply to David Hildenbrand

If the CPU data is migrated after the TOD clock, the CKC timer of a CPU is not rearmed. Let's rearm it when loading the CPU state.

Introduce tcg-stub.c just like kvm-stub.c for tcg specific stubs.

---
 target/s390x/Makefile.objs |  1 +
 target/s390x/machine.c     |  6 ++++++
 target/s390x/tcg-stub.c    | 20 ++++++++++++++++++++
 3 files changed, 27 insertions(+)
 create mode 100644 target/s390x/tcg-stub.c
target/s390x/Makefile.objs +1 -0
index 31932de9cf..22a9a9927a 100644
@@ -5,6 +5,7 @@ obj-$(CONFIG_SOFTMMU) += machine.o ioinst.o arch_dump.o mmu_helper.o diag.o
 obj-$(CONFIG_SOFTMMU) += sigp.o
 obj-$(CONFIG_KVM) += kvm.o
 obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
+obj-$(call lnot,$(CONFIG_TCG)) += tcg-stub.o
 
 # build and run feature list generator
 feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/
target/s390x/machine.c +6 -0
index 84b4928755..bd3230d027 100644
@@ -19,6 +19,7 @@
 #include "cpu.h"
 #include "internal.h"
 #include "kvm_s390x.h"
+#include "tcg_s390x.h"
 #include "sysemu/kvm.h"
 
 static int cpu_post_load(void *opaque, int version_id)
@@ -34,6 +35,11 @@ static int cpu_post_load(void *opaque, int version_id)
         return kvm_s390_vcpu_interrupt_post_load(cpu);
     }
 
+    if (tcg_enabled()) {
+        /* Rearm the CKC timer if necessary */
+        tcg_s390_tod_updated(CPU(cpu), RUN_ON_CPU_NULL);
+    }
+
     return 0;
 }
 
target/s390x/tcg-stub.c new file +20 -0
new file mode 100644
index 0000000000..c93501db0b
@@ -0,0 +1,20 @@
+/*
+ * QEMU TCG support -- s390x specific function stubs.
+ *
+ * Copyright (C) 2018 Red Hat Inc
+ *
+ * Authors:
+ *   David Hildenbrand <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "cpu.h"
+#include "tcg_s390x.h"
+
+void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque)
+{
+}
Signature · 1 line
2.17.1
Reviewed-by on v2 6/7
#24 lore raw

Re: [Qemu-devel] [PATCH v2 6/7] s390x/tcg: rearm the CKC timer during migration

Quoted 5 lines · David Hildenbrand

If the CPU data is migrated after the TOD clock, the CKC timer of a CPU is not rearmed. Let's rearm it when loading the CPU state.

Introduce tcg-stub.c just like kvm-stub.c for tcg specific stubs.

Quoted 7 lines
---
 target/s390x/Makefile.objs |  1 +
 target/s390x/machine.c     |  6 ++++++
 target/s390x/tcg-stub.c    | 20 ++++++++++++++++++++
 3 files changed, 27 insertions(+)
 create mode 100644 target/s390x/tcg-stub.c
#25 lore raw

PATCH v2 7/7 s390x/tcg: fix CPU hotplug with single-threaded TCG

In reply to David Hildenbrand

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, let's use run_on_cpu() for KVM only - KVM requires it due to the initial CPU reset ioctl.

---
 target/s390x/cpu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
target/s390x/cpu.c +6 -5
index 40d6980229..7536dd0f03 100644
@@ -218,11 +218,12 @@ 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
+
+    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:
Signature · 1 line
2.17.1
#26 lore raw

Re: [Qemu-devel] [PATCH v2 7/7] s390x/tcg: fix CPU hotplug with single-threaded TCG

Quoted 20 lines · David Hildenbrand

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, let's use run_on_cpu() for KVM only - KVM requires it due to the initial CPU reset ioctl.

---
 target/s390x/cpu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
target/s390x/cpu.c +0 -1
index 40d6980229..7536dd0f03 100644
@@ -218,11 +218,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 #endif
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
-#if !defined(CONFIG_USER_ONLY)

target/s390x/cpu.c static void s390_cpu_realizefn(DeviceState *dev, Error **errp) · line 226

Quoted 10 lines
target/s390x/cpu.c +6 -4
-    run_on_cpu(cs, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
-#else
-    cpu_reset(cs);
-#endif
+
+    if (kvm_enabled()) {
+        run_on_cpu(cs, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
+    } else {
+        cpu_reset(cs);
+    }

Could you please add a comment with the information from the patch description before that code? Otherwise, I guess everybody who only looks at the code (and not at the git history) will be quite confused about this, too.

Thomas