Xen devel - Jun 2013 - [PATCH v2] 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 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..d37d214 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->is_running )
+            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 06/04/2013 01:49 PM, 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>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

Diana / Alex, want to give this one a spin?

  -George


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

>>> On 04.06.13 at 14:49, 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 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..d37d214 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->is_running )
I'm afraid this is at least a latent bug: "v->is_running" gets
set
before "current" gets switched, and in the intermediate time
__hvm_copy() would copy to the wrong VM (or crash, if "current"
isn't a HVM vCPU).
> +            update_vcpu_system_time(v);
> +    }
Right now, both call paths

{svm,vmx}_intr_assist() -> pt_intr_post() -> hvm_set_guest_time()

and

{svm,vmx}_do_resume() -> hvm_do_resume() -> pt_restore_timer()
-> pt_thaw_time() -> hvm_set_guest_time()

appear to be safe, but it doesn't feel well to rely on this. I'd
therefore prefer to switch the condition above to "v == current"
or, considering that there are no other call paths (easier to prove
if hvm_set_guest_time() was made static, perhaps simply
ASSERT(v == current) before the call.

Jan

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

--On 4 June 2013 14:09:31 +0100 George Dunlap
<george.dunlap@eu.citrix.com>
wrote:
> On 06/04/2013 01:49 PM, 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>
>
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> Diana / Alex, want to give this one a spin?
We will do. If it works we will test a 4.2.2 backport too.

-- 
Alex Bligh

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

On Tue, Jun 4, 2013 at 1:49 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.
Just for public record:

I looked into the other elements that go into the system_time update
calculation, to make sure we weren''t missing anything else that might
need updating.  They are:
 1. d->arch.hvm_domain.pl_time.stime_offset
 2. d->arch.vtsc_offset (if the VM is not an hvm_domain)
 3. d->arch.ns_to_vtsc

#1 is set once in xen/arch/x86/hvm/vpt.c:hvm_init_guest_time() and
never touched again.

#2 and #3 are set in xen/arch/x86/time.c:tsc_set_info, with a comment
saying that the info needs to be set and stabilized before the first
guest RDTSC; so they *should* never be updated while the guest is
running.  It  might be nice to have a check to make sure of that, but
it''s just a bit more work than I feel like doing at the moment
(probably checking all the individual vcpus in a domain).

 -George

On Tue, Jun 4, 2013 at 2:47 PM, Jan Beulich <JBeulich@suse.com>
wrote:
>>>> On 04.06.13 at 14:49, 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 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..d37d214 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->is_running )
>
> I''m afraid this is at least a latent bug:
"v->is_running" gets set
> before "current" gets switched, and in the intermediate time
> __hvm_copy() would copy to the wrong VM (or crash, if "current"
> isn''t a HVM vCPU).
Good catch.

Should we add an ASSERT() to update_vcpu_system_time() that "v ==
current"?

 -George

>>> On 04.06.13 at 18:27, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Tue, Jun 4, 2013 at 2:47 PM, Jan Beulich <JBeulich@suse.com>
wrote:
>>>>> On 04.06.13 at 14:49, 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 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..d37d214 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->is_running )
>>
>> I'm afraid this is at least a latent bug:
"v->is_running" gets set
>> before "current" gets switched, and in the intermediate time
>> __hvm_copy() would copy to the wrong VM (or crash, if
"current"
>> isn't a HVM vCPU).
> 
> Good catch.
> 
> Should we add an ASSERT() to update_vcpu_system_time() that "v ==
current"?
Yes, probably, now that we have a caller of it that wasn't at the
first glance obviously meeting that requirement.

Jan

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

On 04/06/13 14:59, Alex Bligh wrote:
>
>
> --On 4 June 2013 14:09:31 +0100 George Dunlap 
> <george.dunlap@eu.citrix.com> wrote:
>
>> On 06/04/2013 01:49 PM, 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>
>>
>> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
>>
>> Diana / Alex, want to give this one a spin?
>
> We will do. If it works we will test a 4.2.2 backport too.
>
I have tested patch-v2 with xen4.3 and it did not get a clock stuck yet 
across mutiple migrations between hosts (i.e. not localhost). I have 
patched in v3 and will do some further stress testing tomorrow. Once it 
works I will also test a backport to 4.2.2 and also give an update.

Thanks,
Diana


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