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 <da...@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
