[[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls

[[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Enable KVM_SET_CLOCK and KVM_GET_CLOCK Ioctls on S390 for managing guest TOD
clock value.

we add the KVM_CLOCK_FORWARD_ONLY flag to indicate to KVM_SET_CLOCK that the
given clock value should only be set if it is >= the current guest TOD clock
value. This guarantees a monotonically increasing time.

NOTE: In the event that the KVM_CLOCK_FORWARD_ONLY flag is set and the given
time would cause the guest time to jump backward, then we set the guest TOD
clock equal to the host TOD clock. Does this behavior make sense, or is it too
weird? I could believe that other architectures might not want this exact
behavior. Instead they might prefer to implement the function such that an
error code is returned instead of syncing the guest time to host time? In that
case S390 would need another bit KVM_CLOCK_SET_TO_HOST which we could call to
sync host time when the preferred guest time value would otherwise violate
the monotonic property of the KVM_CLOCK_FORWARD_ONLY flag.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 Documentation/virtual/kvm/api.txt |  5 +++
 arch/s390/kvm/kvm-s390.c          | 80 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/kvm.h          |  3 ++
 3 files changed, 88 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index beae3fd..615c2e4 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -779,6 +779,11 @@ struct kvm_clock_data {
 	__u32 pad[9];
 };
 
+S390: KVM_CLOCK_FORWARD_ONLY is used by KVM_SET_CLOCK to indicate that the guest
+TOD clock should not be allowed to jump back in time. This flag guarantees a
+monotonically increasing guest clock. If the clock value specified would cause
+the guest to jump back in time then the guest TOD clock is set to the host
+TOD clock value.
 
 4.31 KVM_GET_VCPU_EVENTS
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 81b0e11..2450db3 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -31,6 +31,7 @@
 #include <asm/switch_to.h>
 #include <asm/facility.h>
 #include <asm/sclp.h>
+#include<asm/timex.h>
 #include "kvm-s390.h"
 #include "gaccess.h"
 
@@ -169,6 +170,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_S390_IRQCHIP:
 	case KVM_CAP_VM_ATTRIBUTES:
 	case KVM_CAP_MP_STATE:
+	case KVM_CAP_ADJUST_CLOCK:
 		r = 1;
 		break;
 	case KVM_CAP_NR_VCPUS:
@@ -338,6 +340,63 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+static int kvm_s390_get_guest_tod(struct kvm *kvm, struct kvm_clock_data *user_clock)
+{
+	u64 current_host_tod;
+	u64 epoch = 0;
+	struct kvm_vcpu *vcpu;
+	unsigned int vcpu_idx;
+	int r;
+
+	/* All vcpu's epochs are in sync. Just Grab the 1st one */
+	kvm_for_each_vcpu(vcpu_idx, vcpu, kvm)
+	{
+		epoch = vcpu->arch.sie_block->epoch;
+		break;
+	}
+
+	r = store_tod_clock(&current_host_tod);
+	if (r)
+		return r;
+
+	user_clock->clock = current_host_tod + epoch;
+	return 0;
+}
+
+/*
+Set the guest's effective TOD clock to the given value. The guest's
+TOD clock is determined by the following formula: gtod = host_tod + epoch.
+NOTE: Even though the epoch value is associated with a "vcpu", there is only
+one TOD clock and epoch value per guest.  All vcpu's epoch values must be kept
+synchronized.
+NOTE: The KVM_CLOCK_FORWARD_ONLY flag is used to indicate that the guest clock
+should only be set to the provided value if doing so does not cause guest time
+to jump backwards. In this case we zero the epoch thereby making the guest TOD
+clock equal to the host TOD clock.
+*/
+static int kvm_s390_set_guest_tod(struct kvm *kvm, struct kvm_clock_data *user_clock)
+{
+	u64 current_host_tod, epoch;
+	struct kvm_vcpu *vcpu;
+	unsigned int vcpu_idx;
+	int r;
+
+	r = store_tod_clock(&current_host_tod);
+	if (r)
+		return r;
+
+    if ((user_clock->flags & KVM_CLOCK_FORWARD_ONLY) &&
+        (current_host_tod > user_clock->clock))
+        epoch = 0;
+    else
+	    epoch = user_clock->clock - current_host_tod;
+
+	kvm_for_each_vcpu(vcpu_idx, vcpu, kvm)
+		vcpu->arch.sie_block->epoch = epoch;
+
+	return 0;
+}
+
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
@@ -397,6 +456,27 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		r = kvm_s390_vm_has_attr(kvm, &attr);
 		break;
 	}
+	case KVM_GET_CLOCK: {
+		struct kvm_clock_data user_clock;
+
+		r = kvm_s390_get_guest_tod(kvm, &user_clock);
+		if (r)
+			break;
+
+		if (copy_to_user(argp, &user_clock, sizeof(user_clock)))
+			r = -EFAULT;
+
+		break;
+	}
+	case KVM_SET_CLOCK: {
+		struct kvm_clock_data user_clock;
+
+		r = -EFAULT;
+		if (copy_from_user(&user_clock, argp, sizeof(struct kvm_clock_data)))
+			break;
+		r = kvm_s390_set_guest_tod(kvm, &user_clock);
+		break;
+	}
 	default:
 		r = -ENOTTY;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index cf3a2ff..a832b6a 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -857,6 +857,9 @@ struct kvm_irqfd {
 	__u8  pad[16];
 };
 
+/* Flag definitions for kvm_clock_data.flags */
+#define KVM_CLOCK_FORWARD_ONLY (1 << 0)
+
 struct kvm_clock_data {
 	__u64 clock;
 	__u32 flags;
-- 
1.8.3.2

Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls

[Christian Borntraeger]

On 09/19/2014 04:19 PM, Jason J. Herne wrote:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Enable KVM_SET_CLOCK and KVM_GET_CLOCK Ioctls on S390 for managing guest TOD
> clock value.
> 

Just some education. On s390 the guest visible TOD clock is thehost TOD clock + hypervisor programmable offset in the control block. There is only one TOD per system, so the offset must be the same for every CPU.

> we add the KVM_CLOCK_FORWARD_ONLY flag to indicate to KVM_SET_CLOCK that the
> given clock value should only be set if it is >= the current guest TOD clock
                                                               host TOD, (right?)

The alternative scheme would be to simply get/set the guest TOD time. This works perfect for migration, but for managedsave the guest time is in the past.
Your approach has the advantange that after managedsave the guest will (most of the time) have the host time of the target system, avoiding that the guest has a time that is in the past (e.g. after 1 week managedsave the guest would live in the past).

Question for Paolo (maybe others) is. Does it make sense to reuse/extend the existing ioctl (I think so, but defining a new one could also be ok)

Christian



> value. This guarantees a monotonically increasing time.
> 
> NOTE: In the event that the KVM_CLOCK_FORWARD_ONLY flag is set and the given
> time would cause the guest time to jump backward, then we set the guest TOD
> clock equal to the host TOD clock. Does this behavior make sense, or is it too
> weird? I could believe that other architectures might not want this exact
> behavior. Instead they might prefer to implement the function such that an
> error code is returned instead of syncing the guest time to host time? In that
> case S390 would need another bit KVM_CLOCK_SET_TO_HOST which we could call to
> sync host time when the preferred guest time value would otherwise violate
> the monotonic property of the KVM_CLOCK_FORWARD_ONLY flag.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  Documentation/virtual/kvm/api.txt |  5 +++
>  arch/s390/kvm/kvm-s390.c          | 80 +++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h          |  3 ++
>  3 files changed, 88 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index beae3fd..615c2e4 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -779,6 +779,11 @@ struct kvm_clock_data {
>  	__u32 pad[9];
>  };
> 
> +S390: KVM_CLOCK_FORWARD_ONLY is used by KVM_SET_CLOCK to indicate that the guest
> +TOD clock should not be allowed to jump back in time. This flag guarantees a
> +monotonically increasing guest clock. If the clock value specified would cause
> +the guest to jump back in time then the guest TOD clock is set to the host
> +TOD clock value.
> 
>  4.31 KVM_GET_VCPU_EVENTS
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 81b0e11..2450db3 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -31,6 +31,7 @@
>  #include <asm/switch_to.h>
>  #include <asm/facility.h>
>  #include <asm/sclp.h>
> +#include<asm/timex.h>
>  #include "kvm-s390.h"
>  #include "gaccess.h"
> 
> @@ -169,6 +170,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_S390_IRQCHIP:
>  	case KVM_CAP_VM_ATTRIBUTES:
>  	case KVM_CAP_MP_STATE:
> +	case KVM_CAP_ADJUST_CLOCK:
>  		r = 1;
>  		break;
>  	case KVM_CAP_NR_VCPUS:
> @@ -338,6 +340,63 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	return ret;
>  }
> 
> +static int kvm_s390_get_guest_tod(struct kvm *kvm, struct kvm_clock_data *user_clock)
> +{
> +	u64 current_host_tod;
> +	u64 epoch = 0;
> +	struct kvm_vcpu *vcpu;
> +	unsigned int vcpu_idx;
> +	int r;
> +
> +	/* All vcpu's epochs are in sync. Just Grab the 1st one */
> +	kvm_for_each_vcpu(vcpu_idx, vcpu, kvm)
> +	{
> +		epoch = vcpu->arch.sie_block->epoch;
> +		break;
> +	}
> +
> +	r = store_tod_clock(&current_host_tod);
> +	if (r)
> +		return r;
> +
> +	user_clock->clock = current_host_tod + epoch;
> +	return 0;
> +}
> +
> +/*
> +Set the guest's effective TOD clock to the given value. The guest's
> +TOD clock is determined by the following formula: gtod = host_tod + epoch.
> +NOTE: Even though the epoch value is associated with a "vcpu", there is only
> +one TOD clock and epoch value per guest.  All vcpu's epoch values must be kept
> +synchronized.
> +NOTE: The KVM_CLOCK_FORWARD_ONLY flag is used to indicate that the guest clock
> +should only be set to the provided value if doing so does not cause guest time
> +to jump backwards. In this case we zero the epoch thereby making the guest TOD
> +clock equal to the host TOD clock.
> +*/
> +static int kvm_s390_set_guest_tod(struct kvm *kvm, struct kvm_clock_data *user_clock)
> +{
> +	u64 current_host_tod, epoch;
> +	struct kvm_vcpu *vcpu;
> +	unsigned int vcpu_idx;
> +	int r;
> +
> +	r = store_tod_clock(&current_host_tod);
> +	if (r)
> +		return r;
> +
> +    if ((user_clock->flags & KVM_CLOCK_FORWARD_ONLY) &&
> +        (current_host_tod > user_clock->clock))
> +        epoch = 0;
> +    else
> +	    epoch = user_clock->clock - current_host_tod;
> +
> +	kvm_for_each_vcpu(vcpu_idx, vcpu, kvm)
> +		vcpu->arch.sie_block->epoch = epoch;
> +
> +	return 0;
> +}
> +
>  long kvm_arch_vm_ioctl(struct file *filp,
>  		       unsigned int ioctl, unsigned long arg)
>  {
> @@ -397,6 +456,27 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		r = kvm_s390_vm_has_attr(kvm, &attr);
>  		break;
>  	}
> +	case KVM_GET_CLOCK: {
> +		struct kvm_clock_data user_clock;
> +
> +		r = kvm_s390_get_guest_tod(kvm, &user_clock);
> +		if (r)
> +			break;
> +
> +		if (copy_to_user(argp, &user_clock, sizeof(user_clock)))
> +			r = -EFAULT;
> +
> +		break;
> +	}
> +	case KVM_SET_CLOCK: {
> +		struct kvm_clock_data user_clock;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&user_clock, argp, sizeof(struct kvm_clock_data)))
> +			break;
> +		r = kvm_s390_set_guest_tod(kvm, &user_clock);
> +		break;
> +	}
>  	default:
>  		r = -ENOTTY;
>  	}
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index cf3a2ff..a832b6a 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -857,6 +857,9 @@ struct kvm_irqfd {
>  	__u8  pad[16];
>  };
> 
> +/* Flag definitions for kvm_clock_data.flags */
> +#define KVM_CLOCK_FORWARD_ONLY (1 << 0)
> +
>  struct kvm_clock_data {
>  	__u64 clock;
>  	__u32 flags;
> 

Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls

[Alexander Graf]

On 19.09.14 20:51, Christian Borntraeger wrote:
> On 09/19/2014 04:19 PM, Jason J. Herne wrote:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Enable KVM_SET_CLOCK and KVM_GET_CLOCK Ioctls on S390 for managing guest TOD
>> clock value.
>>
> 
> Just some education. On s390 the guest visible TOD clock is thehost TOD clock + hypervisor programmable offset in the control block. There is only one TOD per system, so the offset must be the same for every CPU.

Can that offset be negative?

> 
>> we add the KVM_CLOCK_FORWARD_ONLY flag to indicate to KVM_SET_CLOCK that the
>> given clock value should only be set if it is >= the current guest TOD clock
>                                                                host TOD, (right?)
> 
> The alternative scheme would be to simply get/set the guest TOD time. This works perfect for migration, but for managedsave the guest time is in the past.
> Your approach has the advantange that after managedsave the guest will (most of the time) have the host time of the target system, avoiding that the guest has a time that is in the past (e.g. after 1 week managedsave the guest would live in the past).

But that's what users will expect, no? When you save an image in the
past, it should resume at that very point in time.

Also I personally don't care whether the interface is "delta to now" or
"this is the time". In general, "delta to now" is safer because you
can't possibly run back in time. But you also definitely want to check
out the way PPC does it - it also accomodates for the time we spend
inside the migration path itself.


Alex

> 
> Question for Paolo (maybe others) is. Does it make sense to reuse/extend the existing ioctl (I think so, but defining a new one could also be ok)
> 
> Christian
> 
> 
> 
>> value. This guarantees a monotonically increasing time.
>>
>> NOTE: In the event that the KVM_CLOCK_FORWARD_ONLY flag is set and the given
>> time would cause the guest time to jump backward, then we set the guest TOD
>> clock equal to the host TOD clock. Does this behavior make sense, or is it too
>> weird? I could believe that other architectures might not want this exact
>> behavior. Instead they might prefer to implement the function such that an
>> error code is returned instead of syncing the guest time to host time? In that
>> case S390 would need another bit KVM_CLOCK_SET_TO_HOST which we could call to
>> sync host time when the preferred guest time value would otherwise violate
>> the monotonic property of the KVM_CLOCK_FORWARD_ONLY flag.
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>> ---
>>  Documentation/virtual/kvm/api.txt |  5 +++
>>  arch/s390/kvm/kvm-s390.c          | 80 +++++++++++++++++++++++++++++++++++++++
>>  include/uapi/linux/kvm.h          |  3 ++
>>  3 files changed, 88 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index beae3fd..615c2e4 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -779,6 +779,11 @@ struct kvm_clock_data {
>>  	__u32 pad[9];
>>  };
>>
>> +S390: KVM_CLOCK_FORWARD_ONLY is used by KVM_SET_CLOCK to indicate that the guest
>> +TOD clock should not be allowed to jump back in time. This flag guarantees a
>> +monotonically increasing guest clock. If the clock value specified would cause
>> +the guest to jump back in time then the guest TOD clock is set to the host
>> +TOD clock value.
>>
>>  4.31 KVM_GET_VCPU_EVENTS
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 81b0e11..2450db3 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -31,6 +31,7 @@
>>  #include <asm/switch_to.h>
>>  #include <asm/facility.h>
>>  #include <asm/sclp.h>
>> +#include<asm/timex.h>
>>  #include "kvm-s390.h"
>>  #include "gaccess.h"
>>
>> @@ -169,6 +170,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_S390_IRQCHIP:
>>  	case KVM_CAP_VM_ATTRIBUTES:
>>  	case KVM_CAP_MP_STATE:
>> +	case KVM_CAP_ADJUST_CLOCK:
>>  		r = 1;
>>  		break;
>>  	case KVM_CAP_NR_VCPUS:
>> @@ -338,6 +340,63 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>  	return ret;
>>  }
>>
>> +static int kvm_s390_get_guest_tod(struct kvm *kvm, struct kvm_clock_data *user_clock)
>> +{
>> +	u64 current_host_tod;
>> +	u64 epoch = 0;
>> +	struct kvm_vcpu *vcpu;
>> +	unsigned int vcpu_idx;
>> +	int r;
>> +
>> +	/* All vcpu's epochs are in sync. Just Grab the 1st one */
>> +	kvm_for_each_vcpu(vcpu_idx, vcpu, kvm)
>> +	{
>> +		epoch = vcpu->arch.sie_block->epoch;
>> +		break;
>> +	}
>> +
>> +	r = store_tod_clock(&current_host_tod);
>> +	if (r)
>> +		return r;
>> +
>> +	user_clock->clock = current_host_tod + epoch;
>> +	return 0;
>> +}
>> +
>> +/*
>> +Set the guest's effective TOD clock to the given value. The guest's
>> +TOD clock is determined by the following formula: gtod = host_tod + epoch.
>> +NOTE: Even though the epoch value is associated with a "vcpu", there is only
>> +one TOD clock and epoch value per guest.  All vcpu's epoch values must be kept
>> +synchronized.
>> +NOTE: The KVM_CLOCK_FORWARD_ONLY flag is used to indicate that the guest clock
>> +should only be set to the provided value if doing so does not cause guest time
>> +to jump backwards. In this case we zero the epoch thereby making the guest TOD
>> +clock equal to the host TOD clock.

I don't see any point in this.


Alex

Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls

[Christian Borntraeger]

On 09/19/2014 10:38 PM, Alexander Graf wrote:
> 
> 
> On 19.09.14 20:51, Christian Borntraeger wrote:
>> On 09/19/2014 04:19 PM, Jason J. Herne wrote:
>>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>>
>>> Enable KVM_SET_CLOCK and KVM_GET_CLOCK Ioctls on S390 for managing guest TOD
>>> clock value.
>>>
>>
>> Just some education. On s390 the guest visible TOD clock is thehost TOD clock + hypervisor programmable offset in the control block. There is only one TOD per system, so the offset must be the same for every CPU.
> 
> Can that offset be negative?

The offset is an u64, but the usual sum rules apply. The carry is irgnored and by using a large value you can have a negative offset.

> 
>>
>>> we add the KVM_CLOCK_FORWARD_ONLY flag to indicate to KVM_SET_CLOCK that the
>>> given clock value should only be set if it is >= the current guest TOD clock
>>                                                                host TOD, (right?)
>>
>> The alternative scheme would be to simply get/set the guest TOD time. This works perfect for migration, but for managedsave the guest time is in the past.
>> Your approach has the advantange that after managedsave the guest will (most of the time) have the host time of the target system, avoiding that the guest has a time that is in the past (e.g. after 1 week managedsave the guest would live in the past).
> 
> But that's what users will expect, no? When you save an image in the
> past, it should resume at that very point in time.

Actually, I would expect something different (more or less something like standby/resume).

In fact Jasons code that we have internally in testing is doing the simple approach
1. source reads guest time at migration end
2. target sets guest time from source

So we have the guarantee that the time will never move backwards. It also works quite well for migration. As a bonus, we could really reuse the existing ioctl. 

I asked Jason to explore alternatives, though: I think it is somehow wrong, if you save a guest into an image file, open that one month later and the guest will always be 1 month behind unless it uses some kind of ntp. If everybody agrees that this is fine, I will queue up Jasons intial patch (simple get/set). 
The only question is then: shall we use an s390 specific ioctl (e.g. via VM attribute) or just use the existing KVM_SET_CLOCK.
But maybe lets answer the first question before we decide on this.

> 
> Also I personally don't care whether the interface is "delta to now" or
> "this is the time". In general, "delta to now" is safer because you
> can't possibly run back in time. But you also definitely want to check
> out the way PPC does it - it also accomodates for the time we spend
> inside the migration path itself.
> 
> 
> Alex
> 
>>
>> Question for Paolo (maybe others) is. Does it make sense to reuse/extend the existing ioctl (I think so, but defining a new one could also be ok)
>>
>> Christian
>>
>>
>>
>>> value. This guarantees a monotonically increasing time.
>>>
>>> NOTE: In the event that the KVM_CLOCK_FORWARD_ONLY flag is set and the given
>>> time would cause the guest time to jump backward, then we set the guest TOD
>>> clock equal to the host TOD clock. Does this behavior make sense, or is it too
>>> weird? I could believe that other architectures might not want this exact
>>> behavior. Instead they might prefer to implement the function such that an
>>> error code is returned instead of syncing the guest time to host time? In that
>>> case S390 would need another bit KVM_CLOCK_SET_TO_HOST which we could call to
>>> sync host time when the preferred guest time value would otherwise violate
>>> the monotonic property of the KVM_CLOCK_FORWARD_ONLY flag.
>>>
>>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>>> ---
>>>  Documentation/virtual/kvm/api.txt |  5 +++
>>>  arch/s390/kvm/kvm-s390.c          | 80 +++++++++++++++++++++++++++++++++++++++
>>>  include/uapi/linux/kvm.h          |  3 ++
>>>  3 files changed, 88 insertions(+)
>>>
>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>> index beae3fd..615c2e4 100644
>>> --- a/Documentation/virtual/kvm/api.txt
>>> +++ b/Documentation/virtual/kvm/api.txt
>>> @@ -779,6 +779,11 @@ struct kvm_clock_data {
>>>  	__u32 pad[9];
>>>  };
>>>
>>> +S390: KVM_CLOCK_FORWARD_ONLY is used by KVM_SET_CLOCK to indicate that the guest
>>> +TOD clock should not be allowed to jump back in time. This flag guarantees a
>>> +monotonically increasing guest clock. If the clock value specified would cause
>>> +the guest to jump back in time then the guest TOD clock is set to the host
>>> +TOD clock value.
>>>
>>>  4.31 KVM_GET_VCPU_EVENTS
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 81b0e11..2450db3 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -31,6 +31,7 @@
>>>  #include <asm/switch_to.h>
>>>  #include <asm/facility.h>
>>>  #include <asm/sclp.h>
>>> +#include<asm/timex.h>
>>>  #include "kvm-s390.h"
>>>  #include "gaccess.h"
>>>
>>> @@ -169,6 +170,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>  	case KVM_CAP_S390_IRQCHIP:
>>>  	case KVM_CAP_VM_ATTRIBUTES:
>>>  	case KVM_CAP_MP_STATE:
>>> +	case KVM_CAP_ADJUST_CLOCK:
>>>  		r = 1;
>>>  		break;
>>>  	case KVM_CAP_NR_VCPUS:
>>> @@ -338,6 +340,63 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>>  	return ret;
>>>  }
>>>
>>> +static int kvm_s390_get_guest_tod(struct kvm *kvm, struct kvm_clock_data *user_clock)
>>> +{
>>> +	u64 current_host_tod;
>>> +	u64 epoch = 0;
>>> +	struct kvm_vcpu *vcpu;
>>> +	unsigned int vcpu_idx;
>>> +	int r;
>>> +
>>> +	/* All vcpu's epochs are in sync. Just Grab the 1st one */
>>> +	kvm_for_each_vcpu(vcpu_idx, vcpu, kvm)
>>> +	{
>>> +		epoch = vcpu->arch.sie_block->epoch;
>>> +		break;
>>> +	}
>>> +
>>> +	r = store_tod_clock(&current_host_tod);
>>> +	if (r)
>>> +		return r;
>>> +
>>> +	user_clock->clock = current_host_tod + epoch;
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> +Set the guest's effective TOD clock to the given value. The guest's
>>> +TOD clock is determined by the following formula: gtod = host_tod + epoch.
>>> +NOTE: Even though the epoch value is associated with a "vcpu", there is only
>>> +one TOD clock and epoch value per guest.  All vcpu's epoch values must be kept
>>> +synchronized.
>>> +NOTE: The KVM_CLOCK_FORWARD_ONLY flag is used to indicate that the guest clock
>>> +should only be set to the provided value if doing so does not cause guest time
>>> +to jump backwards. In this case we zero the epoch thereby making the guest TOD
>>> +clock equal to the host TOD clock.
> 
> I don't see any point in this.
> 
> 
> Alex
> 

Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls

