[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [qemu-s390x] [PATCH v3 5/9] s390x/tcg: properly impleme
From: |
Thomas Huth |
Subject: |
Re: [Qemu-devel] [qemu-s390x] [PATCH v3 5/9] s390x/tcg: properly implement the TOD |
Date: |
Tue, 26 Jun 2018 12:34:39 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
- Re: [Qemu-devel] [qemu-s390x] [PATCH v3 2/9] s390x/kvm: pass values instead of pointers to kvm_s390_set_clock_*(), (continued)
[Qemu-devel] [PATCH v3 4/9] s390x/tcg: drop tod_basetime, David Hildenbrand, 2018/06/25
[Qemu-devel] [PATCH v3 3/9] s390x/tod: factor out TOD into separate device, David Hildenbrand, 2018/06/25
[Qemu-devel] [PATCH v3 7/9] s390x/tcg: implement SET CLOCK, David Hildenbrand, 2018/06/25
[Qemu-devel] [PATCH v3 6/9] s390x/tcg: SET CLOCK COMPARATOR can clear CKC interrupts, David Hildenbrand, 2018/06/25
[Qemu-devel] [PATCH v3 5/9] s390x/tcg: properly implement the TOD, David Hildenbrand, 2018/06/25
- Re: [Qemu-devel] [qemu-s390x] [PATCH v3 5/9] s390x/tcg: properly implement the TOD,
Thomas Huth <=
[Qemu-devel] [PATCH v3 8/9] s390x/tcg: rearm the CKC timer during migration, David Hildenbrand, 2018/06/25
[Qemu-devel] [PATCH v3 9/9] s390x/tcg: fix CPU hotplug with single-threaded TCG, David Hildenbrand, 2018/06/25