Skip to contentSkip to conversation

s390x: TOD refactoring + TCG CPU hotplug support

PATCH Superseded David Hildenbrand 2 participants 20 messages May 25, 2018 – Jun 7, 2018

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

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

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
Signature · 1 line
2.17.0
#1 lore raw

PATCH 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 c2b775f4eb..167d089722 100644
@@ -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()) {
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.0
#2 lore raw

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

Quoted 4 lines · On Fri, 25 May 2018 13:37:02 +0200 David Hildenbrand <[email protected]> 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?

Quoted 17 lines
---
 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.

#3 lore raw

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

Quoted 9 lines · Cornelia Huck
Quoted 4 lines · On Fri, 25 May 2018 13:37:02 +0200 David Hildenbrand <[email protected]> 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.

Quoted 21 lines
Quoted 18 lines
---
 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.

Signature · 3 lines
Thanks,

David / dhildenb
#4 lore raw

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

Quoted 7 lines · On Mon, 4 Jun 2018 14:00:43 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 6 lines · Cornelia Huck
Quoted 3 lines · On Fri, 25 May 2018 13:37:02 +0200 David Hildenbrand <[email protected]> wrote:

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

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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

#5 lore raw

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

Quoted 14 lines · Cornelia Huck
Quoted 11 lines · On Mon, 4 Jun 2018 14:00:43 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 9 lines · Cornelia Huck
Quoted 4 lines · On Fri, 25 May 2018 13:37:02 +0200 David Hildenbrand <[email protected]> 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?

Quoted 7 lines
Quoted 4 lines

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.

Quoted 3 lines

Header dependencies are always fun :)

So what do you suggest? Thanks!

Signature · 3 lines
Thanks,

David / dhildenb
#6 lore raw

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

Quoted 23 lines · On Thu, 7 Jun 2018 12:49:23 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 14 lines · Cornelia Huck
Quoted 11 lines · On Mon, 4 Jun 2018 14:00:43 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 9 lines · Cornelia Huck
Quoted 4 lines · On Fri, 25 May 2018 13:37:02 +0200 David Hildenbrand <[email protected]> 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?

Quoted 7 lines
Quoted 4 lines

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.

Quoted 9 lines

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

Quoted 3 lines

Header dependencies are always fun :)

So what do you suggest? Thanks!

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

#7 lore raw

PATCH 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 167d089722..557074165a 100644
@@ -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);
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 1f834f35ef..044ffc64c4 100644
@@ -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 */
Signature · 1 line
2.17.0
#8 lore raw

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

---
 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(-)
hw/s390x/tod-qemu.c +41 -4
index 7997ba2b1a..41388f4c7d 100644
@@ -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),
 };
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 +1 -7
index 557074165a..e4d7e82a23 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"
@@ -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);
target/s390x/cpu.h +5 -1
index 4abfe88a3d..810d09c4df 100644
@@ -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);
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 +26 -6
index 044ffc64c4..418326e551 100644
@@ -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)
 {
Signature · 1 line
2.17.0
#9 lore raw

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

Quoted 26 lines · On Fri, 25 May 2018 13:37:04 +0200 David Hildenbrand <[email protected]> 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.

---
 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(-)
include/hw/s390x/tod.h +0 -0
index 43ed71600f..5491245b86 100644

include/hw/s390x/tod.h typedef struct S390TOD { · line 35

Quoted 6 lines
include/hw/s390x/tod.h +3 -0
@@ -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?

Quoted 3 lines
include/hw/s390x/tod.h +0 -0
 } S390TODState;
 
 typedef struct S390TODClass {
#10 lore raw

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

Quoted 24 lines · Cornelia Huck
Quoted 20 lines · On Fri, 25 May 2018 13:37:04 +0200 David Hildenbrand <[email protected]> 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.

---
 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(-)
Quoted 13 lines

include/hw/s390x/tod.h typedef struct S390TOD { · line 35

Quoted 11 lines
include/hw/s390x/tod.h +3 -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;

Does this need some kind of migration handling?

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

Quoted 4 lines
Quoted 4 lines
include/hw/s390x/tod.h +0 -0
 } S390TODState;
 
 typedef struct S390TODClass {
Signature · 3 lines
Thanks,

David / dhildenb
#11 lore raw

PATCH 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 418326e551..2ef9de1cca 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.0
#12 lore raw

PATCH 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 2ef9de1cca..2f5cd6d22a 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 82309faa11..c6ed9507de 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.0
#13 lore raw

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

Quoted 20 lines · On Fri, 25 May 2018 13:37:06 +0200 David Hildenbrand <[email protected]> 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.

---
 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/misc_helper.c +0 -0
index 2ef9de1cca..2f5cd6d22a 100644

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

Quoted 8 lines
target/s390x/misc_helper.c +5 -0
@@ -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?

Quoted 14 lines
target/s390x/misc_helper.c +14 -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);
+
+    return 0;
+}
+
#14 lore raw

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

Quoted 26 lines · Cornelia Huck
Quoted 23 lines · On Fri, 25 May 2018 13:37:06 +0200 David Hildenbrand <[email protected]> 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.

---
 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/misc_helper.c +0 -0
index 2ef9de1cca..2f5cd6d22a 100644
@@ -191,6 +191,25 @@ void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque)
     helper_sckc(env, env->ckc);
 }
 