[Alexander Graf]

On 08.10.14 16:09, Jason Herne wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> wrote on 09/22/2014
> 05:08:34 AM:
> ...
>> Actually, I would expect something different (more or less something
>> like standby/resume).
>>
>> In fact Jasons code that we have internally in testing is doing the
>> simple approach
>> 1. source reads guest time at migration end
>> 2. target sets guest time from source
>>
>> So we have the guarantee that the time will never move backwards. It
>> also works quite well for migration. As a bonus, we could really
>> reuse the existing ioctl.
>>
>> I asked Jason to explore alternatives, though: I think it is somehow
>> wrong, if you save a guest into an image file, open that one month
>> later and the guest will always be 1 month behind unless it uses
>> some kind of ntp. If everybody agrees that this is fine, I will
>> queue up Jasons intial patch (simple get/set).
>> The only question is then: shall we use an s390 specific ioctl (e.g.
>> via VM attribute) or just use the existing KVM_SET_CLOCK.
>> But maybe lets answer the first question before we decide on this.
> 
> Ping. Does anyone feel strongly about this issue? I'm interested in
> opinions so we can get s390 TOD clock migration working :).
> 
> We need to decide which interface to use, s390 specific ioctl or
> KVM_SET_CLOCK.

I don't have any particular preference. If anything, I'm leaning towards
KVM_SET_CLOCK.

