[Qemu-devel] [PATCH v1 0/7] s390x: TOD refactoring + TCG CPU hotplug support

[Qemu-devel] [PATCH v1 0/7] s390x: TOD refactoring + TCG CPU hotplug support

[David Hildenbrand]

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).

The cpu timer is in a similar bad shape, but this will require more work :(

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        |  84 ++++++++++++++++++++++++
 hw/s390x/tod.c             | 131 +++++++++++++++++++++++++++++++++++++
 include/hw/s390x/tod.h     |  65 ++++++++++++++++++
 target/s390x/cpu.c         |  52 ++-------------
 target/s390x/cpu.h         |   9 +--
 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     |   7 ++
 target/s390x/misc_helper.c |  59 +++++++++++++++--
 target/s390x/translate.c   |   9 +++
 15 files changed, 434 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

-- 
2.17.0

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

[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.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index dc704b57d6..93282f7c59 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -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
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e548d341a0..390f021107 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -35,6 +35,7 @@
 #include "migration/register.h"
 #include "cpu_models.h"
 #include "hw/nmi.h"
+#include "hw/s390x/tod.h"
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
@@ -187,58 +188,6 @@ static void s390_memory_init(ram_addr_t mem_size)
     s390_stattrib_init();
 }
 
-#define S390_TOD_CLOCK_VALUE_MISSING    0x00
-#define S390_TOD_CLOCK_VALUE_PRESENT    0x01
-
-static void gtod_save(QEMUFile *f, void *opaque)
-{
-    uint64_t tod_low;
-    uint8_t tod_high;
-    int r;
-
-    r = s390_get_clock(&tod_high, &tod_low);
-    if (r) {
-        warn_report("Unable to get guest clock for migration: %s",
-                    strerror(-r));
-        error_printf("Guest clock will not be migrated "
-                     "which could cause the guest to hang.");
-        qemu_put_byte(f, S390_TOD_CLOCK_VALUE_MISSING);
-        return;
-    }
-
-    qemu_put_byte(f, S390_TOD_CLOCK_VALUE_PRESENT);
-    qemu_put_byte(f, tod_high);
-    qemu_put_be64(f, tod_low);
-}
-
-static int gtod_load(QEMUFile *f, void *opaque, int version_id)
-{
-    uint64_t tod_low;
-    uint8_t tod_high;
-    int r;
-
-    if (qemu_get_byte(f) == S390_TOD_CLOCK_VALUE_MISSING) {
-        warn_report("Guest clock was not migrated. This could "
-                    "cause the guest to hang.");
-        return 0;
-    }
-
-    tod_high = qemu_get_byte(f);
-    tod_low = qemu_get_be64(f);
-
-    r = s390_set_clock(&tod_high, &tod_low);
-    if (r) {
-        error_report("Unable to set KVM guest TOD clock: %s", strerror(-r));
-    }
-
-    return r;
-}
-
-static SaveVMHandlers savevm_gtod = {
-    .save_state = gtod_save,
-    .load_state = gtod_load,
-};
-
 static void s390_init_ipl_dev(const char *kernel_filename,
                               const char *kernel_cmdline,
                               const char *initrd_filename, const char *firmware,
@@ -363,8 +312,8 @@ static void ccw_init(MachineState *machine)
         s390_create_sclpconsole("sclplmconsole", serial_hd(1));
     }
 
-    /* Register savevm handler for guest TOD clock */
-    register_savevm_live(NULL, "todclock", 0, 1, &savevm_gtod, NULL);
+    /* init the TOD clock */
+    s390_init_tod();
 }
 
 static void s390_cpu_plug(HotplugHandler *hotplug_dev,
diff --git a/hw/s390x/tod-kvm.c b/hw/s390x/tod-kvm.c
new file mode 100644
index 0000000000..3fe43fc114
--- /dev/null
+++ b/hw/s390x/tod-kvm.c
@@ -0,0 +1,64 @@
+/*
+ * TOD (Time Of Day) clock - KVM implementation
+ *
+ * Copyright 2018 Red Hat, Inc.
+ * Author(s): David Hildenbrand <david@redhat.com>
+ *
+ * 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 "qapi/error.h"
+#include "hw/s390x/tod.h"
+#include "kvm_s390x.h"
+
+static void kvm_s390_tod_get(S390TODState *td, S390TOD *tod, Error **errp)
+{
+    int r;
+
+    r = kvm_s390_get_clock_ext(&tod->high, &tod->low);
+    if (r == -ENXIO) {
+        r = kvm_s390_get_clock(&tod->high, &tod->low);
+    }
+    if (r) {
+        error_setg(errp, "Unable to get KVM guest TOD clock: %s",
+                   strerror(-r));
+    }
+}
+
+static void kvm_s390_tod_set(S390TODState *td, S390TOD *tod, Error **errp)
+{
+    int r;
+
+    r = kvm_s390_set_clock_ext(&tod->high, &tod->low);
+    if (r == -ENXIO) {
+        r = kvm_s390_set_clock(&tod->high, &tod->low);
+    }
+    if (r) {
+        error_setg(errp, "Unable to set KVM guest TOD clock: %s",
+                   strerror(-r));
+    }
+}
+
+static void kvm_s390_tod_class_init(ObjectClass *oc, void *data)
+{
+    S390TODClass *tdc = S390_TOD_CLASS(oc);
+
+    tdc->get = kvm_s390_tod_get;
+    tdc->set = kvm_s390_tod_set;
+}
+
+static TypeInfo kvm_s390_tod_info = {
+    .name = TYPE_KVM_S390_TOD,
+    .parent = TYPE_S390_TOD,
+    .instance_size = sizeof(S390TODState),
+    .class_init = kvm_s390_tod_class_init,
+    .class_size = sizeof(S390TODClass),
+};
+
+static void register_types(void)
+{
+    type_register_static(&kvm_s390_tod_info);
+}
+type_init(register_types);
diff --git a/hw/s390x/tod-qemu.c b/hw/s390x/tod-qemu.c
new file mode 100644
index 0000000000..7997ba2b1a
--- /dev/null
+++ b/hw/s390x/tod-qemu.c
@@ -0,0 +1,47 @@
+/*
+ * TOD (Time Of Day) clock - QEMU implementation
+ *
+ * Copyright 2018 Red Hat, Inc.
+ * Author(s): David Hildenbrand <david@redhat.com>
+ *
+ * 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 "qapi/error.h"
+#include "hw/s390x/tod.h"
+
+static void qemu_s390_tod_get(S390TODState *td, S390TOD *tod, Error **errp)
+{
+    /* FIXME */
+    tod->high = 0;
+    tod->low = 0;
+}
+
+static void qemu_s390_tod_set(S390TODState *td, S390TOD *tod, Error **errp)
+{
+    /* FIXME */
+}
+
+static void qemu_s390_tod_class_init(ObjectClass *oc, void *data)
+{
+    S390TODClass *tdc = S390_TOD_CLASS(oc);
+
+    tdc->get = qemu_s390_tod_get;
+    tdc->set = qemu_s390_tod_set;
+}
+
+static TypeInfo qemu_s390_tod_info = {
+    .name = TYPE_QEMU_S390_TOD,
+    .parent = TYPE_S390_TOD,
+    .instance_size = sizeof(S390TODState),
+    .class_init = qemu_s390_tod_class_init,
+    .class_size = sizeof(S390TODClass),
+};
+
+static void register_types(void)
+{
+    type_register_static(&qemu_s390_tod_info);
+}
+type_init(register_types);
diff --git a/hw/s390x/tod.c b/hw/s390x/tod.c
new file mode 100644
index 0000000000..c1b3438452
--- /dev/null
+++ b/hw/s390x/tod.c
@@ -0,0 +1,120 @@
+/*
+ * TOD (Time Of Day) clock
+ *
+ * Copyright 2018 Red Hat, Inc.
+ * Author(s): David Hildenbrand <david@redhat.com>
+ *
+ * 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 "hw/s390x/tod.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "sysemu/kvm.h"
+#include "migration/register.h"
+
+void s390_init_tod(void)
+{
+    Object *obj;
+
+    if (kvm_enabled()) {
+        obj = object_new(TYPE_KVM_S390_TOD);
+    } else {
+        obj = object_new(TYPE_QEMU_S390_TOD);
+    }
+    object_property_add_child(qdev_get_machine(), TYPE_S390_TOD, obj, NULL);
+    object_unref(obj);
+
+    qdev_init_nofail(DEVICE(obj));
+}
+
+#define S390_TOD_CLOCK_VALUE_MISSING    0x00
+#define S390_TOD_CLOCK_VALUE_PRESENT    0x01
+
+static void s390_tod_save(QEMUFile *f, void *opaque)
+{
+    S390TODState *td = opaque;
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    Error *err = NULL;
+    S390TOD tod;
+
+    tdc->get(td, &tod, &err);
+    if (err) {
+        warn_report_err(err);
+        error_printf("Guest clock will not be migrated "
+                     "which could cause the guest to hang.");
+        qemu_put_byte(f, S390_TOD_CLOCK_VALUE_MISSING);
+        return;
+    }
+
+    qemu_put_byte(f, S390_TOD_CLOCK_VALUE_PRESENT);
+    qemu_put_byte(f, tod.high);
+    qemu_put_be64(f, tod.low);
+}
+
+static int s390_tod_load(QEMUFile *f, void *opaque, int version_id)
+{
+    S390TODState *td = opaque;
+    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
+    Error *err = NULL;
+    S390TOD tod;
+
+    if (qemu_get_byte(f) == S390_TOD_CLOCK_VALUE_MISSING) {
+        warn_report("Guest clock was not migrated. This could "
+                    "cause the guest to hang.");
+        return 0;
+    }
+
+    tod.high = qemu_get_byte(f);
+    tod.low = qemu_get_be64(f);
+
+    tdc->set(td, &tod, &err);
+    if (err) {
+        error_report_err(err);
+        return -1;
+    }
+    return 0;
+}
+
+static SaveVMHandlers savevm_tod = {
+    .save_state = s390_tod_save,
+    .load_state = s390_tod_load,
+};
+
+static void s390_tod_realize(DeviceState *dev, Error **errp)
+{
+    S390TODState *td = S390_TOD(dev);
+
+    /* Legacy migration interface */
+    register_savevm_live(NULL, "todclock", 0, 1, &savevm_tod, td);
+}
+
+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;
+}
+
+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);
diff --git a/include/hw/s390x/tod.h b/include/hw/s390x/tod.h
new file mode 100644
index 0000000000..43ed71600f
--- /dev/null
+++ b/include/hw/s390x/tod.h
@@ -0,0 +1,46 @@
+/*
+ * TOD (Time Of Day) clock
+ *
+ * Copyright 2018 Red Hat, Inc.
+ * Author(s): David Hildenbrand <david@redhat.com>
+ *
+ * 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;
+} 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
+
+typedef struct S390TODState {
+    /* private */
+    DeviceState parent_obj;
+} S390TODState;
+
+typedef struct S390TODClass {
+    /* private */
+    DeviceClass parent_class;
+
+    /* public */
+    void (*get)(S390TODState *td, S390TOD *tod, Error **errp);
+    void (*set)(S390TODState *td, S390TOD *tod, Error **errp);
+} S390TODClass;
+
+void s390_init_tod(void);
+
+#endif
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index c2b775f4eb..167d089722 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -391,38 +391,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()) {
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 6629a533f3..ac51c17fb4 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -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);
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index c383bf4ee9..8567aa49f7 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -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);
-- 
2.17.0