Quoted 7 lines

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

Quoted 5 lines
target/s390x/misc_helper.c +5 -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);

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

Quoted 16 lines
Quoted 15 lines
target/s390x/misc_helper.c +14 -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);
+
+    return 0;
+}
+
Signature · 3 lines
Thanks,

David / dhildenb
#15 lore raw

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

Quoted 35 lines · On Thu, 7 Jun 2018 12:48:21 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 26 lines · Cornelia Huck
Quoted 23 lines · On Fri, 25 May 2018 13:37:06 +0200 David Hildenbrand <[email protected]> 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.

---
 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/misc_helper.c +0 -0
index 2ef9de1cca..2f5cd6d22a 100644
@@ -191,6 +191,25 @@ void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque)
     helper_sckc(env, env->ckc);
 }
 
Quoted 7 lines

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

Quoted 5 lines
target/s390x/misc_helper.c +5 -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);  

Would it make sense to cache the class as well?

Quoted 3 lines

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

#16 lore raw

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

---
 target/s390x/machine.c | 7 +++++++
 1 file changed, 7 insertions(+)
target/s390x/machine.c +7 -0
index 84b4928755..53d226ff48 100644
@@ -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;
 }
 
Signature · 1 line
2.17.0
#17 lore raw

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

Quoted 13 lines · On Fri, 25 May 2018 13:37:07 +0200 David Hildenbrand <[email protected]> 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.

---
 target/s390x/machine.c | 7 +++++++
 1 file changed, 7 insertions(+)
target/s390x/machine.c +0 -0
index 84b4928755..53d226ff48 100644

target/s390x/machine.c static int cpu_post_load(void *opaque, int version_id) · line 40

Quoted 7 lines
target/s390x/machine.c +4 -0
@@ -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.

Quoted 6 lines
target/s390x/machine.c +3 -0
+    }
+#endif
+
     return 0;
 }
 
#18 lore raw

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

Quoted 19 lines · Cornelia Huck
Quoted 16 lines · On Fri, 25 May 2018 13:37:07 +0200 David Hildenbrand <[email protected]> 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.

---
 target/s390x/machine.c | 7 +++++++
 1 file changed, 7 insertions(+)
target/s390x/machine.c +0 -0
index 84b4928755..53d226ff48 100644
@@ -34,6 +34,13 @@ static int cpu_post_load(void *opaque, int version_id)
         return kvm_s390_vcpu_interrupt_post_load(cpu);
     }
 
Quoted 7 lines

target/s390x/machine.c static int cpu_post_load(void *opaque, int version_id) · line 40

Quoted 4 lines
target/s390x/machine.c +4 -0
+#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!

Signature · 3 lines
Thanks,

David / dhildenb
#19 lore raw

PATCH 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 e4d7e82a23..5fceed7948 100644
@@ -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:
Signature · 1 line
2.17.0