> Then we need to decide if we're going to snap a guest clock forward
> on the resume of a "suspend to disk" type operation. The alternative
> is to fix up the guest TOD value such that the guest notices no
> change of time, which as Christian points out, seems wrong. Unless we
> really want to show no time change and force the guest to use ntp to
> figure out that he is behind.

Just do it the same as x86 :).


Alex

Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls

[Christian Borntraeger]

Am 08.10.2014 16:55, schrieb Alexander Graf:
> 
> 
> On 08.10.14 16:09, Jason Herne wrote:
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote on 09/22/2014
>> 05:08:34 AM:
>> ...
>>> Actually, I would expect something different (more or less something
>>> like standby/resume).
>>>
>>> In fact Jasons code that we have internally in testing is doing the
>>> simple approach
>>> 1. source reads guest time at migration end
>>> 2. target sets guest time from source
>>>
>>> So we have the guarantee that the time will never move backwards. It
>>> also works quite well for migration. As a bonus, we could really
>>> reuse the existing ioctl.
>>>
>>> I asked Jason to explore alternatives, though: I think it is somehow
>>> wrong, if you save a guest into an image file, open that one month
>>> later and the guest will always be 1 month behind unless it uses
>>> some kind of ntp. If everybody agrees that this is fine, I will
>>> queue up Jasons intial patch (simple get/set).
>>> The only question is then: shall we use an s390 specific ioctl (e.g.
>>> via VM attribute) or just use the existing KVM_SET_CLOCK.
>>> But maybe lets answer the first question before we decide on this.
>>
>> Ping. Does anyone feel strongly about this issue? I'm interested in
>> opinions so we can get s390 TOD clock migration working :).
>>
>> We need to decide which interface to use, s390 specific ioctl or
>> KVM_SET_CLOCK.
> 
> I don't have any particular preference. If anything, I'm leaning towards
> KVM_SET_CLOCK.
> 
>> Then we need to decide if we're going to snap a guest clock forward
>> on the resume of a "suspend to disk" type operation. The alternative
>> is to fix up the guest TOD value such that the guest notices no
>> change of time, which as Christian points out, seems wrong. Unless we
>> really want to show no time change and force the guest to use ntp to
>> figure out that he is behind.
> 
> Just do it the same as x86 :).

