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" <jjhe...@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 <jjhe...@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(¤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.
>
> I don't see any point in this.
>
>
> 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
