Xen devel - Jun 2013 - [PATCH v3] x86/vtsc: update vcpu_time in hvm_set_guest_time

When using a vtsc, hvm_set_guest_time changes hvm_vcpu.stime_offset,
which is used in the vcpu time structure to calculate the
tsc_timestamp, so after updating stime_offset we need to propagate the
change to vcpu_time in order for the guest to get the right time if
using the PV clock.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes since v2:
 * Check for v == current instead of v->is_running to know if the vcpu
   is running.
Changes since v1:
 * Perform the call to update_vcpu_system_time in hvm_set_guest_time
   if the offset has changed and the vCPU is running.
---
 xen/arch/x86/hvm/vpt.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 8dee662..4b1e1a3 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -57,7 +57,18 @@ u64 hvm_get_guest_time(struct vcpu *v)
 
 void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
 {
-    v->arch.hvm_vcpu.stime_offset += guest_time - hvm_get_guest_time(v);
+    u64 offset = guest_time - hvm_get_guest_time(v);
+
+    if ( offset ) {
+        v->arch.hvm_vcpu.stime_offset += offset;
+        /*
+         * If hvm_vcpu.stime_offset is updated make sure to
+         * also update vcpu time, since this value is used to
+         * calculate the TSC.
+         */
+        if ( v == current )
+            update_vcpu_system_time(v);
+    }
 }
 
 static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Tue, Jun 4, 2013 at 4:32 PM, Roger Pau Monne <roger.pau@citrix.com>
wrote:
> When using a vtsc, hvm_set_guest_time changes hvm_vcpu.stime_offset,
> which is used in the vcpu time structure to calculate the
> tsc_timestamp, so after updating stime_offset we need to propagate the
> change to vcpu_time in order for the guest to get the right time if
> using the PV clock.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Changes since v2:
>  * Check for v == current instead of v->is_running to know if the vcpu
>    is running.
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

It might not be bad at some point to add an ASSERT() in
__update_vcpu_system_time() that v == current.

 -George

On 04/06/13 16:32, Roger Pau Monne wrote:
> When using a vtsc, hvm_set_guest_time changes hvm_vcpu.stime_offset,
> which is used in the vcpu time structure to calculate the
> tsc_timestamp, so after updating stime_offset we need to propagate the
> change to vcpu_time in order for the guest to get the right time if
> using the PV clock.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
Tested-By: Diana Crisan  <dcrisan@flexiant.com>
> ---
> Changes since v2:
>   * Check for v == current instead of v->is_running to know if the vcpu
>     is running.
> Changes since v1:
>   * Perform the call to update_vcpu_system_time in hvm_set_guest_time
>     if the offset has changed and the vCPU is running.
> ---
>   xen/arch/x86/hvm/vpt.c |   13 ++++++++++++-
>   1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
> index 8dee662..4b1e1a3 100644
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -57,7 +57,18 @@ u64 hvm_get_guest_time(struct vcpu *v)
>   
>   void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
>   {
> -    v->arch.hvm_vcpu.stime_offset += guest_time -
hvm_get_guest_time(v);
> +    u64 offset = guest_time - hvm_get_guest_time(v);
> +
> +    if ( offset ) {
> +        v->arch.hvm_vcpu.stime_offset += offset;
> +        /*
> +         * If hvm_vcpu.stime_offset is updated make sure to
> +         * also update vcpu time, since this value is used to
> +         * calculate the TSC.
> +         */
> +        if ( v == current )
> +            update_vcpu_system_time(v);
> +    }
>   }
>   
>   static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
I have tested this patch and I have not been able to reproduce the guest 
stuck-clock problem we were seeing before (see *HVM Migration of domU on 
Qemu-upstream DM causes stuck system clock with ACPI* for details of the 
problem).
Note: this was tested without the temporary fix to our problem ( which 
is specifying tsc_mode='native_paravirt').

