[Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[David Hildenbrand]

Just like on other architectures, we should stop the clock while the guest
is not running. This is already properly done for TCG. Right now, doing an
offline migration (stop, migrate, cont) can easily trigger stalls in the
guest.

Even doing a
    (hmp) stop
    ... wait 2 minutes ...
    (hmp) cont
will already trigger stalls.

So whenever the guest stops, backup the KVM TOD. When continuning to run
the guest, restore the KVM TOD.

One special case is starting a simple VM: Reading the TOD from KVM to
stop it right away until the guest is actually started means that the
time of any simple VM will already differ to the host time. We can
simply leave the TOD running and the guest won't be able to recognize
it.

For migration, we actually want to keep the TOD stopped until really
starting the guest. To be able to catch most errors, we should however
try to set the TOD in addition to simply storing it. So we can still
catch basic migration problems.

If anything goes wrong while backing up/restoring the TOD, we have to
ignore it (but print a warning). This is then basically a fallback to
old behavior (TOD remains running).

I tested this very basically with an initrd:
    1. Start a simple VM. Observed that the TOD is kept running. Old
       behavior.
    2. Ordinary live migration. Observed that the TOD is temporarily
       stopped on the destination when setting the new value and
       correctly started when finally starting the guest.
    3. Offline live migration. (stop, migrate, cont). Observed that the
       TOD will be stopped on the source with the "stop" command. On the
       destination, the TOD is temporarily stopped when setting the new
       value and correctly started when finally starting the guest via
       "cont".
    4. Simple stop/cont correctly stops/starts the TOD. (multiple stops
       or conts in a row have no effect, so works as expected)

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/tod-kvm.c     | 88 +++++++++++++++++++++++++++++++++++++++++-
 include/hw/s390x/tod.h |  7 +++-
 2 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/tod-kvm.c b/hw/s390x/tod-kvm.c
index df564ab89c..d4b12e1145 100644
--- a/hw/s390x/tod-kvm.c
+++ b/hw/s390x/tod-kvm.c
@@ -10,10 +10,11 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "sysemu/sysemu.h"
 #include "hw/s390x/tod.h"
 #include "kvm_s390x.h"
 
-static void kvm_s390_tod_get(const S390TODState *td, S390TOD *tod, Error **errp)
+static void kvm_s390_get_tod_raw(S390TOD *tod, Error **errp)
 {
     int r;
 
@@ -27,7 +28,17 @@ static void kvm_s390_tod_get(const S390TODState *td, S390TOD *tod, Error **errp)
     }
 }
 
-static void kvm_s390_tod_set(S390TODState *td, const S390TOD *tod, Error **errp)
+static void kvm_s390_tod_get(const S390TODState *td, S390TOD *tod, Error **errp)
+{
+    if (td->stopped) {
+        *tod = td->base;
+        return;
+    }
+
+    kvm_s390_get_tod_raw(tod, errp);
+}
+
+static void kvm_s390_set_tod_raw(const S390TOD *tod, Error **errp)
 {
     int r;
 
@@ -38,6 +49,58 @@ static void kvm_s390_tod_set(S390TODState *td, const S390TOD *tod, Error **errp)
     if (r) {
         error_setg(errp, "Unable to set KVM guest TOD clock: %s",
                    strerror(-r));
+        return;
+    }
+}
+
+static void kvm_s390_tod_set(S390TODState *td, const S390TOD *tod, Error **errp)
+{
+    Error *local_err = NULL;
+
+    /*
+     * Somebody (e.g. migration) set the TOD. We'll store it into KVM to
+     * properly detect errors now but take a look at the runstate to decide
+     * whether really to keep the tod running. E.g. during migration, this
+     * is the point where we want to stop the inititally running TOD to fire
+     * it back up when actually starting the migrated guest.
+     */
+    kvm_s390_set_tod_raw(tod, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (runstate_is_running()) {
+        td->stopped = false;
+    } else {
+        td->stopped = true;
+        td->base = *tod;
+    }
+}
+
+static void kvm_s390_tod_vm_state_change(void *opaque, int running,
+                                         RunState state)
+{
+    S390TODState *td = opaque;
+    Error *local_err = NULL;
+
+    if (running && td->stopped) {
+        /* set the old TOD when running the VM - start the TOD clock */
+        kvm_s390_set_tod_raw(&td->base, &local_err);
+        if (local_err) {
+            warn_report_err(local_err);
+        }
+        /* Treat errors like the TOD was running all the time. */
+        td->stopped = false;
+    } else if (!running && !td->stopped) {
+        /* store the TOD when stopping the VM - stop the TOD clock */
+        kvm_s390_get_tod_raw(&td->base, &local_err);
+        if (local_err) {
+            /* keep the TOD running in case we could not back it up */
+            warn_report_err(local_err);
+        } else {
+            td->stopped = true;
+        }
     }
 }
 
@@ -49,10 +112,31 @@ static void kvm_s390_tod_class_init(ObjectClass *oc, void *data)
     tdc->set = kvm_s390_tod_set;
 }
 