[Qemu-devel] [PATCH v1 2/7] s390x/tcg: drop tod_basetime

[David Hildenbrand]

Never set to anything but 0.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.c         | 1 -
 target/s390x/cpu.h         | 1 -
 target/s390x/misc_helper.c | 4 ++--
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 167d089722..557074165a 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -290,7 +290,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);
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index ac51c17fb4..4abfe88a3d 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -131,7 +131,6 @@ struct CPUS390XState {
 #endif
 
     uint64_t tod_offset;
-    uint64_t tod_basetime;
     QEMUTimer *tod_timer;
 
     QEMUTimer *cpu_timer;
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 1f834f35ef..044ffc64c4 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -142,7 +142,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;
 }
@@ -162,7 +162,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 */
-- 
2.17.0

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

[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.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/tod-qemu.c        | 45 ++++++++++++++++++++++++++++++++++----
 hw/s390x/tod.c             | 11 ++++++++++
 include/hw/s390x/tod.h     | 19 ++++++++++++++++
 target/s390x/cpu.c         |  8 +------
 target/s390x/cpu.h         |  6 ++++-
 target/s390x/internal.h    | 15 -------------
 target/s390x/misc_helper.c | 32 ++++++++++++++++++++++-----
 7 files changed, 103 insertions(+), 33 deletions(-)

diff --git a/hw/s390x/tod-qemu.c b/hw/s390x/tod-qemu.c
index 7997ba2b1a..41388f4c7d 100644
--- a/hw/s390x/tod-qemu.c
+++ b/hw/s390x/tod-qemu.c
@@ -11,17 +11,40 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/s390x/tod.h"
+#include "qemu/timer.h"
+#include "qemu/cutils.h"
+#include "cpu.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 +55,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),
 };
