[Qemu-devel] [PATCH v2 0/2] s390/z14: extended TOD-clock support

[Qemu-devel] [PATCH v2 0/2] s390/z14: extended TOD-clock support

[Christian Borntraeger]

The last last remaining z14 item on my queue.

v1->v2:
- split patches
- fix white spaces (also fixup kvm_s390_set_clock)
- single phrase error message
- use strerror

Collin L. Walling (2):
  s390/kvm: Support for get/set of extended TOD-Clock for guest
  s390/kvm: make TOD setting failures fatal for migration

 hw/s390x/s390-virtio-ccw.c |  7 ++-----
 target/s390x/cpu.c         | 26 +++++++++++++++++++-------
 target/s390x/kvm-stub.c    | 10 ++++++++++
 target/s390x/kvm.c         | 35 ++++++++++++++++++++++++++++++++++-
 target/s390x/kvm_s390x.h   |  2 ++
 5 files changed, 67 insertions(+), 13 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v2 1/2] s390/kvm: Support for get/set of extended TOD-Clock for guest

[Christian Borntraeger]

From: "Collin L. Walling" <walling@linux.vnet.ibm.com>

Provides an interface for getting and setting the guest's extended
TOD-Clock via a single ioctl to kvm. If the ioctl fails because it
is not support by kvm, then we fall back to the old style of
retrieving the clock via two ioctls.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[split failure change from epoch index change]
---
 target/s390x/cpu.c       | 26 +++++++++++++++++++-------
 target/s390x/kvm-stub.c  | 10 ++++++++++
 target/s390x/kvm.c       | 35 ++++++++++++++++++++++++++++++++++-
 target/s390x/kvm_s390x.h |  2 ++
 4 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 34538c3..c8f1219 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -357,22 +357,34 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
 
 int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
 {
+    int r = 0;
+
     if (kvm_enabled()) {
-        return kvm_s390_get_clock(tod_high, tod_low);
+        r = kvm_s390_get_clock_ext(tod_high, tod_low);
+        if (r == -ENXIO) {
+            return kvm_s390_get_clock(tod_high, tod_low);
+        }
+    } else {
+        /* Fixme TCG */
+        *tod_high = 0;
+        *tod_low = 0;
     }
-    /* Fixme TCG */
-    *tod_high = 0;
-    *tod_low = 0;
-    return 0;
+
+    return r;
 }
 
 int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
 {
+    int r = 0;
+
     if (kvm_enabled()) {
-        return kvm_s390_set_clock(tod_high, tod_low);
+        r = kvm_s390_set_clock_ext(tod_high, tod_low);
+        if (r == -ENXIO) {
+            return kvm_s390_set_clock(tod_high, tod_low);
+        }
     }
     /* Fixme TCG */
-    return 0;
+    return r;
 }
 
 int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
index 261e1cd..43f02c2 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -68,11 +68,21 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
     return -ENOSYS;
 }
 
+int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
+{
+    return -ENOSYS;
+}
+
 int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
 {
     return -ENOSYS;
 }
 
+int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
+{
+    return -ENOSYS;
+}
+
 void kvm_s390_enable_css_support(S390CPU *cpu)
 {
 }
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index ebb75ca..4c944a5 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -643,10 +643,27 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
     return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
 }
 