Thanks,
Diana

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 05/06/13 13:02, Diana Crisan wrote:
> On 04/06/13 16:32, Roger Pau Monne wrote:
>> When using a vtsc, hvm_set_guest_time changes hvm_vcpu.stime_offset,
>> which is used in the vcpu time structure to calculate the
>> tsc_timestamp, so after updating stime_offset we need to propagate the
>> change to vcpu_time in order for the guest to get the right time if
>> using the PV clock.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Tested-By: Diana Crisan  <dcrisan@flexiant.com>
>> ---
>> Changes since v2:
>>   * Check for v == current instead of v->is_running to know if the
vcpu
>>     is running.
>> Changes since v1:
>>   * Perform the call to update_vcpu_system_time in hvm_set_guest_time
>>     if the offset has changed and the vCPU is running.
>> ---
>>   xen/arch/x86/hvm/vpt.c |   13 ++++++++++++-
>>   1 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
>> index 8dee662..4b1e1a3 100644
>> --- a/xen/arch/x86/hvm/vpt.c
>> +++ b/xen/arch/x86/hvm/vpt.c
>> @@ -57,7 +57,18 @@ u64 hvm_get_guest_time(struct vcpu *v)
>>     void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
>>   {
>> -    v->arch.hvm_vcpu.stime_offset += guest_time - 
>> hvm_get_guest_time(v);
>> +    u64 offset = guest_time - hvm_get_guest_time(v);
>> +
>> +    if ( offset ) {
>> +        v->arch.hvm_vcpu.stime_offset += offset;
>> +        /*
>> +         * If hvm_vcpu.stime_offset is updated make sure to
>> +         * also update vcpu time, since this value is used to
>> +         * calculate the TSC.
>> +         */
>> +        if ( v == current )
>> +            update_vcpu_system_time(v);
>> +    }
>>   }
>>     static int pt_irq_vector(struct periodic_time *pt, enum 
>> hvm_intsrc src)
>
> I have tested this patch and I have not been able to reproduce the 
> guest stuck-clock problem we were seeing before (see *HVM Migration of 
> domU on Qemu-upstream DM causes stuck system clock with ACPI* for 
> details of the problem).
> Note: this was tested without the temporary fix to our problem ( which 
> is specifying tsc_mode='native_paravirt').
Great, thanks Diana.

  -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 05/06/13 14:06, George Dunlap wrote:
> On 05/06/13 13:02, Diana Crisan wrote:
>> On 04/06/13 16:32, Roger Pau Monne wrote:
>>> When using a vtsc, hvm_set_guest_time changes
hvm_vcpu.stime_offset,
>>> which is used in the vcpu time structure to calculate the
>>> tsc_timestamp, so after updating stime_offset we need to propagate
the
>>> change to vcpu_time in order for the guest to get the right time if
>>> using the PV clock.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Cc: Keir Fraser <keir@xen.org>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Tested-By: Diana Crisan  <dcrisan@flexiant.com>
>>> ---
>>> Changes since v2:
>>>   * Check for v == current instead of v->is_running to know if
the vcpu
>>>     is running.
>>> Changes since v1:
>>>   * Perform the call to update_vcpu_system_time in
hvm_set_guest_time
>>>     if the offset has changed and the vCPU is running.
>>> ---
>>>   xen/arch/x86/hvm/vpt.c |   13 ++++++++++++-
>>>   1 files changed, 12 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
>>> index 8dee662..4b1e1a3 100644
>>> --- a/xen/arch/x86/hvm/vpt.c
>>> +++ b/xen/arch/x86/hvm/vpt.c
>>> @@ -57,7 +57,18 @@ u64 hvm_get_guest_time(struct vcpu *v)
>>>     void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
>>>   {
>>> -    v->arch.hvm_vcpu.stime_offset += guest_time - 
>>> hvm_get_guest_time(v);
>>> +    u64 offset = guest_time - hvm_get_guest_time(v);
>>> +
>>> +    if ( offset ) {
>>> +        v->arch.hvm_vcpu.stime_offset += offset;
>>> +        /*
>>> +         * If hvm_vcpu.stime_offset is updated make sure to
>>> +         * also update vcpu time, since this value is used to
>>> +         * calculate the TSC.
>>> +         */
>>> +        if ( v == current )
>>> +            update_vcpu_system_time(v);
>>> +    }
>>>   }
>>>     static int pt_irq_vector(struct periodic_time *pt, enum 
>>> hvm_intsrc src)
>>
>> I have tested this patch and I have not been able to reproduce the 
>> guest stuck-clock problem we were seeing before (see *HVM Migration 
>> of domU on Qemu-upstream DM causes stuck system clock with ACPI* for 
>> details of the problem).
>> Note: this was tested without the temporary fix to our problem ( 
>> which is specifying tsc_mode='native_paravirt').
>
> Great, thanks Diana.
>
No problem.

