Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls
Am 08.10.2014 16:55, schrieb Alexander Graf: > > > On 08.10.14 16:09, Jason Herne wrote: >> Christian Borntraeger 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls
On 08.10.14 16:09, Jason Herne wrote: > Christian Borntraeger 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls
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 (jjhe...@linux.vnet.ibm.com) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls
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"
>>>
>>> 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
>>> ---
>>> 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
>>> #include
>>> #include
>>> +#include
>>> #include "kv
Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls
On 19.09.14 20:51, Christian Borntraeger wrote:
> On 09/19/2014 04:19 PM, Jason J. Herne wrote:
>> From: "Jason J. Herne"
>>
>> 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
>> ---
>> 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
>> #include
>> #include
>> +#include
>> #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(¤t_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_
Re: [[RFC] KVM-S390: Provide guest TOD Clock Get/Set Controls
On 09/19/2014 04:19 PM, Jason J. Herne wrote:
> From: "Jason J. Herne"
>
> 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
> ---
> 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
> #include
> #include
> +#include
> #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(¤t_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 kv