diff --git a/hw/s390x/tod.c b/hw/s390x/tod.c
index c1b3438452..7495d8c057 100644
--- a/hw/s390x/tod.c
+++ b/hw/s390x/tod.c
@@ -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
 
diff --git a/include/hw/s390x/tod.h b/include/hw/s390x/tod.h
index 43ed71600f..5491245b86 100644
--- a/include/hw/s390x/tod.h
+++ b/include/hw/s390x/tod.h
@@ -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
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 557074165a..e4d7e82a23 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -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"
@@ -44,6 +43,7 @@
 #include "hw/hw.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/sysemu.h"
+#include "hw/s390x/s390-virtio-ccw.h"
 #endif
 #include "fpu/softfloat.h"
 
@@ -276,9 +276,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;
@@ -287,9 +284,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);
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 4abfe88a3d..810d09c4df 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -130,7 +130,6 @@ struct CPUS390XState {
     uint64_t cpuid;
 #endif
 
-    uint64_t tod_offset;
     QEMUTimer *tod_timer;
 
     QEMUTimer *cpu_timer;
@@ -765,6 +764,11 @@ void s390_program_interrupt(CPUS390XState *env, uint32_t code, int ilen,
 /* service interrupts are floating therefore we must not pass an cpustate */
 void s390_sclp_extint(uint32_t parm);
 
+
+/* misc_helper.c */
+void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque);
+
+
 /* mmu_helper.c */
 int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
                          int len, bool is_write);
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index e392a02d12..f2a771e2b4 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -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)
 {
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 044ffc64c4..418326e551 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -40,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 */
@@ -139,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);
@@ -165,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)
 {
-- 
2.17.0

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

[David Hildenbrand]

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

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/misc_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 418326e551..2ef9de1cca 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -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;
     }