+static void kvm_s390_tod_init(Object *obj)
+{
+    S390TODState *td = S390_TOD(obj);
+
+    /*
+     * The TOD is initially running (value stored in KVM). Avoid needless
+     * loading/storing of the TOD when starting a simple VM, so let it
+     * run although the (never started) VM is stopped. For migration, we
+     * will properly set the TOD later.
+     */
+    td->stopped = false;
+
+    /*
+     * We need to know when the VM gets started/stopped to start/stop the TOD.
+     * As we can never have more than one TOD instance (and that will never be
+     * removed), registering here and never unregistering is good enough.
+     */
+    qemu_add_vm_change_state_handler(kvm_s390_tod_vm_state_change, td);
+}
+
 static TypeInfo kvm_s390_tod_info = {
     .name = TYPE_KVM_S390_TOD,
     .parent = TYPE_S390_TOD,
     .instance_size = sizeof(S390TODState),
+    .instance_init = kvm_s390_tod_init,
     .class_init = kvm_s390_tod_class_init,
     .class_size = sizeof(S390TODClass),
 };
diff --git a/include/hw/s390x/tod.h b/include/hw/s390x/tod.h
index 413c0d7c02..30558d4386 100644
--- a/include/hw/s390x/tod.h
+++ b/include/hw/s390x/tod.h
@@ -31,8 +31,13 @@ typedef struct S390TODState {
     /* private */
     DeviceState parent_obj;
 
-    /* unused by KVM implementation */
+    /*
+     * Used by TCG remember the time base. Used by KVM to backup the TOD
+     * while the TOD is stopped.
+     */
     S390TOD base;
+    /* Used by KVM to remember if the TOD is stopped and base is valid. */
+    bool stopped;
 } S390TODState;
 
 typedef struct S390TODClass {
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[Thomas Huth]

On 2018-11-27 12:41, David Hildenbrand wrote:
> Just like on other architectures, we should stop the clock while the guest
> is not running. This is already properly done for TCG. Right now, doing an
> offline migration (stop, migrate, cont) can easily trigger stalls in the
> guest.
> 
> Even doing a
>     (hmp) stop
>     ... wait 2 minutes ...
>     (hmp) cont
> will already trigger stalls.
> 
> So whenever the guest stops, backup the KVM TOD. When continuning to run
> the guest, restore the KVM TOD.
> 
> One special case is starting a simple VM: Reading the TOD from KVM to
> stop it right away until the guest is actually started means that the
> time of any simple VM will already differ to the host time. We can
> simply leave the TOD running and the guest won't be able to recognize
> it.
> 
> For migration, we actually want to keep the TOD stopped until really
> starting the guest. To be able to catch most errors, we should however
> try to set the TOD in addition to simply storing it. So we can still
> catch basic migration problems.
> 
> If anything goes wrong while backing up/restoring the TOD, we have to
> ignore it (but print a warning). This is then basically a fallback to
> old behavior (TOD remains running).
> 
> I tested this very basically with an initrd:
>     1. Start a simple VM. Observed that the TOD is kept running. Old
>        behavior.
>     2. Ordinary live migration. Observed that the TOD is temporarily
>        stopped on the destination when setting the new value and
>        correctly started when finally starting the guest.
>     3. Offline live migration. (stop, migrate, cont). Observed that the
>        TOD will be stopped on the source with the "stop" command. On the
>        destination, the TOD is temporarily stopped when setting the new
>        value and correctly started when finally starting the guest via
>        "cont".
>     4. Simple stop/cont correctly stops/starts the TOD. (multiple stops
>        or conts in a row have no effect, so works as expected)
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/tod-kvm.c     | 88 +++++++++++++++++++++++++++++++++++++++++-
>  include/hw/s390x/tod.h |  7 +++-
>  2 files changed, 92 insertions(+), 3 deletions(-)
[...]
> @@ -49,10 +112,31 @@ static void kvm_s390_tod_class_init(ObjectClass *oc, void *data)
>      tdc->set = kvm_s390_tod_set;
>  }
>  
> +static void kvm_s390_tod_init(Object *obj)
> +{
> +    S390TODState *td = S390_TOD(obj);
> +
> +    /*
> +     * The TOD is initially running (value stored in KVM). Avoid needless
> +     * loading/storing of the TOD when starting a simple VM, so let it
> +     * run although the (never started) VM is stopped. For migration, we
> +     * will properly set the TOD later.
> +     */
> +    td->stopped = false;
> +
> +    /*
> +     * We need to know when the VM gets started/stopped to start/stop the TOD.
> +     * As we can never have more than one TOD instance (and that will never be
> +     * removed), registering here and never unregistering is good enough.
> +     */
> +    qemu_add_vm_change_state_handler(kvm_s390_tod_vm_state_change, td);

I think you should rather do this in a realize() function instead, since
instance_init can be called multiple times (during introspection of the
object, for example). See my blog article here for some details:

https://people.redhat.com/~thuth/blog/qemu/2018/09/10/instance-init-realize.html

> +}
> +
>  static TypeInfo kvm_s390_tod_info = {
>      .name = TYPE_KVM_S390_TOD,
>      .parent = TYPE_S390_TOD,
>      .instance_size = sizeof(S390TODState),
> +    .instance_init = kvm_s390_tod_init,
>      .class_init = kvm_s390_tod_class_init,
>      .class_size = sizeof(S390TODClass),
>  };

 Thomas

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[Cornelia Huck]

