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

[Cornelia Huck]

On Mon, 25 Sep 2017 12:23:02 +0200
Christian Borntraeger <address@hidden> wrote:

> From: "Collin L. Walling" <address@hidden>
> 
> 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.
> 
> If kvm fails to set a nonzero epoch index, then we ultimately fail
> the migration altogether and the guest will resume normally on the
> original host machine.

I'd prefer to have that part split off, as it is a change in behaviour
and I don't think we should mix it with adding support for an improved
interface.

> 
> 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>
> ---
>  hw/s390x/s390-virtio-ccw.c |  8 +++-----
>  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, 69 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index fafbc6d..bad09f5 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -213,13 +213,11 @@ 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 guest clock value. "
> +                     "s390_get_clock returned error %d.\n", r);

Please keep to a single phrase in error_report(). Also, I find
strerror() often more useful.

>      }
>  
> -    return 0;
> +    return r;
>  }
>  
>  static SaveVMHandlers savevm_gtod = {

Otherwise looks good.