-- 
2.17.0

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

[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.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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(-)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 59cba86a27..97c60ca7bc 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -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)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 157619403d..5c6f33ed9c 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -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 */
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 2ef9de1cca..2f5cd6d22a 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -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)
 {
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 82309faa11..c6ed9507de 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -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);
-- 
2.17.0

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

[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.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/machine.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/s390x/machine.c b/target/s390x/machine.c
index 84b4928755..53d226ff48 100644
--- a/target/s390x/machine.c
+++ b/target/s390x/machine.c
@@ -34,6 +34,13 @@ static int cpu_post_load(void *opaque, int version_id)
         return kvm_s390_vcpu_interrupt_post_load(cpu);
     }
 
+#ifdef CONFIG_TCG
+    if (tcg_enabled()) {
+        /* Rearm the CKC timer if necessary */
+        tcg_s390_tod_updated(CPU(cpu), RUN_ON_CPU_NULL);
+    }
+#endif
+
     return 0;
 }
 
-- 
2.17.0

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

[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.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index e4d7e82a23..5fceed7948 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -220,11 +220,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:
-- 
2.17.0

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

[Cornelia Huck]

On Fri, 25 May 2018 13:37:02 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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.

Can't you include it in tod-kvm.c instead? Why was it not needed before?

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  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

Otherwise, looks good.

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

[Cornelia Huck]

On Fri, 25 May 2018 13:37:04 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/tod-qemu.c        | 45 ++++++++++++++++++++++++++++++++++----
>  hw/s390x/tod.c             | 11 ++++++++++
>  include/hw/s390x/tod.h     | 19 ++++++++++++++++
>  target/s390x/cpu.c         |  8 +------
>  target/s390x/cpu.h         |  6 ++++-
>  target/s390x/internal.h    | 15 -------------
>  target/s390x/misc_helper.c | 32 ++++++++++++++++++++++-----
>  7 files changed, 103 insertions(+), 33 deletions(-)

> diff --git a/include/hw/s390x/tod.h b/include/hw/s390x/tod.h
> index 43ed71600f..5491245b86 100644
> --- a/include/hw/s390x/tod.h
> +++ b/include/hw/s390x/tod.h
> @@ -30,6 +30,9 @@ typedef struct S390TOD {
>  typedef struct S390TODState {
>      /* private */
>      DeviceState parent_obj;
> +
> +    /* unused by KVM implementation */
> +    S390TOD base;

Does this need some kind of migration handling?

>  } S390TODState;
>  
>  typedef struct S390TODClass {

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

[David Hildenbrand]

On 04.06.2018 13:57, Cornelia Huck wrote:
> On Fri, 25 May 2018 13:37:04 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> 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.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/tod-qemu.c        | 45 ++++++++++++++++++++++++++++++++++----
>>  hw/s390x/tod.c             | 11 ++++++++++
>>  include/hw/s390x/tod.h     | 19 ++++++++++++++++
>>  target/s390x/cpu.c         |  8 +------
>>  target/s390x/cpu.h         |  6 ++++-
>>  target/s390x/internal.h    | 15 -------------
>>  target/s390x/misc_helper.c | 32 ++++++++++++++++++++++-----
>>  7 files changed, 103 insertions(+), 33 deletions(-)
> 
>> diff --git a/include/hw/s390x/tod.h b/include/hw/s390x/tod.h
>> index 43ed71600f..5491245b86 100644
>> --- a/include/hw/s390x/tod.h
>> +++ b/include/hw/s390x/tod.h
>> @@ -30,6 +30,9 @@ typedef struct S390TOD {
>>  typedef struct S390TODState {
>>      /* private */
>>      DeviceState parent_obj;
>> +
>> +    /* unused by KVM implementation */
>> +    S390TOD base;
> 
> Does this need some kind of migration handling?

This will be automatically migrated by migrating the TOD value itself now :)

> 
>>  } S390TODState;
>>  
>>  typedef struct S390TODClass {


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 04.06.2018 13:38, Cornelia Huck wrote:
> On Fri, 25 May 2018 13:37:02 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> 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.
> 
> Can't you include it in tod-kvm.c instead? Why was it not needed before?

Guess I can, kvm_s390x.h looks to me like the right place, as S390CPU is
used in there.

All other users include "cpu.h" already, so it isn't needed.

> 
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  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
> 
> Otherwise, looks good.
> 


-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Fri, 25 May 2018 13:37:06 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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 <david@redhat.com>
> ---
>  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(-)
> 

> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 2ef9de1cca..2f5cd6d22a 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -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);

Would it make sense to cache the class as well?

> +    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;
> +}
> +

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

[Cornelia Huck]

On Fri, 25 May 2018 13:37:07 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/machine.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/s390x/machine.c b/target/s390x/machine.c
> index 84b4928755..53d226ff48 100644
> --- a/target/s390x/machine.c
> +++ b/target/s390x/machine.c
> @@ -34,6 +34,13 @@ static int cpu_post_load(void *opaque, int version_id)
>          return kvm_s390_vcpu_interrupt_post_load(cpu);
>      }
>  
> +#ifdef CONFIG_TCG
> +    if (tcg_enabled()) {
> +        /* Rearm the CKC timer if necessary */
> +        tcg_s390_tod_updated(CPU(cpu), RUN_ON_CPU_NULL);

Stub this out so the explicit CONFIG_TCG is not required? But I don't
care too much.

> +    }
> +#endif
> +
>      return 0;
>  }
>  

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