On Tue, 27 Nov 2018 12:41:02 +0100
David Hildenbrand <david@redhat.com> wrote:

> Just like on other architectures, we should stop the clock while the guest
> is not running. This is already properly done for TCG. Right now, doing an
> offline migration (stop, migrate, cont) can easily trigger stalls in the
> guest.
> 
> Even doing a
>     (hmp) stop
>     ... wait 2 minutes ...
>     (hmp) cont
> will already trigger stalls.
> 
> So whenever the guest stops, backup the KVM TOD. When continuning to run
> the guest, restore the KVM TOD.
> 
> One special case is starting a simple VM: Reading the TOD from KVM to
> stop it right away until the guest is actually started means that the
> time of any simple VM will already differ to the host time. We can
> simply leave the TOD running and the guest won't be able to recognize
> it.
> 
> For migration, we actually want to keep the TOD stopped until really
> starting the guest. To be able to catch most errors, we should however
> try to set the TOD in addition to simply storing it. So we can still
> catch basic migration problems.
> 
> If anything goes wrong while backing up/restoring the TOD, we have to
> ignore it (but print a warning). This is then basically a fallback to
> old behavior (TOD remains running).
> 
> I tested this very basically with an initrd:
>     1. Start a simple VM. Observed that the TOD is kept running. Old
>        behavior.
>     2. Ordinary live migration. Observed that the TOD is temporarily
>        stopped on the destination when setting the new value and
>        correctly started when finally starting the guest.
>     3. Offline live migration. (stop, migrate, cont). Observed that the
>        TOD will be stopped on the source with the "stop" command. On the
>        destination, the TOD is temporarily stopped when setting the new
>        value and correctly started when finally starting the guest via
>        "cont".
>     4. Simple stop/cont correctly stops/starts the TOD. (multiple stops
>        or conts in a row have no effect, so works as expected)
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/tod-kvm.c     | 88 +++++++++++++++++++++++++++++++++++++++++-
>  include/hw/s390x/tod.h |  7 +++-
>  2 files changed, 92 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/tod-kvm.c b/hw/s390x/tod-kvm.c
> index df564ab89c..d4b12e1145 100644
> --- a/hw/s390x/tod-kvm.c
> +++ b/hw/s390x/tod-kvm.c

> @@ -38,6 +49,58 @@ static void kvm_s390_tod_set(S390TODState *td, const S390TOD *tod, Error **errp)
>      if (r) {
>          error_setg(errp, "Unable to set KVM guest TOD clock: %s",
>                     strerror(-r));
> +        return;

I don't think you need this new 'return'.

> +    }
> +}

(...)

> +static void kvm_s390_tod_init(Object *obj)
> +{
> +    S390TODState *td = S390_TOD(obj);
> +
> +    /*
> +     * The TOD is initially running (value stored in KVM). Avoid needless
> +     * loading/storing of the TOD when starting a simple VM, so let it
> +     * run although the (never started) VM is stopped. For migration, we
> +     * will properly set the TOD later.
> +     */
> +    td->stopped = false;
> +
> +    /*
> +     * We need to know when the VM gets started/stopped to start/stop the TOD.
> +     * As we can never have more than one TOD instance (and that will never be
> +     * removed), registering here and never unregistering is good enough.
> +     */
> +    qemu_add_vm_change_state_handler(kvm_s390_tod_vm_state_change, td);
> +}
> +
>  static TypeInfo kvm_s390_tod_info = {
>      .name = TYPE_KVM_S390_TOD,
>      .parent = TYPE_S390_TOD,
>      .instance_size = sizeof(S390TODState),
> +    .instance_init = kvm_s390_tod_init,

See Thomas' comment wrt realize vs. instance_init.

>      .class_init = kvm_s390_tod_class_init,
>      .class_size = sizeof(S390TODClass),
>  };

This one is a bit late for 3.1, but should it be cc:stable?

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[Christian Borntraeger]

On 27.11.2018 12:41, David Hildenbrand wrote:
> Just like on other architectures, we should stop the clock while the guest
> is not running. This is already properly done for TCG. Right now, doing an
> offline migration (stop, migrate, cont) can easily trigger stalls in the
> guest.
> 
> Even doing a
>     (hmp) stop
>     ... wait 2 minutes ...
>     (hmp) cont
> will already trigger stalls.
> 
> So whenever the guest stops, backup the KVM TOD. When continuning to run
> the guest, restore the KVM TOD.

We do a similar thing for managedsave so it probably makes sense to solve
the stall warnings. Now: At the same time, we actually want to have the guest
see the real time and maybe even share the TOD clock with the host in some
way, while at the same time avoid the stall warnings.