+int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
+{
+    int r;
+    struct kvm_s390_vm_tod_clock gtod;
+
+    struct kvm_device_attr attr = {
+        .group = KVM_S390_VM_TOD,
+        .attr = KVM_S390_VM_TOD_EXT,
+        .addr = (uint64_t)(&gtod),
+    };
+
+    r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
+    *tod_high = gtod.epoch_idx;
+    *tod_low  = gtod.tod;
+
+    return r;
+}
+
 int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
 {
     int r;
-
     struct kvm_device_attr attr = {
         .group = KVM_S390_VM_TOD,
         .attr = KVM_S390_VM_TOD_LOW,
@@ -663,6 +680,22 @@ int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
     return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
 }
 
+int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
+{
+    struct kvm_s390_vm_tod_clock gtod = {
+        .epoch_idx = *tod_high,
+        .tod  = *tod_low,
+    };
+
+    struct kvm_device_attr attr = {
+        .group = KVM_S390_VM_TOD,
+        .attr = KVM_S390_VM_TOD_EXT,
+        .addr = (uint64_t)(&gtod),
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
+}
+
 /**
  * kvm_s390_mem_op:
  * @addr:      the logical start address in guest memory
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index 2d594bd..501fc5a 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -29,7 +29,9 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu);
 int kvm_s390_get_ri(void);
 int kvm_s390_get_gs(void);
 int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
+int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
 int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_clock);
+int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
 void kvm_s390_enable_css_support(S390CPU *cpu);
 int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
                                     int vq, bool assign);
-- 
2.9.4

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

[Christian Borntraeger]

From: "Collin L. Walling" <walling@linux.vnet.ibm.com>

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 <walling@linux.vnet.ibm.com>
Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[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;
 }
 
 static SaveVMHandlers savevm_gtod = {
-- 
2.9.4

Re: [Qemu-devel] [PATCH v2 1/2] s390/kvm: Support for get/set of extended TOD-Clock for guest

[Thomas Huth]

On 04.10.2017 12:57, Christian Borntraeger wrote:
> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
> 
> Provides an interface for getting and setting the guest's extended
> TOD-Clock via a single ioctl to kvm. If the ioctl fails because it
> is not support by kvm, then we fall back to the old style of
> retrieving the clock via two ioctls.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [split failure change from epoch index change]
> ---
>  target/s390x/cpu.c       | 26 +++++++++++++++++++-------
>  target/s390x/kvm-stub.c  | 10 ++++++++++
>  target/s390x/kvm.c       | 35 ++++++++++++++++++++++++++++++++++-
>  target/s390x/kvm_s390x.h |  2 ++
>  4 files changed, 65 insertions(+), 8 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 34538c3..c8f1219 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -357,22 +357,34 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
>  
>  int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
>  {
> +    int r = 0;
> +
>      if (kvm_enabled()) {
> -        return kvm_s390_get_clock(tod_high, tod_low);
> +        r = kvm_s390_get_clock_ext(tod_high, tod_low);
> +        if (r == -ENXIO) {
> +            return kvm_s390_get_clock(tod_high, tod_low);
> +        }
> +    } else {
> +        /* Fixme TCG */
> +        *tod_high = 0;
> +        *tod_low = 0;
>      }
> -    /* Fixme TCG */
> -    *tod_high = 0;
> -    *tod_low = 0;
> -    return 0;
> +
> +    return r;
>  }
>  
>  int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>  {
> +    int r = 0;
> +
>      if (kvm_enabled()) {
> -        return kvm_s390_set_clock(tod_high, tod_low);
> +        r = kvm_s390_set_clock_ext(tod_high, tod_low);
> +        if (r == -ENXIO) {
> +            return kvm_s390_set_clock(tod_high, tod_low);
> +        }
>      }
>      /* Fixme TCG */
> -    return 0;
> +    return r;
>  }
>  
>  int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
> diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
> index 261e1cd..43f02c2 100644
> --- a/target/s390x/kvm-stub.c
> +++ b/target/s390x/kvm-stub.c
> @@ -68,11 +68,21 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
>      return -ENOSYS;
>  }
>  
> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
> +{
> +    return -ENOSYS;
> +}
> +
>  int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>  {
>      return -ENOSYS;
>  }
>  
> +int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
> +{
> +    return -ENOSYS;
> +}
> +
>  void kvm_s390_enable_css_support(S390CPU *cpu)
>  {
>  }
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index ebb75ca..4c944a5 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -643,10 +643,27 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
>      return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
>  }
>  
> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
> +{
> +    int r;
> +    struct kvm_s390_vm_tod_clock gtod;
> +

So you've got a blank line here...

> +    struct kvm_device_attr attr = {
> +        .group = KVM_S390_VM_TOD,
> +        .attr = KVM_S390_VM_TOD_EXT,
> +        .addr = (uint64_t)(&gtod),
> +    };
> +
> +    r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
> +    *tod_high = gtod.epoch_idx;
> +    *tod_low  = gtod.tod;
> +
> +    return r;
> +}
> +
>  int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>  {
>      int r;
> -

... but removed the blank line here ... it would be more consequent to
do it the same way everywhere?

>      struct kvm_device_attr attr = {
>          .group = KVM_S390_VM_TOD,
>          .attr = KVM_S390_VM_TOD_LOW,
> @@ -663,6 +680,22 @@ int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>      return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>  }
>  
> +int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
> +{
> +    struct kvm_s390_vm_tod_clock gtod = {
> +        .epoch_idx = *tod_high,
> +        .tod  = *tod_low,
> +    };
> +
> +    struct kvm_device_attr attr = {
> +        .group = KVM_S390_VM_TOD,
> +        .attr = KVM_S390_VM_TOD_EXT,
> +        .addr = (uint64_t)(&gtod),

You could remove the parentheses around "&gtod" (same in the
get_clock_ext function).

> +    };
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
> +}
> +
>  /**
>   * kvm_s390_mem_op:
>   * @addr:      the logical start address in guest memory
> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
> index 2d594bd..501fc5a 100644
> --- a/target/s390x/kvm_s390x.h
> +++ b/target/s390x/kvm_s390x.h
> @@ -29,7 +29,9 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu);
>  int kvm_s390_get_ri(void);
>  int kvm_s390_get_gs(void);
>  int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
>  int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_clock);
> +int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
>  void kvm_s390_enable_css_support(S390CPU *cpu);
>  int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
>                                      int vq, bool assign);
> 

Just cosmetic nits... patch looks fine otherwise, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/2] s390/kvm: Support for get/set of extended TOD-Clock for guest

[Christian Borntraeger]

On 10/04/2017 01:42 PM, Thomas Huth wrote:
> On 04.10.2017 12:57, Christian Borntraeger wrote:
>> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
>>
>> Provides an interface for getting and setting the guest's extended
>> TOD-Clock via a single ioctl to kvm. If the ioctl fails because it
>> is not support by kvm, then we fall back to the old style of
>> retrieving the clock via two ioctls.
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> [split failure change from epoch index change]
>> ---
>>  target/s390x/cpu.c       | 26 +++++++++++++++++++-------
>>  target/s390x/kvm-stub.c  | 10 ++++++++++
>>  target/s390x/kvm.c       | 35 ++++++++++++++++++++++++++++++++++-
>>  target/s390x/kvm_s390x.h |  2 ++
>>  4 files changed, 65 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 34538c3..c8f1219 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -357,22 +357,34 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
>>  
>>  int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
>>  {
>> +    int r = 0;
>> +
>>      if (kvm_enabled()) {
>> -        return kvm_s390_get_clock(tod_high, tod_low);
>> +        r = kvm_s390_get_clock_ext(tod_high, tod_low);
>> +        if (r == -ENXIO) {
>> +            return kvm_s390_get_clock(tod_high, tod_low);
>> +        }
>> +    } else {
>> +        /* Fixme TCG */
>> +        *tod_high = 0;
>> +        *tod_low = 0;
>>      }
>> -    /* Fixme TCG */
>> -    *tod_high = 0;
>> -    *tod_low = 0;
>> -    return 0;
>> +
>> +    return r;
>>  }
>>  
>>  int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>>  {
>> +    int r = 0;
>> +
>>      if (kvm_enabled()) {
>> -        return kvm_s390_set_clock(tod_high, tod_low);
>> +        r = kvm_s390_set_clock_ext(tod_high, tod_low);
>> +        if (r == -ENXIO) {
>> +            return kvm_s390_set_clock(tod_high, tod_low);
>> +        }
>>      }
>>      /* Fixme TCG */
>> -    return 0;
>> +    return r;
>>  }
>>  
>>  int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
>> diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
>> index 261e1cd..43f02c2 100644
>> --- a/target/s390x/kvm-stub.c
>> +++ b/target/s390x/kvm-stub.c
>> @@ -68,11 +68,21 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
>>      return -ENOSYS;
>>  }
>>  
>> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
>> +{
>> +    return -ENOSYS;
>> +}
>> +
>>  int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>>  {
>>      return -ENOSYS;
>>  }
>>  
>> +int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
>> +{
>> +    return -ENOSYS;
>> +}
>> +
>>  void kvm_s390_enable_css_support(S390CPU *cpu)
>>  {
>>  }
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index ebb75ca..4c944a5 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -643,10 +643,27 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
>>      return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
>>  }
>>  
>> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
>> +{
>> +    int r;
>> +    struct kvm_s390_vm_tod_clock gtod;
>> +
> 
> So you've got a blank line here...

Yes, seems that I have forgotten this one. 
I will let Conny decide if I should resend or if she can fixup.


> 
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_S390_VM_TOD,
>> +        .attr = KVM_S390_VM_TOD_EXT,
>> +        .addr = (uint64_t)(&gtod),
>> +    };
>> +
>> +    r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
>> +    *tod_high = gtod.epoch_idx;
>> +    *tod_low  = gtod.tod;
>> +
>> +    return r;
>> +}
>> +
>>  int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>>  {
>>      int r;
>> -
> 
> ... but removed the blank line here ... it would be more consequent to
> do it the same way everywhere?
> 
>>      struct kvm_device_attr attr = {
>>          .group = KVM_S390_VM_TOD,
>>          .attr = KVM_S390_VM_TOD_LOW,
>> @@ -663,6 +680,22 @@ int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>>      return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>>  }
>>  
>> +int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
>> +{
>> +    struct kvm_s390_vm_tod_clock gtod = {
>> +        .epoch_idx = *tod_high,
>> +        .tod  = *tod_low,
>> +    };
>> +
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_S390_VM_TOD,
>> +        .attr = KVM_S390_VM_TOD_EXT,
>> +        .addr = (uint64_t)(&gtod),
> 
> You could remove the parentheses around "&gtod" (same in the
> get_clock_ext function).

dito.
> 
>> +    };
>> +
>> +    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>> +}
>> +
>>  /**
>>   * kvm_s390_mem_op:
>>   * @addr:      the logical start address in guest memory
>> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
>> index 2d594bd..501fc5a 100644
>> --- a/target/s390x/kvm_s390x.h
>> +++ b/target/s390x/kvm_s390x.h
>> @@ -29,7 +29,9 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu);
>>  int kvm_s390_get_ri(void);
>>  int kvm_s390_get_gs(void);
>>  int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
>> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
>>  int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_clock);
>> +int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
>>  void kvm_s390_enable_css_support(S390CPU *cpu);
>>  int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
>>                                      int vq, bool assign);
>>
> 
> Just cosmetic nits... patch looks fine otherwise, so:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

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

[Thomas Huth]

On 04.10.2017 12:57, Christian Borntraeger wrote:
> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
> 
> 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 <walling@linux.vnet.ibm.com>
> Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [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 <thuth@redhat.com>

(and I wonder whether we should fail the S390_TOD_CLOCK_VALUE_MISSING
case with returning an error instead of 0, too?)

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" <walling@linux.vnet.ibm.com>
>>
>> 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 <walling@linux.vnet.ibm.com>
>> Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> [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 <thuth@redhat.com>
> 
> (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.

Re: [Qemu-devel] [PATCH v2 1/2] s390/kvm: Support for get/set of extended TOD-Clock for guest

[Cornelia Huck]

On Wed, 4 Oct 2017 13:44:31 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 10/04/2017 01:42 PM, Thomas Huth wrote:
> > On 04.10.2017 12:57, Christian Borntraeger wrote:  
> >> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
> >>
> >> Provides an interface for getting and setting the guest's extended
> >> TOD-Clock via a single ioctl to kvm. If the ioctl fails because it
> >> is not support by kvm, then we fall back to the old style of
> >> retrieving the clock via two ioctls.
> >>
> >> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> >> Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
> >> Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> [split failure change from epoch index change]
> >> ---
> >>  target/s390x/cpu.c       | 26 +++++++++++++++++++-------
> >>  target/s390x/kvm-stub.c  | 10 ++++++++++
> >>  target/s390x/kvm.c       | 35 ++++++++++++++++++++++++++++++++++-
> >>  target/s390x/kvm_s390x.h |  2 ++
> >>  4 files changed, 65 insertions(+), 8 deletions(-)

> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >> index ebb75ca..4c944a5 100644
> >> --- a/target/s390x/kvm.c
> >> +++ b/target/s390x/kvm.c
> >> @@ -643,10 +643,27 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
> >>      return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
> >>  }
> >>  
> >> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
> >> +{
> >> +    int r;
> >> +    struct kvm_s390_vm_tod_clock gtod;
> >> +  
> > 
> > So you've got a blank line here...  
> 
> Yes, seems that I have forgotten this one. 
> I will let Conny decide if I should resend or if she can fixup.

No worries, I can make this consistent on applying.

Re: [Qemu-devel] [PATCH v2 0/2] s390/z14: extended TOD-clock support

[Cornelia Huck]

On Wed,  4 Oct 2017 12:57:49 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> The last last remaining z14 item on my queue.

So the really last one? :)

> 
> v1->v2:
> - split patches
> - fix white spaces (also fixup kvm_s390_set_clock)
> - single phrase error message
> - use strerror
> 
> Collin L. Walling (2):
>   s390/kvm: Support for get/set of extended TOD-Clock for guest
>   s390/kvm: make TOD setting failures fatal for migration
> 
>  hw/s390x/s390-virtio-ccw.c |  7 ++-----
>  target/s390x/cpu.c         | 26 +++++++++++++++++++-------
>  target/s390x/kvm-stub.c    | 10 ++++++++++
>  target/s390x/kvm.c         | 35 ++++++++++++++++++++++++++++++++++-
>  target/s390x/kvm_s390x.h   |  2 ++
>  5 files changed, 67 insertions(+), 13 deletions(-)
> 

Thanks, applied.

Re: [Qemu-devel] [PATCH v2 0/2] s390/z14: extended TOD-clock support

[Christian Borntraeger]

On 10/04/2017 03:57 PM, Cornelia Huck wrote:
> On Wed,  4 Oct 2017 12:57:49 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> The last last remaining z14 item on my queue.
> 
> So the really last one? :)

Have a look at the full model of z14 in target/s390x/gen-features.c

There are some things that we could do, for example the test pending
external interruption or the insert reference multiple. Both instructions
do not make much sense for Linux.

Then there is hpma2, which is a host memory management thing and only becomes
relevant if we ever implement it for vsie.

But nothing of that is high priority right now.
> 
>>
>> v1->v2:
>> - split patches
>> - fix white spaces (also fixup kvm_s390_set_clock)
>> - single phrase error message
>> - use strerror
>>
>> Collin L. Walling (2):
>>   s390/kvm: Support for get/set of extended TOD-Clock for guest
>>   s390/kvm: make TOD setting failures fatal for migration
>>
>>  hw/s390x/s390-virtio-ccw.c |  7 ++-----
>>  target/s390x/cpu.c         | 26 +++++++++++++++++++-------
>>  target/s390x/kvm-stub.c    | 10 ++++++++++
>>  target/s390x/kvm.c         | 35 ++++++++++++++++++++++++++++++++++-
>>  target/s390x/kvm_s390x.h   |  2 ++
>>  5 files changed, 67 insertions(+), 13 deletions(-)
>>
> 
> Thanks, applied.
>