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 <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?
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