One idea is to signal the guest on migration (and stop start events) and let
the guest do a time sync with the host.
In fact, it seems that by architecture we need to signal a special sclp signal 
anyway if stsi information would change (e.g. on migration). Maybe we can 
piggyback on that and do some time sync interface in the future.
> 
> One special case is starting a simple VM: Reading the TOD from KVM to
> stop it right away until the guest is actually started means that the
> time of any simple VM will already differ to the host time. We can
> simply leave the TOD running and the guest won't be able to recognize
> it.
> 
> For migration, we actually want to keep the TOD stopped until really
> starting the guest. To be able to catch most errors, we should however
> try to set the TOD in addition to simply storing it. So we can still
> catch basic migration problems.
> 
> If anything goes wrong while backing up/restoring the TOD, we have to
> ignore it (but print a warning). This is then basically a fallback to
> old behavior (TOD remains running).
> 
> I tested this very basically with an initrd:
>     1. Start a simple VM. Observed that the TOD is kept running. Old
>        behavior.
>     2. Ordinary live migration. Observed that the TOD is temporarily
>        stopped on the destination when setting the new value and
>        correctly started when finally starting the guest.
>     3. Offline live migration. (stop, migrate, cont). Observed that the
>        TOD will be stopped on the source with the "stop" command. On the
>        destination, the TOD is temporarily stopped when setting the new
>        value and correctly started when finally starting the guest via
>        "cont".
>     4. Simple stop/cont correctly stops/starts the TOD. (multiple stops
>        or conts in a row have no effect, so works as expected)
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/tod-kvm.c     | 88 +++++++++++++++++++++++++++++++++++++++++-
>  include/hw/s390x/tod.h |  7 +++-
>  2 files changed, 92 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/tod-kvm.c b/hw/s390x/tod-kvm.c
> index df564ab89c..d4b12e1145 100644
> --- a/hw/s390x/tod-kvm.c
> +++ b/hw/s390x/tod-kvm.c
> @@ -10,10 +10,11 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "sysemu/sysemu.h"
>  #include "hw/s390x/tod.h"
>  #include "kvm_s390x.h"
>  
> -static void kvm_s390_tod_get(const S390TODState *td, S390TOD *tod, Error **errp)
> +static void kvm_s390_get_tod_raw(S390TOD *tod, Error **errp)
>  {
>      int r;
>  
> @@ -27,7 +28,17 @@ static void kvm_s390_tod_get(const S390TODState *td, S390TOD *tod, Error **errp)
>      }
>  }
>  
> -static void kvm_s390_tod_set(S390TODState *td, const S390TOD *tod, Error **errp)
> +static void kvm_s390_tod_get(const S390TODState *td, S390TOD *tod, Error **errp)
> +{
> +    if (td->stopped) {
> +        *tod = td->base;
> +        return;
> +    }
> +
> +    kvm_s390_get_tod_raw(tod, errp);
> +}
> +
> +static void kvm_s390_set_tod_raw(const S390TOD *tod, Error **errp)
>  {
>      int r;
>  
> @@ -38,6 +49,58 @@ static void kvm_s390_tod_set(S390TODState *td, const S390TOD *tod, Error **errp)
>      if (r) {
>          error_setg(errp, "Unable to set KVM guest TOD clock: %s",
>                     strerror(-r));
> +        return;
> +    }
> +}
> +
> +static void kvm_s390_tod_set(S390TODState *td, const S390TOD *tod, Error **errp)
> +{
> +    Error *local_err = NULL;
> +
> +    /*
> +     * Somebody (e.g. migration) set the TOD. We'll store it into KVM to
> +     * properly detect errors now but take a look at the runstate to decide
> +     * whether really to keep the tod running. E.g. during migration, this
> +     * is the point where we want to stop the inititally running TOD to fire
> +     * it back up when actually starting the migrated guest.
> +     */
> +    kvm_s390_set_tod_raw(tod, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (runstate_is_running()) {
> +        td->stopped = false;
> +    } else {
> +        td->stopped = true;
> +        td->base = *tod;
> +    }
> +}
> +
> +static void kvm_s390_tod_vm_state_change(void *opaque, int running,
> +                                         RunState state)
> +{
> +    S390TODState *td = opaque;
> +    Error *local_err = NULL;
> +
> +    if (running && td->stopped) {
> +        /* set the old TOD when running the VM - start the TOD clock */
> +        kvm_s390_set_tod_raw(&td->base, &local_err);
> +        if (local_err) {
> +            warn_report_err(local_err);
> +        }
> +        /* Treat errors like the TOD was running all the time. */
> +        td->stopped = false;
> +    } else if (!running && !td->stopped) {
> +        /* store the TOD when stopping the VM - stop the TOD clock */
> +        kvm_s390_get_tod_raw(&td->base, &local_err);
> +        if (local_err) {
> +            /* keep the TOD running in case we could not back it up */
> +            warn_report_err(local_err);
> +        } else {
> +            td->stopped = true;
> +        }
>      }
>  }
>  
> @@ -49,10 +112,31 @@ static void kvm_s390_tod_class_init(ObjectClass *oc, void *data)
>      tdc->set = kvm_s390_tod_set;
>  }
>  
> +static void kvm_s390_tod_init(Object *obj)
> +{
> +    S390TODState *td = S390_TOD(obj);
> +
> +    /*
> +     * The TOD is initially running (value stored in KVM). Avoid needless
> +     * loading/storing of the TOD when starting a simple VM, so let it
> +     * run although the (never started) VM is stopped. For migration, we
> +     * will properly set the TOD later.
> +     */
> +    td->stopped = false;
> +
> +    /*
> +     * We need to know when the VM gets started/stopped to start/stop the TOD.
> +     * As we can never have more than one TOD instance (and that will never be
> +     * removed), registering here and never unregistering is good enough.
> +     */
> +    qemu_add_vm_change_state_handler(kvm_s390_tod_vm_state_change, td);
> +}
> +
>  static TypeInfo kvm_s390_tod_info = {
>      .name = TYPE_KVM_S390_TOD,
>      .parent = TYPE_S390_TOD,
>      .instance_size = sizeof(S390TODState),
> +    .instance_init = kvm_s390_tod_init,
>      .class_init = kvm_s390_tod_class_init,
>      .class_size = sizeof(S390TODClass),
>  };
> diff --git a/include/hw/s390x/tod.h b/include/hw/s390x/tod.h
> index 413c0d7c02..30558d4386 100644
> --- a/include/hw/s390x/tod.h
> +++ b/include/hw/s390x/tod.h
> @@ -31,8 +31,13 @@ typedef struct S390TODState {
>      /* private */
>      DeviceState parent_obj;
>  
> -    /* unused by KVM implementation */
> +    /*
> +     * Used by TCG remember the time base. Used by KVM to backup the TOD
> +     * while the TOD is stopped.
> +     */
>      S390TOD base;
> +    /* Used by KVM to remember if the TOD is stopped and base is valid. */
> +    bool stopped;
>  } S390TODState;
>  
>  typedef struct S390TODClass {
> 

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[Cornelia Huck]

On Tue, 27 Nov 2018 13:43:24 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 27.11.2018 12:41, David Hildenbrand wrote:
> > Just like on other architectures, we should stop the clock while the guest
> > is not running. This is already properly done for TCG. Right now, doing an
> > offline migration (stop, migrate, cont) can easily trigger stalls in the
> > guest.
> > 
> > Even doing a
> >     (hmp) stop
> >     ... wait 2 minutes ...
> >     (hmp) cont
> > will already trigger stalls.
> > 
> > So whenever the guest stops, backup the KVM TOD. When continuning to run
> > the guest, restore the KVM TOD.  
> 
> We do a similar thing for managedsave so it probably makes sense to solve
> the stall warnings. Now: At the same time, we actually want to have the guest
> see the real time and maybe even share the TOD clock with the host in some
> way, while at the same time avoid the stall warnings.
> 
> One idea is to signal the guest on migration (and stop start events) and let
> the guest do a time sync with the host.
> In fact, it seems that by architecture we need to signal a special sclp signal 
> anyway if stsi information would change (e.g. on migration). Maybe we can 
> piggyback on that and do some time sync interface in the future.

That sounds interesting.

Out of curiosity, is z/VM already doing something like that when you
migrate Linux guests? Or do they do it differently enough from what
QEMU is doing anyway?

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[David Hildenbrand]

On 27.11.18 13:19, Thomas Huth wrote:
> On 2018-11-27 12:41, David Hildenbrand wrote:
>> Just like on other architectures, we should stop the clock while the guest
>> is not running. This is already properly done for TCG. Right now, doing an
>> offline migration (stop, migrate, cont) can easily trigger stalls in the
>> guest.
>>
>> Even doing a
>>     (hmp) stop
>>     ... wait 2 minutes ...
>>     (hmp) cont
>> will already trigger stalls.
>>
>> So whenever the guest stops, backup the KVM TOD. When continuning to run
>> the guest, restore the KVM TOD.
>>
>> One special case is starting a simple VM: Reading the TOD from KVM to
>> stop it right away until the guest is actually started means that the
>> time of any simple VM will already differ to the host time. We can
>> simply leave the TOD running and the guest won't be able to recognize
>> it.
>>
>> For migration, we actually want to keep the TOD stopped until really
>> starting the guest. To be able to catch most errors, we should however
>> try to set the TOD in addition to simply storing it. So we can still
>> catch basic migration problems.
>>
>> If anything goes wrong while backing up/restoring the TOD, we have to
>> ignore it (but print a warning). This is then basically a fallback to
>> old behavior (TOD remains running).
>>
>> I tested this very basically with an initrd:
>>     1. Start a simple VM. Observed that the TOD is kept running. Old
>>        behavior.
>>     2. Ordinary live migration. Observed that the TOD is temporarily
>>        stopped on the destination when setting the new value and
>>        correctly started when finally starting the guest.
>>     3. Offline live migration. (stop, migrate, cont). Observed that the
>>        TOD will be stopped on the source with the "stop" command. On the
>>        destination, the TOD is temporarily stopped when setting the new
>>        value and correctly started when finally starting the guest via
>>        "cont".
>>     4. Simple stop/cont correctly stops/starts the TOD. (multiple stops
>>        or conts in a row have no effect, so works as expected)
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/tod-kvm.c     | 88 +++++++++++++++++++++++++++++++++++++++++-
>>  include/hw/s390x/tod.h |  7 +++-
>>  2 files changed, 92 insertions(+), 3 deletions(-)
> [...]
>> @@ -49,10 +112,31 @@ static void kvm_s390_tod_class_init(ObjectClass *oc, void *data)
>>      tdc->set = kvm_s390_tod_set;
>>  }
>>  
>> +static void kvm_s390_tod_init(Object *obj)
>> +{
>> +    S390TODState *td = S390_TOD(obj);
>> +
>> +    /*
>> +     * The TOD is initially running (value stored in KVM). Avoid needless
>> +     * loading/storing of the TOD when starting a simple VM, so let it
>> +     * run although the (never started) VM is stopped. For migration, we
>> +     * will properly set the TOD later.
>> +     */
>> +    td->stopped = false;
>> +
>> +    /*
>> +     * We need to know when the VM gets started/stopped to start/stop the TOD.
>> +     * As we can never have more than one TOD instance (and that will never be
>> +     * removed), registering here and never unregistering is good enough.
>> +     */
>> +    qemu_add_vm_change_state_handler(kvm_s390_tod_vm_state_change, td);
> 
> I think you should rather do this in a realize() function instead, since
> instance_init can be called multiple times (during introspection of the
> object, for example). See my blog article here for some details:
> 
> https://people.redhat.com/~thuth/blog/qemu/2018/09/10/instance-init-realize.html


Even for types that cannot be created by the user?

Anyhow, realize is the right thing to do, I just wanted to avoid
introducing a realize callback for tod clocks.

Thanks!

> 
>> +}
>> +
>>  static TypeInfo kvm_s390_tod_info = {
>>      .name = TYPE_KVM_S390_TOD,
>>      .parent = TYPE_S390_TOD,
>>      .instance_size = sizeof(S390TODState),
>> +    .instance_init = kvm_s390_tod_init,
>>      .class_init = kvm_s390_tod_class_init,
>>      .class_size = sizeof(S390TODClass),
>>  };
> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[David Hildenbrand]

