Re: [Qemu-devel] [qemu-s390x] [PATCH v3 5/9] s390x/tcg: properly implement the TOD

[Thomas Huth <thuth@redhat.com>]

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>