Re: [Qemu-devel] [PATCH v2 2/2] s390/kvm: make TOD setting failures fatal for migration

[Christian Borntraeger]

On 10/04/2017 01:48 PM, Thomas Huth wrote:
> On 04.10.2017 12:57, Christian Borntraeger wrote:
>> From: "Collin L. Walling" <address@hidden>
>>
>> If we fail to set a proper TOD clock on the target system,  this can
>> already result in some problematic cases. We print several warn messages
>> on source and target in that case.
>>
>> If kvm fails to set a nonzero epoch index, then we must ultimately fail
>> the migration as this will result in a giant time leap backwards. This
>> patch lets the migration fail if we can not set the guest time on the
>> target.
>>
>> On failure the guest will resume normally on the original host machine.
>>
>> Signed-off-by: Collin L. Walling <address@hidden>
>> Reviewed-by: Eric Farman <address@hidden>
>> Reviewed-by: Claudio Imbrenda <address@hidden>
>> Signed-off-by: Christian Borntraeger <address@hidden>
>> [split failure change from epoch index change, minor fixups]
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index fafbc6d..b7adb93 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -213,13 +213,10 @@ static int gtod_load(QEMUFile *f, void *opaque, int 
>> version_id)
>>  
>>      r = s390_set_clock(&tod_high, &tod_low);
>>      if (r) {
>> -        warn_report("Unable to set guest clock for migration: %s",
>> -                    strerror(-r));
>> -        error_printf("Guest clock will not be restored "
>> -                     "which could cause the guest to hang.");
>> +        error_report("Unable to set KVM guest TOD clock: %s", strerror(-r));
>>      }
>>  
>> -    return 0;
>> +    return r;
>>  }
> 
> Reviewed-by: Thomas Huth <address@hidden>
> 
> (and I wonder whether we should fail the S390_TOD_CLOCK_VALUE_MISSING
> case with returning an error instead of 0, too?)

Yes. This patch does the minimal thing in "making things fail on error" now that
we have a case where things are guaranteed to be out of sync. One can argue that
we actually should fail migration on the source instead of sending 
S390_TOD_CLOCK_VALUE_MISSING.

If you want to send a patch that make migration fail for more cases I am 
inclined to ack such a thing.