[Cornelia Huck]

On Mon, 4 Jun 2018 14:00:43 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 04.06.2018 13:38, Cornelia Huck wrote:
> > On Fri, 25 May 2018 13:37:02 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> 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.  
> > 
> > Can't you include it in tod-kvm.c instead? Why was it not needed before?  
> 
> Guess I can, kvm_s390x.h looks to me like the right place, as S390CPU is
> used in there.
> 
> All other users include "cpu.h" already, so it isn't needed.

Hm, and for kvm_s390_irq, this file is using a forward declaration,
which would likely be more awkward for this structure.

Header dependencies are always fun :)

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

[David Hildenbrand]

On 04.06.2018 14:09, Cornelia Huck wrote:
> On Fri, 25 May 2018 13:37:07 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> 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.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/machine.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/target/s390x/machine.c b/target/s390x/machine.c
>> index 84b4928755..53d226ff48 100644
>> --- a/target/s390x/machine.c
>> +++ b/target/s390x/machine.c
>> @@ -34,6 +34,13 @@ static int cpu_post_load(void *opaque, int version_id)
>>          return kvm_s390_vcpu_interrupt_post_load(cpu);
>>      }
>>  
>> +#ifdef CONFIG_TCG
>> +    if (tcg_enabled()) {
>> +        /* Rearm the CKC timer if necessary */
>> +        tcg_s390_tod_updated(CPU(cpu), RUN_ON_CPU_NULL);
> 
> Stub this out so the explicit CONFIG_TCG is not required? But I don't
> care too much.


Yes, can do!

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 04.06.2018 14:05, Cornelia Huck wrote:
> On Fri, 25 May 2018 13:37:06 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> 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 <david@redhat.com>
>> ---
>>  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(-)
>>
> 
>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>> index 2ef9de1cca..2f5cd6d22a 100644
>> --- a/target/s390x/misc_helper.c
>> +++ b/target/s390x/misc_helper.c
>> @@ -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);
> 
> Would it make sense to cache the class as well?

