s390x: TOD refactoring + TCG CPU hotplug support
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
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
@@ -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
@@ -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
@@ -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
@@ -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
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.hinkvm_s390x.h(due to S390CPU) to compiletod-kvm.c.
Can't you include it in tod-kvm.c instead? Why was it not needed before?
Quoted 17 lines
Signed-off-by David Hildenbrand <[email protected]>--- 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 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.hinkvm_s390x.h(due to S390CPU) to compiletod-kvm.c.Can't you include it in
tod-kvm.cinstead? 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
Signed-off-by David Hildenbrand <[email protected]>--- 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.hOtherwise, looks good.
Signature · 3 lines
Thanks, David / dhildenb
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.hinkvm_s390x.h(due to S390CPU) to compiletod-kvm.c.Can't you include it in
tod-kvm.cinstead? Why was it not needed before?Guess I can,
kvm_s390x.hlooks 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 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.hinkvm_s390x.h(due to S390CPU) to compiletod-kvm.c.Can't you include it in
tod-kvm.cinstead? Why was it not needed before?
Quoted 7 lines
Quoted 4 lines
Guess I can,
kvm_s390x.hlooks 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
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.hinkvm_s390x.h(due to S390CPU) to compiletod-kvm.c.Can't you include it in
tod-kvm.cinstead? Why was it not needed before?Quoted 7 lines
Quoted 4 lines
Guess I can,
kvm_s390x.hlooks 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.hfor 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.
PATCH 2/7 s390x/tcg: drop tod_basetime
In reply to David HildenbrandNever 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
@@ -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
@@ -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
@@ -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
PATCH 3/7 s390x/tcg: properly implement the TOD
In reply to David HildenbrandRight 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
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.
Signed-off-by David Hildenbrand <[email protected]>--- 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
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 {
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.
Signed-off-by David Hildenbrand <[email protected]>--- 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@@ -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
PATCH 4/7 s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts
In reply to David HildenbrandLet'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
@@ -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
PATCH 5/7 s390x/tcg: implement SET CLOCK
In reply to David HildenbrandThis 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
@@ -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
@@ -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
@@ -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
@@ -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
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_ALIGNto load the operand manually - this will properly trigger a SPECIFICATION exception.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 3 +-- target/s390x/misc_helper.c | 19 +++++++++++++++++++ target/s390x/translate.c | 9 +++++++++ 4 files changed, 30 insertions(+), 2 deletions(-)target/s390x/misc_helper.c +0
-0
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 ; +} +
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_ALIGNto load the operand manually - this will properly trigger a SPECIFICATION exception.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 3 +-- target/s390x/misc_helper.c | 19 +++++++++++++++++++ target/s390x/translate.c | 9 +++++++++ 4 files changed, 30 insertions(+), 2 deletions(-)target/s390x/misc_helper.c +0
-0@@ -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
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_ALIGNto load the operand manually - this will properly trigger a SPECIFICATION exception.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 3 +-- target/s390x/misc_helper.c | 19 +++++++++++++++++++ target/s390x/translate.c | 9 +++++++++ 4 files changed, 30 insertions(+), 2 deletions(-)target/s390x/misc_helper.c +0
-0@@ -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 :)
PATCH 6/7 s390x/tcg: rearm the CKC timer during migration
In reply to David HildenbrandIf 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
@@ -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
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.
Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/machine.c | 7 +++++++ 1 file changed, 7 insertions(+)target/s390x/machine.c +0
-0
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 ; }
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.
Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/machine.c | 7 +++++++ 1 file changed, 7 insertions(+)target/s390x/machine.c +0
-0@@ -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_TCGis not required? But I don't care too much.
Yes, can do!
Signature · 3 lines
Thanks, David / dhildenb
PATCH 7/7 s390x/tcg: fix CPU hotplug with single-threaded TCG
In reply to David Hildenbrandrun_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
@@ -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