On 27.11.18 13:38, Cornelia Huck wrote:
> On Tue, 27 Nov 2018 12:41:02 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Just like on other architectures, we should stop the clock while the guest
>> is not running. This is already properly done for TCG. Right now, doing an
>> offline migration (stop, migrate, cont) can easily trigger stalls in the
>> guest.
>>
>> Even doing a
>>     (hmp) stop
>>     ... wait 2 minutes ...
>>     (hmp) cont
>> will already trigger stalls.
>>
>> So whenever the guest stops, backup the KVM TOD. When continuning to run
>> the guest, restore the KVM TOD.
>>
>> One special case is starting a simple VM: Reading the TOD from KVM to
>> stop it right away until the guest is actually started means that the
>> time of any simple VM will already differ to the host time. We can
>> simply leave the TOD running and the guest won't be able to recognize
>> it.
>>
>> For migration, we actually want to keep the TOD stopped until really
>> starting the guest. To be able to catch most errors, we should however
>> try to set the TOD in addition to simply storing it. So we can still
>> catch basic migration problems.
>>
>> If anything goes wrong while backing up/restoring the TOD, we have to
>> ignore it (but print a warning). This is then basically a fallback to
>> old behavior (TOD remains running).
>>
>> I tested this very basically with an initrd:
>>     1. Start a simple VM. Observed that the TOD is kept running. Old
>>        behavior.
>>     2. Ordinary live migration. Observed that the TOD is temporarily
>>        stopped on the destination when setting the new value and
>>        correctly started when finally starting the guest.
>>     3. Offline live migration. (stop, migrate, cont). Observed that the
>>        TOD will be stopped on the source with the "stop" command. On the
>>        destination, the TOD is temporarily stopped when setting the new
>>        value and correctly started when finally starting the guest via
>>        "cont".
>>     4. Simple stop/cont correctly stops/starts the TOD. (multiple stops
>>        or conts in a row have no effect, so works as expected)
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/tod-kvm.c     | 88 +++++++++++++++++++++++++++++++++++++++++-
>>  include/hw/s390x/tod.h |  7 +++-
>>  2 files changed, 92 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/tod-kvm.c b/hw/s390x/tod-kvm.c
>> index df564ab89c..d4b12e1145 100644
>> --- a/hw/s390x/tod-kvm.c
>> +++ b/hw/s390x/tod-kvm.c
> 
>> @@ -38,6 +49,58 @@ static void kvm_s390_tod_set(S390TODState *td, const S390TOD *tod, Error **errp)
>>      if (r) {
>>          error_setg(errp, "Unable to set KVM guest TOD clock: %s",
>>                     strerror(-r));
>> +        return;
> 
> I don't think you need this new 'return'.

Yes, very right!

Thanks!


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[David Hildenbrand]

On 27.11.18 13:43, Christian Borntraeger wrote:
> On 27.11.2018 12:41, David Hildenbrand wrote:
>> Just like on other architectures, we should stop the clock while the guest
>> is not running. This is already properly done for TCG. Right now, doing an
>> offline migration (stop, migrate, cont) can easily trigger stalls in the
>> guest.
>>
>> Even doing a
>>     (hmp) stop
>>     ... wait 2 minutes ...
>>     (hmp) cont
>> will already trigger stalls.
>>
>> So whenever the guest stops, backup the KVM TOD. When continuning to run
>> the guest, restore the KVM TOD.
> 
> We do a similar thing for managedsave so it probably makes sense to solve
> the stall warnings. Now: At the same time, we actually want to have the guest
> see the real time and maybe even share the TOD clock with the host in some
> way, while at the same time avoid the stall warnings.

managedsave works right now because we save/restore the TOD. Making the
guest TOD jump is never a good idea. At least not blindly (as you
mention below, special SCLP signals eventually).

> 
> One idea is to signal the guest on migration (and stop start events) and let
> the guest do a time sync with the host.

Doing so on stop events will most likely kill the ability to debug (e.g.
single step) your guest. You will trap into the SCLP signal handler in
the guest all the time. I don't think this is a good idea for the
general case. It might be for migration. We might instead want to handle
this in KVM directly and not in QEMU.

> In fact, it seems that by architecture we need to signal a special sclp signal 
> anyway if stsi information would change (e.g. on migration). Maybe we can 
> piggyback on that and do some time sync interface in the future.

I like the idea but I rather consider it an addon feature and that
shouldn't be the default.

As SCLP documentation is (as far as I know, please correct me if I am
wrong) not publicly available, I suggest going ahead with what I propose
here (what other architectures do) and implementing what you describe on
top later.

Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[Thomas Huth]

On 2018-11-27 13:50, David Hildenbrand wrote:
> On 27.11.18 13:19, Thomas Huth wrote:
>> On 2018-11-27 12:41, David Hildenbrand wrote:
>>> Just like on other architectures, we should stop the clock while the guest
>>> is not running. This is already properly done for TCG. Right now, doing an
>>> offline migration (stop, migrate, cont) can easily trigger stalls in the
>>> guest.
>>>
>>> Even doing a
>>>     (hmp) stop
>>>     ... wait 2 minutes ...
>>>     (hmp) cont
>>> will already trigger stalls.
>>>
>>> So whenever the guest stops, backup the KVM TOD. When continuning to run
>>> the guest, restore the KVM TOD.
>>>
>>> One special case is starting a simple VM: Reading the TOD from KVM to
>>> stop it right away until the guest is actually started means that the
>>> time of any simple VM will already differ to the host time. We can
>>> simply leave the TOD running and the guest won't be able to recognize
>>> it.
>>>
>>> For migration, we actually want to keep the TOD stopped until really
>>> starting the guest. To be able to catch most errors, we should however
>>> try to set the TOD in addition to simply storing it. So we can still
>>> catch basic migration problems.
>>>
>>> If anything goes wrong while backing up/restoring the TOD, we have to
>>> ignore it (but print a warning). This is then basically a fallback to
>>> old behavior (TOD remains running).
>>>
>>> I tested this very basically with an initrd:
>>>     1. Start a simple VM. Observed that the TOD is kept running. Old
>>>        behavior.
>>>     2. Ordinary live migration. Observed that the TOD is temporarily
>>>        stopped on the destination when setting the new value and
>>>        correctly started when finally starting the guest.
>>>     3. Offline live migration. (stop, migrate, cont). Observed that the
>>>        TOD will be stopped on the source with the "stop" command. On the
>>>        destination, the TOD is temporarily stopped when setting the new
>>>        value and correctly started when finally starting the guest via
>>>        "cont".
>>>     4. Simple stop/cont correctly stops/starts the TOD. (multiple stops
>>>        or conts in a row have no effect, so works as expected)
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  hw/s390x/tod-kvm.c     | 88 +++++++++++++++++++++++++++++++++++++++++-
>>>  include/hw/s390x/tod.h |  7 +++-
>>>  2 files changed, 92 insertions(+), 3 deletions(-)
>> [...]
>>> @@ -49,10 +112,31 @@ static void kvm_s390_tod_class_init(ObjectClass *oc, void *data)
>>>      tdc->set = kvm_s390_tod_set;
>>>  }
>>>  
>>> +static void kvm_s390_tod_init(Object *obj)
>>> +{
>>> +    S390TODState *td = S390_TOD(obj);
>>> +
>>> +    /*
>>> +     * The TOD is initially running (value stored in KVM). Avoid needless
>>> +     * loading/storing of the TOD when starting a simple VM, so let it
>>> +     * run although the (never started) VM is stopped. For migration, we
>>> +     * will properly set the TOD later.
>>> +     */
>>> +    td->stopped = false;
>>> +
>>> +    /*
>>> +     * We need to know when the VM gets started/stopped to start/stop the TOD.
>>> +     * As we can never have more than one TOD instance (and that will never be
>>> +     * removed), registering here and never unregistering is good enough.
>>> +     */
>>> +    qemu_add_vm_change_state_handler(kvm_s390_tod_vm_state_change, td);
>>
>> I think you should rather do this in a realize() function instead, since
>> instance_init can be called multiple times (during introspection of the
>> object, for example). See my blog article here for some details:
>>
>> https://people.redhat.com/~thuth/blog/qemu/2018/09/10/instance-init-realize.html
> 
> 
> Even for types that cannot be created by the user?

Yes, you still can run "device_add s390-tod-kvm,help" at the HMP prompt,
or introspect the device via QMP. Both will trigger a temporary instance
of the device, where instance_init is executed.

 Thomas

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[Thomas Huth]

On 2018-11-27 13:43, Christian Borntraeger wrote:
> On 27.11.2018 12:41, David Hildenbrand wrote:
>> Just like on other architectures, we should stop the clock while the guest
>> is not running. This is already properly done for TCG. Right now, doing an
>> offline migration (stop, migrate, cont) can easily trigger stalls in the
>> guest.
>>
>> Even doing a
>>     (hmp) stop
>>     ... wait 2 minutes ...
>>     (hmp) cont
>> will already trigger stalls.
>>
>> So whenever the guest stops, backup the KVM TOD. When continuning to run
>> the guest, restore the KVM TOD.
> 
> We do a similar thing for managedsave so it probably makes sense to solve
> the stall warnings. Now: At the same time, we actually want to have the guest
> see the real time and maybe even share the TOD clock with the host in some
> way, while at the same time avoid the stall warnings.

Hmm, by the way, do we also have to consider the "-rtc clock=host|rt|vm"
option from the commandl line, or is this an orthogonal concept?

 Thomas

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[Christian Borntraeger]

On 27.11.2018 14:06, Thomas Huth wrote:
> On 2018-11-27 13:43, Christian Borntraeger wrote:
>> On 27.11.2018 12:41, David Hildenbrand wrote:
>>> Just like on other architectures, we should stop the clock while the guest
>>> is not running. This is already properly done for TCG. Right now, doing an
>>> offline migration (stop, migrate, cont) can easily trigger stalls in the
>>> guest.
>>>
>>> Even doing a
>>>     (hmp) stop
>>>     ... wait 2 minutes ...
>>>     (hmp) cont
>>> will already trigger stalls.
>>>
>>> So whenever the guest stops, backup the KVM TOD. When continuning to run
>>> the guest, restore the KVM TOD.
>>
>> We do a similar thing for managedsave so it probably makes sense to solve
>> the stall warnings. Now: At the same time, we actually want to have the guest
>> see the real time and maybe even share the TOD clock with the host in some
>> way, while at the same time avoid the stall warnings.
> 
> Hmm, by the way, do we also have to consider the "-rtc clock=host|rt|vm"
> option from the commandl line, or is this an orthogonal concept?

I think this option does not work at all in how time works on s390.

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[David Hildenbrand]

On 27.11.18 14:06, Thomas Huth wrote:
> On 2018-11-27 13:43, Christian Borntraeger wrote:
>> On 27.11.2018 12:41, David Hildenbrand wrote:
>>> Just like on other architectures, we should stop the clock while the guest
>>> is not running. This is already properly done for TCG. Right now, doing an
>>> offline migration (stop, migrate, cont) can easily trigger stalls in the
>>> guest.
>>>
>>> Even doing a
>>>     (hmp) stop
>>>     ... wait 2 minutes ...
>>>     (hmp) cont
>>> will already trigger stalls.
>>>
>>> So whenever the guest stops, backup the KVM TOD. When continuning to run
>>> the guest, restore the KVM TOD.
>>
>> We do a similar thing for managedsave so it probably makes sense to solve
>> the stall warnings. Now: At the same time, we actually want to have the guest
>> see the real time and maybe even share the TOD clock with the host in some
>> way, while at the same time avoid the stall warnings.
> 
> Hmm, by the way, do we also have to consider the "-rtc clock=host|rt|vm"
> option from the commandl line, or is this an orthogonal concept?
> 
>  Thomas
> 

-rtc [base=utc|localtime|date][,clock=host|vm][,driftfix=none|slew]
       Specify base as "utc" or "localtime" to let the RTC start at the current UTC or local time, respectively. "localtime" is required for
       correct date in MS-DOS or Windows. To start at a specific point in time, provide date in the format "2006-06-17T16:01:21" or
       "2006-06-17". The default base is UTC.

       By default the RTC is driven by the host system time. This allows using of the RTC as accurate reference clock inside the guest,
       specifically if the host time is smoothly following an accurate external reference clock, e.g. via NTP.  If you want to isolate the
       guest time from the host, you can set clock to "rt" instead.  To even prevent it from progressing during suspension, you can set it to
       "vm".

Most of the stuff here is not supported (should we error out if specified?)
and I think this is only considered for the RTC clock on x86, not "ordinary" clocks
(e.g. kvmclock)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1] s390x/tod: properly stop the KVM TOD while the guest is not running

[David Hildenbrand]

> This one is a bit late for 3.1, but should it be cc:stable?
> 

Missed this question. Good point, not sure if this is worth cc:stable.

-- 

Thanks,

David / dhildenb