Probably, but I don't think this is as critical as the flic (as this
code is TCG only and the class conversion should be fairly fast compared
to a device lookup).

> 
>> +    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;
>> +}
>> +
> 


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 04.06.2018 14:14, Cornelia Huck wrote:
> On Mon, 4 Jun 2018 14:00:43 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 04.06.2018 13:38, Cornelia Huck wrote:
>>> On Fri, 25 May 2018 13:37:02 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> 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.  
>>>
>>> Can't you include it in tod-kvm.c instead? Why was it not needed before?  
>>
>> Guess I can, kvm_s390x.h looks to me like the right place, as S390CPU is
>> used in there.
>>
>> All other users include "cpu.h" already, so it isn't needed.
> 
> Hm, and for kvm_s390_irq, this file is using a forward declaration,
> which would likely be more awkward for this structure.

I remember that we wanted to use cpu-qom.h for this typedef fun, that's
why I decided to do it this way.

> 
> Header dependencies are always fun :)
> 

So what do you suggest? Thanks!

-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Thu, 7 Jun 2018 12:49:23 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 04.06.2018 14:14, Cornelia Huck wrote:
> > On Mon, 4 Jun 2018 14:00:43 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 04.06.2018 13:38, Cornelia Huck wrote:  
> >>> On Fri, 25 May 2018 13:37:02 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> 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.    
> >>>
> >>> Can't you include it in tod-kvm.c instead? Why was it not needed before?    
> >>
> >> Guess I can, kvm_s390x.h looks to me like the right place, as S390CPU is
> >> used in there.
> >>
> >> All other users include "cpu.h" already, so it isn't needed.  
> > 
> > Hm, and for kvm_s390_irq, this file is using a forward declaration,
> > which would likely be more awkward for this structure.  
> 
> I remember that we wanted to use cpu-qom.h for this typedef fun, that's
> why I decided to do it this way.
> 
> > 
> > Header dependencies are always fun :)
> >   
> 
> So what do you suggest? Thanks!
> 

Let's go with cpu-qom.h for this, then.

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

[Cornelia Huck]

On Thu, 7 Jun 2018 12:48:21 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 04.06.2018 14:05, Cornelia Huck wrote:
> > On Fri, 25 May 2018 13:37:06 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> 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 <david@redhat.com>
> >> ---
> >>  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(-)
> >>  
> >   
> >> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> >> index 2ef9de1cca..2f5cd6d22a 100644
> >> --- a/target/s390x/misc_helper.c
> >> +++ b/target/s390x/misc_helper.c
> >> @@ -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);  
> > 
> > Would it make sense to cache the class as well?  
> 
> Probably, but I don't think this is as critical as the flic (as this
> code is TCG only and the class conversion should be fairly fast compared
> to a device lookup).

Yes, it probably does not matter that much. Do as you like :)