On a side note, the patch applied cleanly to 4.2.2 and it works as 
expected too.
>  -George
>
--
Diana

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

George, Roger,

--On 5 June 2013 14:06:09 +0100 George Dunlap
<george.dunlap@eu.citrix.com>
wrote:
>> I have tested this patch and I have not been able to reproduce the
>> guest stuck-clock problem we were seeing before (see *HVM Migration of
>> domU on Qemu-upstream DM causes stuck system clock with ACPI* for
>> details of the problem).
>> Note: this was tested without the temporary fix to our problem ( which
>> is specifying tsc_mode=''native_paravirt'').
>
> Great, thanks Diana.
I see this has made master: commit 32c864a35ece2c24a336d183869a546798a4b241.

Do you plan to backport this to 4.2? Diana has verified it applies and
fixes the same issue there.

-- 
Alex Bligh

>>> On 06.06.13 at 08:59, Alex Bligh <alex@alex.org.uk> wrote:
> --On 5 June 2013 14:06:09 +0100 George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> 
>>> I have tested this patch and I have not been able to reproduce the
>>> guest stuck-clock problem we were seeing before (see *HVM Migration
of
>>> domU on Qemu-upstream DM causes stuck system clock with ACPI* for
>>> details of the problem).
>>> Note: this was tested without the temporary fix to our problem (
which
>>> is specifying tsc_mode=''native_paravirt'').
>>
>> Great, thanks Diana.
> 
> I see this has made master: commit
32c864a35ece2c24a336d183869a546798a4b241.
> 
> Do you plan to backport this to 4.2? Diana has verified it applies and
> fixes the same issue there.
Yes, I have this on my list of pending backports, but currently there''s
no rush.

Jan

On 06/06/13 08:20, Jan Beulich wrote:
>>>> On 06.06.13 at 08:59, Alex Bligh <alex@alex.org.uk>
wrote:
>> --On 5 June 2013 14:06:09 +0100 George Dunlap
<george.dunlap@eu.citrix.com> wrote:
>>
>>>> I have tested this patch and I have not been able to reproduce
the
>>>> guest stuck-clock problem we were seeing before (see *HVM
Migration of
>>>> domU on Qemu-upstream DM causes stuck system clock with ACPI*
for
>>>> details of the problem).
>>>> Note: this was tested without the temporary fix to our problem
( which
>>>> is specifying tsc_mode=''native_paravirt'').
>>> Great, thanks Diana.
>> I see this has made master: commit
32c864a35ece2c24a336d183869a546798a4b241.
>>
>> Do you plan to backport this to 4.2? Diana has verified it applies and
>> fixes the same issue there.
> Yes, I have this on my list of pending backports, but currently
there''s
> no rush.
...but thank you for chasing it to make sure that it was on somebody''s 
list and didn''t get forgotten. :-)

  -George

Jan,

--On 6 June 2013 08:20:17 +0100 Jan Beulich <JBeulich@suse.com> wrote:
> Yes, I have this on my list of pending backports, but currently
there''s
> no rush.
Thanks, and indeed.

-- 
Alex Bligh