| Message ID | 20180625115352.6889-6-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: TOD refactoring + TCG CPU hotplug support | expand |
On 25.06.2018 13:53, David Hildenbrand 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. > > Introduce "tcg_s390x.h" just like "kvm_s390x.h" for tcg specific > function declarations that should not go into cpu.h. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/tod-qemu.c | 46 ++++++++++++++++++++++++++++++++++---- > hw/s390x/tod.c | 11 +++++++++ > include/hw/s390x/tod.h | 19 ++++++++++++++++ > target/s390x/cpu.c | 7 ------ > target/s390x/cpu.h | 1 - > target/s390x/internal.h | 16 ------------- > target/s390x/misc_helper.c | 30 ++++++++++++++++++++----- > target/s390x/tcg_s390x.h | 18 +++++++++++++++ > 8 files changed, 114 insertions(+), 34 deletions(-) > create mode 100644 target/s390x/tcg_s390x.h > > diff --git a/hw/s390x/tod-qemu.c b/hw/s390x/tod-qemu.c > index 03ea1ce4e8..941e84693e 100644 > --- a/hw/s390x/tod-qemu.c > +++ b/hw/s390x/tod-qemu.c > @@ -11,19 +11,43 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "hw/s390x/tod.h" > +#include "qemu/timer.h" > +#include "qemu/cutils.h" > +#include "cpu.h" > +#include "tcg_s390x.h" > > static void qemu_s390_tod_get(const S390TODState *td, S390TOD *tod, > Error **errp) > { > - /* FIXME */ > - tod->high = 0; > - tod->low = 0; > + *tod = td->base; > + > + tod->low += time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); > + if (tod->low < td->base.low) { > + tod->high++; > + } > } > > static void qemu_s390_tod_set(S390TODState *td, const 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) { Just a matter of taste, but I'd rather use "td->base.low > tod->low" here instead (since the operations before and after the if-statement are related to td->base). > + td->base.high--; > + } > + > + /* > + * The TOD has been changed and we have to recalculate the CKC values > + * for all CPUs. We do this asynchronously, as "SET CLOCK should be > + * issued only while all other activity on all CPUs .. has been > + * suspended". > + */ > + CPU_FOREACH(cpu) { > + async_run_on_cpu(cpu, tcg_s390_tod_updated, RUN_ON_CPU_NULL); > + } > } > > static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) > @@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) > tdc->set = qemu_s390_tod_set; > } > > +static void qemu_s390_tod_init(Object *obj) > +{ > + S390TODState *td = S390_TOD(obj); > + struct tm tm; > + > + qemu_get_timedate(&tm, 0); > + td->base.high = 0; > + td->base.low = TOD_UNIX_EPOCH + (time2tod(mktimegm(&tm)) * 1000000000ULL); > + if (td->base.low < TOD_UNIX_EPOCH) { > + td->base.high += 1; > + } > +} Nit: It would be sufficient to do this in the realize() function instead. > 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/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > index dd5273949b..be341b5295 100644 > --- a/target/s390x/misc_helper.c > +++ b/target/s390x/misc_helper.c > @@ -28,6 +28,8 @@ > #include "qemu/timer.h" > #include "exec/exec-all.h" > #include "exec/cpu_ldst.h" > +#include "qapi/error.h" > +#include "tcg_s390x.h" > > #if !defined(CONFIG_USER_ONLY) > #include "sysemu/cpus.h" > @@ -39,6 +41,7 @@ > #include "hw/s390x/ioinst.h" > #include "hw/s390x/s390-pci-inst.h" > #include "hw/boards.h" > +#include "hw/s390x/tod.h" > #endif > > /* #define DEBUG_HELPER */ > @@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1) > /* Store Clock */ > uint64_t HELPER(stck)(CPUS390XState *env) > { > - uint64_t time; > + S390TODState *td = s390_get_todstate(); > + S390TODClass *tdc = S390_TOD_GET_CLASS(td); > + S390TOD tod; > > - time = env->tod_offset + > - time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); > - > - return time; > + tdc->get(td, &tod, &error_abort); > + return tod.low; > } > > /* Set Clock Comparator */ > void HELPER(sckc)(CPUS390XState *env, uint64_t time) > { > + S390TODState *td = s390_get_todstate(); > + S390TODClass *tdc = S390_TOD_GET_CLASS(td); > + S390TOD tod_base; > + > if (time == -1ULL) { > return; > } > > env->ckc = time; > > + tdc->get(td, &tod_base, &error_abort); > + tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); So the tdc->get first adds the time2tod, and then you subtract it here again? Can't you simply use td->base.low directly instead? > /* difference between origins */ > - time -= env->tod_offset; > + time -= tod_base.low; > > /* nanoseconds */ > time = tod2time(time); > @@ -164,6 +174,14 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time) > timer_mod(env->tod_timer, time); > } ... I found only nits, so with or without my suggested modifications: Reviewed-by: Thomas Huth <thuth@redhat.com>
>> + td->base = *tod; >> + >> + td->base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); >> + if (tod->low < td->base.low) { > > Just a matter of taste, but I'd rather use "td->base.low > tod->low" > here instead (since the operations before and after the if-statement are > related to td->base). Changed. > >> + td->base.high--; >> + } >> + >> + /* >> + * The TOD has been changed and we have to recalculate the CKC values >> + * for all CPUs. We do this asynchronously, as "SET CLOCK should be >> + * issued only while all other activity on all CPUs .. has been >> + * suspended". >> + */ >> + CPU_FOREACH(cpu) { >> + async_run_on_cpu(cpu, tcg_s390_tod_updated, RUN_ON_CPU_NULL); >> + } >> } >> >> static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) >> @@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) >> tdc->set = qemu_s390_tod_set; >> } >> >> +static void qemu_s390_tod_init(Object *obj) >> +{ >> + S390TODState *td = S390_TOD(obj); >> + struct tm tm; >> + >> + qemu_get_timedate(&tm, 0); >> + td->base.high = 0; >> + td->base.low = TOD_UNIX_EPOCH + (time2tod(mktimegm(&tm)) * 1000000000ULL); >> + if (td->base.low < TOD_UNIX_EPOCH) { >> + td->base.high += 1; >> + } >> +} > > Nit: It would be sufficient to do this in the realize() function instead. Then I'll have to overwrite the realize function and store the parent_realize function - something that I want to avoid if not really necessary. (for now it was also done in the cpu initfn, so that should be fine) > >> 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/target/s390x/misc_helper.c b/target/s390x/misc_helper.c >> index dd5273949b..be341b5295 100644 >> --- a/target/s390x/misc_helper.c >> +++ b/target/s390x/misc_helper.c >> @@ -28,6 +28,8 @@ >> #include "qemu/timer.h" >> #include "exec/exec-all.h" >> #include "exec/cpu_ldst.h" >> +#include "qapi/error.h" >> +#include "tcg_s390x.h" >> >> #if !defined(CONFIG_USER_ONLY) >> #include "sysemu/cpus.h" >> @@ -39,6 +41,7 @@ >> #include "hw/s390x/ioinst.h" >> #include "hw/s390x/s390-pci-inst.h" >> #include "hw/boards.h" >> +#include "hw/s390x/tod.h" >> #endif >> >> /* #define DEBUG_HELPER */ >> @@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1) >> /* Store Clock */ >> uint64_t HELPER(stck)(CPUS390XState *env) >> { >> - uint64_t time; >> + S390TODState *td = s390_get_todstate(); >> + S390TODClass *tdc = S390_TOD_GET_CLASS(td); >> + S390TOD tod; >> >> - time = env->tod_offset + >> - time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); >> - >> - return time; >> + tdc->get(td, &tod, &error_abort); >> + return tod.low; >> } >> >> /* Set Clock Comparator */ >> void HELPER(sckc)(CPUS390XState *env, uint64_t time) >> { >> + S390TODState *td = s390_get_todstate(); >> + S390TODClass *tdc = S390_TOD_GET_CLASS(td); >> + S390TOD tod_base; >> + >> if (time == -1ULL) { >> return; >> } >> >> env->ckc = time; >> >> + tdc->get(td, &tod_base, &error_abort); >> + tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); > > So the tdc->get first adds the time2tod, and then you subtract it here > again? Can't you simply use td->base.low directly instead? That might be a good idea, previously I had tdc->get_base(), but dropped it because it cannot be implemented for KVM. Simply accessing the member here should be fine. Thanks! > >> /* difference between origins */ >> - time -= env->tod_offset; >> + time -= tod_base.low; >> >> /* nanoseconds */ >> time = tod2time(time); >> @@ -164,6 +174,14 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time) >> timer_mod(env->tod_timer, time); >> } > > ... I found only nits, so with or without my suggested modifications: > > Reviewed-by: Thomas Huth <thuth@redhat.com> >
On Tue, 26 Jun 2018 14:06:00 +0200 David Hildenbrand <david@redhat.com> wrote: > >> @@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) > >> tdc->set = qemu_s390_tod_set; > >> } > >> > >> +static void qemu_s390_tod_init(Object *obj) > >> +{ > >> + S390TODState *td = S390_TOD(obj); > >> + struct tm tm; > >> + > >> + qemu_get_timedate(&tm, 0); > >> + td->base.high = 0; > >> + td->base.low = TOD_UNIX_EPOCH + (time2tod(mktimegm(&tm)) * 1000000000ULL); > >> + if (td->base.low < TOD_UNIX_EPOCH) { > >> + td->base.high += 1; > >> + } > >> +} > > > > Nit: It would be sufficient to do this in the realize() function instead. > > Then I'll have to overwrite the realize function and store the > parent_realize function - something that I want to avoid if not really > necessary. Agreed. I'd just leave it as it is now. > > (for now it was also done in the cpu initfn, so that should be fine) > > > > >> 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/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > >> index dd5273949b..be341b5295 100644 > >> --- a/target/s390x/misc_helper.c > >> +++ b/target/s390x/misc_helper.c > >> @@ -28,6 +28,8 @@ > >> #include "qemu/timer.h" > >> #include "exec/exec-all.h" > >> #include "exec/cpu_ldst.h" > >> +#include "qapi/error.h" > >> +#include "tcg_s390x.h" > >> > >> #if !defined(CONFIG_USER_ONLY) > >> #include "sysemu/cpus.h" > >> @@ -39,6 +41,7 @@ > >> #include "hw/s390x/ioinst.h" > >> #include "hw/s390x/s390-pci-inst.h" > >> #include "hw/boards.h" > >> +#include "hw/s390x/tod.h" > >> #endif > >> > >> /* #define DEBUG_HELPER */ > >> @@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1) > >> /* Store Clock */ > >> uint64_t HELPER(stck)(CPUS390XState *env) > >> { > >> - uint64_t time; > >> + S390TODState *td = s390_get_todstate(); > >> + S390TODClass *tdc = S390_TOD_GET_CLASS(td); > >> + S390TOD tod; > >> > >> - time = env->tod_offset + > >> - time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); > >> - > >> - return time; > >> + tdc->get(td, &tod, &error_abort); > >> + return tod.low; > >> } > >> > >> /* Set Clock Comparator */ > >> void HELPER(sckc)(CPUS390XState *env, uint64_t time) > >> { > >> + S390TODState *td = s390_get_todstate(); > >> + S390TODClass *tdc = S390_TOD_GET_CLASS(td); > >> + S390TOD tod_base; > >> + > >> if (time == -1ULL) { > >> return; > >> } > >> > >> env->ckc = time; > >> > >> + tdc->get(td, &tod_base, &error_abort); > >> + tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); > > > > So the tdc->get first adds the time2tod, and then you subtract it here > > again? Can't you simply use td->base.low directly instead? > > That might be a good idea, previously I had tdc->get_base(), but dropped > it because it cannot be implemented for KVM. > > Simply accessing the member here should be fine. Thanks! So, we're guaranteed to have td->base.low at the correct value? (Sorry, having a bit of a hard time following around here.) > > > > >> /* difference between origins */ > >> - time -= env->tod_offset; > >> + time -= tod_base.low; > >> > >> /* nanoseconds */ > >> time = tod2time(time); > >> @@ -164,6 +174,14 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time) > >> timer_mod(env->tod_timer, time); > >> } > > > > ... I found only nits, so with or without my suggested modifications: > > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > >
On 26.06.2018 14:27, Cornelia Huck wrote: > On Tue, 26 Jun 2018 14:06:00 +0200 > David Hildenbrand <david@redhat.com> wrote: > >>>> @@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) >>>> tdc->set = qemu_s390_tod_set; >>>> } >>>> >>>> +static void qemu_s390_tod_init(Object *obj) >>>> +{ >>>> + S390TODState *td = S390_TOD(obj); >>>> + struct tm tm; >>>> + >>>> + qemu_get_timedate(&tm, 0); >>>> + td->base.high = 0; >>>> + td->base.low = TOD_UNIX_EPOCH + (time2tod(mktimegm(&tm)) * 1000000000ULL); >>>> + if (td->base.low < TOD_UNIX_EPOCH) { >>>> + td->base.high += 1; >>>> + } >>>> +} >>> >>> Nit: It would be sufficient to do this in the realize() function instead. >> >> Then I'll have to overwrite the realize function and store the >> parent_realize function - something that I want to avoid if not really >> necessary. > > Agreed. I'd just leave it as it is now. > >> >> (for now it was also done in the cpu initfn, so that should be fine) >> >>> >>>> 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/target/s390x/misc_helper.c b/target/s390x/misc_helper.c >>>> index dd5273949b..be341b5295 100644 >>>> --- a/target/s390x/misc_helper.c >>>> +++ b/target/s390x/misc_helper.c >>>> @@ -28,6 +28,8 @@ >>>> #include "qemu/timer.h" >>>> #include "exec/exec-all.h" >>>> #include "exec/cpu_ldst.h" >>>> +#include "qapi/error.h" >>>> +#include "tcg_s390x.h" >>>> >>>> #if !defined(CONFIG_USER_ONLY) >>>> #include "sysemu/cpus.h" >>>> @@ -39,6 +41,7 @@ >>>> #include "hw/s390x/ioinst.h" >>>> #include "hw/s390x/s390-pci-inst.h" >>>> #include "hw/boards.h" >>>> +#include "hw/s390x/tod.h" >>>> #endif >>>> >>>> /* #define DEBUG_HELPER */ >>>> @@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1) >>>> /* Store Clock */ >>>> uint64_t HELPER(stck)(CPUS390XState *env) >>>> { >>>> - uint64_t time; >>>> + S390TODState *td = s390_get_todstate(); >>>> + S390TODClass *tdc = S390_TOD_GET_CLASS(td); >>>> + S390TOD tod; >>>> >>>> - time = env->tod_offset + >>>> - time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); >>>> - >>>> - return time; >>>> + tdc->get(td, &tod, &error_abort); >>>> + return tod.low; >>>> } >>>> >>>> /* Set Clock Comparator */ >>>> void HELPER(sckc)(CPUS390XState *env, uint64_t time) >>>> { >>>> + S390TODState *td = s390_get_todstate(); >>>> + S390TODClass *tdc = S390_TOD_GET_CLASS(td); >>>> + S390TOD tod_base; >>>> + >>>> if (time == -1ULL) { >>>> return; >>>> } >>>> >>>> env->ckc = time; >>>> >>>> + tdc->get(td, &tod_base, &error_abort); >>>> + tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); >>> >>> So the tdc->get first adds the time2tod, and then you subtract it here >>> again? Can't you simply use td->base.low directly instead? >> >> That might be a good idea, previously I had tdc->get_base(), but dropped >> it because it cannot be implemented for KVM. >> >> Simply accessing the member here should be fine. Thanks! > > So, we're guaranteed to have td->base.low at the correct value? (Sorry, > having a bit of a hard time following around here.) > Yes, just verified, changed and tested. We will have the value at that place. I'll give some more time to review the other parts, then I can resend.
On Tue, 26 Jun 2018 14:28:18 +0200 David Hildenbrand <david@redhat.com> wrote: > On 26.06.2018 14:27, Cornelia Huck wrote: > > On Tue, 26 Jun 2018 14:06:00 +0200 > > David Hildenbrand <david@redhat.com> wrote: > >>>> /* Set Clock Comparator */ > >>>> void HELPER(sckc)(CPUS390XState *env, uint64_t time) > >>>> { > >>>> + S390TODState *td = s390_get_todstate(); > >>>> + S390TODClass *tdc = S390_TOD_GET_CLASS(td); > >>>> + S390TOD tod_base; > >>>> + > >>>> if (time == -1ULL) { > >>>> return; > >>>> } > >>>> > >>>> env->ckc = time; > >>>> > >>>> + tdc->get(td, &tod_base, &error_abort); > >>>> + tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); > >>> > >>> So the tdc->get first adds the time2tod, and then you subtract it here > >>> again? Can't you simply use td->base.low directly instead? > >> > >> That might be a good idea, previously I had tdc->get_base(), but dropped > >> it because it cannot be implemented for KVM. > >> > >> Simply accessing the member here should be fine. Thanks! > > > > So, we're guaranteed to have td->base.low at the correct value? (Sorry, > > having a bit of a hard time following around here.) > > > > Yes, just verified, changed and tested. We will have the value at that > place. > > I'll give some more time to review the other parts, then I can resend. Ok, thx.
diff --git a/hw/s390x/tod-qemu.c b/hw/s390x/tod-qemu.c index 03ea1ce4e8..941e84693e 100644 --- a/hw/s390x/tod-qemu.c +++ b/hw/s390x/tod-qemu.c @@ -11,19 +11,43 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "hw/s390x/tod.h" +#include "qemu/timer.h" +#include "qemu/cutils.h" +#include "cpu.h" +#include "tcg_s390x.h" static void qemu_s390_tod_get(const S390TODState *td, S390TOD *tod, Error **errp) { - /* FIXME */ - tod->high = 0; - tod->low = 0; + *tod = td->base; + + tod->low += time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); + if (tod->low < td->base.low) { + tod->high++; + } } static void qemu_s390_tod_set(S390TODState *td, const 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) @@ -34,10 +58,24 @@ static void qemu_s390_tod_class_init(ObjectClass *oc, void *data) tdc->set = qemu_s390_tod_set; } +static void qemu_s390_tod_init(Object *obj) +{ + 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 0501affa7f..1c63f411e6 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_todstate(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 7096b574ae..413c0d7c02 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, const 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 + (((ns & 0xff10000000000000ull) / 125) << 9); +} + +/* Converts s390's clock format to ns */ +static inline uint64_t tod2time(uint64_t t) +{ + return ((t >> 9) * 125) + (((t & 0x1ff) * 125) >> 9); +} + void s390_init_tod(void); +S390TODState *s390_get_todstate(void); #endif diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index a41b3f34ab..40d6980229 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" @@ -275,9 +274,6 @@ static void s390_cpu_initfn(Object *obj) CPUState *cs = CPU(obj); S390CPU *cpu = S390_CPU(obj); CPUS390XState *env = &cpu->env; -#if !defined(CONFIG_USER_ONLY) - struct tm tm; -#endif cs->env_ptr = env; cs->halted = 1; @@ -286,9 +282,6 @@ static void s390_cpu_initfn(Object *obj) s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL); s390_cpu_model_register_props(obj); #if !defined(CONFIG_USER_ONLY) - qemu_get_timedate(&tm, 0); - env->tod_offset = TOD_UNIX_EPOCH + - (time2tod(mktimegm(&tm)) * 1000000000ULL); env->tod_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu); env->cpu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu); s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu); diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 4abfe88a3d..2c3dd2d189 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; diff --git a/target/s390x/internal.h b/target/s390x/internal.h index 6cf63340bf..f2a771e2b4 100644 --- a/target/s390x/internal.h +++ b/target/s390x/internal.h @@ -237,22 +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 + (((ns & 0xff10000000000000ull) / 125) << 9); - -} - -/* Converts s390's clock format to ns */ -static inline uint64_t tod2time(uint64_t t) -{ - return ((t >> 9) * 125) + (((t & 0x1ff) * 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 dd5273949b..be341b5295 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -28,6 +28,8 @@ #include "qemu/timer.h" #include "exec/exec-all.h" #include "exec/cpu_ldst.h" +#include "qapi/error.h" +#include "tcg_s390x.h" #if !defined(CONFIG_USER_ONLY) #include "sysemu/cpus.h" @@ -39,6 +41,7 @@ #include "hw/s390x/ioinst.h" #include "hw/s390x/s390-pci-inst.h" #include "hw/boards.h" +#include "hw/s390x/tod.h" #endif /* #define DEBUG_HELPER */ @@ -138,25 +141,32 @@ void HELPER(spx)(CPUS390XState *env, uint64_t a1) /* Store Clock */ uint64_t HELPER(stck)(CPUS390XState *env) { - uint64_t time; + S390TODState *td = s390_get_todstate(); + S390TODClass *tdc = S390_TOD_GET_CLASS(td); + S390TOD tod; - time = env->tod_offset + - time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); - - return time; + tdc->get(td, &tod, &error_abort); + return tod.low; } /* Set Clock Comparator */ void HELPER(sckc)(CPUS390XState *env, uint64_t time) { + S390TODState *td = s390_get_todstate(); + S390TODClass *tdc = S390_TOD_GET_CLASS(td); + S390TOD tod_base; + if (time == -1ULL) { return; } env->ckc = time; + tdc->get(td, &tod_base, &error_abort); + tod_base.low -= time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); + /* difference between origins */ - time -= env->tod_offset; + time -= tod_base.low; /* nanoseconds */ time = tod2time(time); @@ -164,6 +174,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) { diff --git a/target/s390x/tcg_s390x.h b/target/s390x/tcg_s390x.h new file mode 100644 index 0000000000..4e308aa0ce --- /dev/null +++ b/target/s390x/tcg_s390x.h @@ -0,0 +1,18 @@ +/* + * QEMU TCG support -- s390x specific functions. + * + * Copyright 2018 Red Hat, Inc. + * + * Authors: + * 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 TCG_S390X_H +#define TCG_S390X_H + +void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque); + +#endif /* TCG_S390X_H */
Right now, each CPU has its own TOD. Especially, the TOD will differ based on creation time of a CPU - e.g. when hotplugging a CPU the times will differ quite a lot, resulting in stall warnings in the guest. Let's use a single TOD by implementing our new TOD device. Prepare it for TOD-clock epoch extension. Most importantly, whenever we set the TOD, we have to update the CKC timer. Introduce "tcg_s390x.h" just like "kvm_s390x.h" for tcg specific function declarations that should not go into cpu.h. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/s390x/tod-qemu.c | 46 ++++++++++++++++++++++++++++++++++---- hw/s390x/tod.c | 11 +++++++++ include/hw/s390x/tod.h | 19 ++++++++++++++++ target/s390x/cpu.c | 7 ------ target/s390x/cpu.h | 1 - target/s390x/internal.h | 16 ------------- target/s390x/misc_helper.c | 30 ++++++++++++++++++++----- target/s390x/tcg_s390x.h | 18 +++++++++++++++ 8 files changed, 114 insertions(+), 34 deletions(-) create mode 100644 target/s390x/tcg_s390x.h