Yes, that is usally always the right thing to do with Linux :-)

Jason, can you post the minimal patch that used SET_CLOCK/GET_CLOCK to set/get the bits 0-63 of the TOD? (also apply it internally so that we can test it for some days. Its too late for this merge window anyway.)

If we want some different scheme, we can certainly discuss extension via the flags (and pad) in the future. So this interface is certainly not a dead end if we need more

I have 2 possible extension in mind:
1. the thing that we discussed, lets see if we need a fix or not
2. making KVM on s390x ready for 2042 and beyond (there is no architecture yet but STCKE stores a byte of zeroes to the left of the TOD clock value. So I guess if this is extended somewhen we might want an additional flag plus a maximum of 1 additional byte. There is plenty of pad space so this is fine

Christian

Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls

[Jason J. Herne]

This is a reply to the following thread:
http://www.spinics.net/lists/kvm/msg108448.html

I'm sending it in this fashion because my normal mail client
is not allowing me to send it in plain text and the html is
getting rejected by the mailing list. Sorry to those of you who
received both this and the original.


Ping. Does anyone feel strongly about this issue? I'm interested in
opinions so we can get s390 TOD clock migration working :).

We need to decide which interface to use, s390 specific ioctl or
KVM_SET_CLOCK.

Then we need to decide if we're going to snap a guest clock forward
on the resume of a "suspend to disk" type operation. The alternative
is to fix up the guest TOD value such that the guest notices no
change of time, which as Christian points out, seems wrong. Unless we
really want to show no time change and force the guest to use ntp to
figure out that he is behind.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)