Xen devel - Jun 2013 - [PATCH] x86/vtsc: update vcpu_time after 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.

This was not done correctly, since in context_switch
update_vcpu_system_time was called before vmx_do_resume, which caused
the vcpu_info time structure to be updated with the wrong values. This
patch fixes this by calling update_vcpu_system_time after the call to
hvm_set_guest_time has happened.

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>
---
 xen/arch/x86/hvm/hvm.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a962ce2..e257dcf 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -343,6 +343,12 @@ void hvm_do_resume(struct vcpu *v)
     ioreq_t *p;
 
     pt_restore_timer(v);
+    /*
+     * Update vcpu_info, since the call to pt_restore_timer can change
+     * the value in v->arch.hvm_vcpu.stime_offset that is used
+     * to calculate the TSC in vcpu_info->time.
+     */
+    update_vcpu_system_time(v);
 
     check_wakeup_from_wait();
 
-- 
1.7.7.5 (Apple Git-26)


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

>>> On 04.06.13 at 11:10, 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.
> 
> This was not done correctly, since in context_switch
> update_vcpu_system_time was called before vmx_do_resume, which caused
> the vcpu_info time structure to be updated with the wrong values. This
> patch fixes this by calling update_vcpu_system_time after the call to
> hvm_set_guest_time has happened.
So at the first glance I was thinking this would be fixing a regression
from commit ae5092f420e87a4a6b541bf581378c8cc0ee3a99, but
after a closer look it looks like this was done even earlier before.
Can you confirm this (not the least because this would have
implications on the need to backport this change)?
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -343,6 +343,12 @@ void hvm_do_resume(struct vcpu *v)
>      ioreq_t *p;
>  
>      pt_restore_timer(v);
> +    /*
> +     * Update vcpu_info, since the call to pt_restore_timer can change
> +     * the value in v->arch.hvm_vcpu.stime_offset that is used
> +     * to calculate the TSC in vcpu_info->time.
> +     */
> +    update_vcpu_system_time(v);
Adding it here means, unless I''m mistaken, the one in
context_switch() is now pointless, so I''d encourage you to
gate that one on !is_hvm_vcpu() (with a comment saying that
in this case it''s being done in hvm_do_resume()).

Jan

On 04/06/13 11:24, Jan Beulich wrote:
>>>> On 04.06.13 at 11:10, 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.
>>
>> This was not done correctly, since in context_switch
>> update_vcpu_system_time was called before vmx_do_resume, which caused
>> the vcpu_info time structure to be updated with the wrong values. This
>> patch fixes this by calling update_vcpu_system_time after the call to
>> hvm_set_guest_time has happened.
> 
> So at the first glance I was thinking this would be fixing a regression
> from commit ae5092f420e87a4a6b541bf581378c8cc0ee3a99, but
> after a closer look it looks like this was done even earlier before.
> Can you confirm this (not the least because this would have
> implications on the need to backport this change)?
I''ve took a look at the commit, and I don''t think it
introduced a
regression, a call to update_vcpu_system_time was removed, but this call
was also made before calling context_switch, which wouldn''t fix the
problem at hand. This should be backported to all the versions that
expose the XENFEAT_hvm_safe_pvclock feature.
> 
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -343,6 +343,12 @@ void hvm_do_resume(struct vcpu *v)
>>      ioreq_t *p;
>>  
>>      pt_restore_timer(v);
>> +    /*
>> +     * Update vcpu_info, since the call to pt_restore_timer can change
>> +     * the value in v->arch.hvm_vcpu.stime_offset that is used
>> +     * to calculate the TSC in vcpu_info->time.
>> +     */
>> +    update_vcpu_system_time(v);
> 
> Adding it here means, unless I''m mistaken, the one in
> context_switch() is now pointless, so I''d encourage you to
> gate that one on !is_hvm_vcpu() (with a comment saying that
> in this case it''s being done in hvm_do_resume()).
Yes, the call in context_switch is now superseded by the one in
hvm_do_resume for the HVM case. I will change it and resend the patch,
thanks for the review.

On 06/04/2013 10:10 AM, 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.
>
> This was not done correctly, since in context_switch
> update_vcpu_system_time was called before vmx_do_resume, which caused
> the vcpu_info time structure to be updated with the wrong values. This
> patch fixes this by calling update_vcpu_system_time after the call to
> hvm_set_guest_time has happened.
Would it make more sense to actually do this in hvm_set_guest_time() 
instead, so that this window where the vcpu system_time is closed for 
all callers, not just hvm_do_resume?

You could gate calling update_vcpu_system_time on:
1. Whether stime_offset actually changed
2. Whether v is currently running

  -George

On 06/04/2013 10:46 AM, Roger Pau Monné wrote:
> On 04/06/13 11:24, Jan Beulich wrote:
>>>>> On 04.06.13 at 11:10, 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.
>>>
>>> This was not done correctly, since in context_switch
>>> update_vcpu_system_time was called before vmx_do_resume, which
caused
>>> the vcpu_info time structure to be updated with the wrong values.
This
>>> patch fixes this by calling update_vcpu_system_time after the call
to
>>> hvm_set_guest_time has happened.
>>
>> So at the first glance I was thinking this would be fixing a regression
>> from commit ae5092f420e87a4a6b541bf581378c8cc0ee3a99, but
>> after a closer look it looks like this was done even earlier before.
>> Can you confirm this (not the least because this would have
>> implications on the need to backport this change)?
>
> I''ve took a look at the commit, and I don''t think it
introduced a
> regression, a call to update_vcpu_system_time was removed, but this call
> was also made before calling context_switch, which wouldn''t fix
the
> problem at hand. This should be backported to all the versions that
> expose the XENFEAT_hvm_safe_pvclock feature.
>
>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -343,6 +343,12 @@ void hvm_do_resume(struct vcpu *v)
>>>       ioreq_t *p;
>>>
>>>       pt_restore_timer(v);
>>> +    /*
>>> +     * Update vcpu_info, since the call to pt_restore_timer can
change
>>> +     * the value in v->arch.hvm_vcpu.stime_offset that is used
>>> +     * to calculate the TSC in vcpu_info->time.
>>> +     */
>>> +    update_vcpu_system_time(v);
>>
>> Adding it here means, unless I''m mistaken, the one in
>> context_switch() is now pointless, so I''d encourage you to
>> gate that one on !is_hvm_vcpu() (with a comment saying that
>> in this case it''s being done in hvm_do_resume()).
>
> Yes, the call in context_switch is now superseded by the one in
> hvm_do_resume for the HVM case. I will change it and resend the patch,
> thanks for the review.
I agree that it''s worth trying to avoid calling the same function
twice;
but since the common case is for pt_restore_timer() to not actually make 
any substantial changes to hvm_vcpu.stime_offset, I think it would be 
better if we had the basic "update the system time" call shared
between
HVM and PV codepaths, and have the uncommon case where 
hvm_vcpu.stime_offset does change just call it twice.

Updating it at hvm_set_guest_time() will also make sure that there
wont''
be any problems between the update at pt_intr_post() and the next time 
the vcpu is scheduled.

  -George

On 04/06/13 11:47, George Dunlap wrote:
> On 06/04/2013 10:10 AM, 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.
>>
>> This was not done correctly, since in context_switch
>> update_vcpu_system_time was called before vmx_do_resume, which caused
>> the vcpu_info time structure to be updated with the wrong values. This
>> patch fixes this by calling update_vcpu_system_time after the call to
>> hvm_set_guest_time has happened.
> 
> Would it make more sense to actually do this in hvm_set_guest_time()
> instead, so that this window where the vcpu system_time is closed for
> all callers, not just hvm_do_resume?
> 
> You could gate calling update_vcpu_system_time on:
> 1. Whether stime_offset actually changed
> 2. Whether v is currently running
This was my first approach, but I thought it was better to fix the
actual call to update_vcpu_system_time to happen in the right place
(before the context switch), rather than adding more calls to
update_vcpu_system_time all over the place.

If later we decide for whatever reason to add more offsets to the vtsc,
we will also have to add more calls to update_vcpu_system_time in every
place that we modify those offsets, which doesn''t seem right.

On 06/04/2013 10:58 AM, Roger Pau Monné wrote:
> On 04/06/13 11:47, George Dunlap wrote:
>> On 06/04/2013 10:10 AM, 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.
>>>
>>> This was not done correctly, since in context_switch
>>> update_vcpu_system_time was called before vmx_do_resume, which
caused
>>> the vcpu_info time structure to be updated with the wrong values.
This
>>> patch fixes this by calling update_vcpu_system_time after the call
to
>>> hvm_set_guest_time has happened.
>>
>> Would it make more sense to actually do this in hvm_set_guest_time()
>> instead, so that this window where the vcpu system_time is closed for
>> all callers, not just hvm_do_resume?
>>
>> You could gate calling update_vcpu_system_time on:
>> 1. Whether stime_offset actually changed
>> 2. Whether v is currently running
>
> This was my first approach, but I thought it was better to fix the
> actual call to update_vcpu_system_time to happen in the right place
> (before the context switch), rather than adding more calls to
> update_vcpu_system_time all over the place.
>
> If later we decide for whatever reason to add more offsets to the vtsc,
> we will also have to add more calls to update_vcpu_system_time in every
> place that we modify those offsets, which doesn't seem right.
But you will have to add those anyway if the guest is running. 
Otherwise you'll have this same window, where vcpu_info.system_time is 
out of sync with hvm_vcpu.stime_offset, and this same kind of bug can 
happen.

  -George


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

On 04/06/13 12:00, George Dunlap wrote:
> On 06/04/2013 10:58 AM, Roger Pau Monné wrote:
>> On 04/06/13 11:47, George Dunlap wrote:
>>> On 06/04/2013 10:10 AM, 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.
>>>>
>>>> This was not done correctly, since in context_switch
>>>> update_vcpu_system_time was called before vmx_do_resume, which
caused
>>>> the vcpu_info time structure to be updated with the wrong
values. This
>>>> patch fixes this by calling update_vcpu_system_time after the
call to
>>>> hvm_set_guest_time has happened.
>>>
>>> Would it make more sense to actually do this in
hvm_set_guest_time()
>>> instead, so that this window where the vcpu system_time is closed
for
>>> all callers, not just hvm_do_resume?
>>>
>>> You could gate calling update_vcpu_system_time on:
>>> 1. Whether stime_offset actually changed
>>> 2. Whether v is currently running
>>
>> This was my first approach, but I thought it was better to fix the
>> actual call to update_vcpu_system_time to happen in the right place
>> (before the context switch), rather than adding more calls to
>> update_vcpu_system_time all over the place.
>>
>> If later we decide for whatever reason to add more offsets to the vtsc,
>> we will also have to add more calls to update_vcpu_system_time in every
>> place that we modify those offsets, which doesn't seem right.
> 
> But you will have to add those anyway if the guest is running. Otherwise
> you'll have this same window, where vcpu_info.system_time is out of
sync
> with hvm_vcpu.stime_offset, and this same kind of bug can happen.
This could be solved by also adding a call to update_vcpu_system_time in
pt_intr_post, but given that I'm not sure any more if it's not best to
just do the call to update_vcpu_system_time in hvm_set_guest_time and
get done with it.


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

Roger,

--On 4 June 2013 11:10:50 +0200 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.
>
> This was not done correctly, since in context_switch
> update_vcpu_system_time was called before vmx_do_resume, which caused
> the vcpu_info time structure to be updated with the wrong values. This
> patch fixes this by calling update_vcpu_system_time after the call to
> hvm_set_guest_time has happened.
	
I think this is the ''proper'' fix to the bug Diana reported
about
stuck clock - can you confirm? If so, we''re happy to test this on
4.3 and/or do try our hand at a backport to 4.2.

-- 
Alex Bligh

On 06/04/2013 11:24 AM, Alex Bligh wrote:
> Roger,
>
> --On 4 June 2013 11:10:50 +0200 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.
>>
>> This was not done correctly, since in context_switch
>> update_vcpu_system_time was called before vmx_do_resume, which caused
>> the vcpu_info time structure to be updated with the wrong values. This
>> patch fixes this by calling update_vcpu_system_time after the call to
>> hvm_set_guest_time has happened.
>
> I think this is the ''proper'' fix to the bug Diana
reported about
> stuck clock - can you confirm? If so, we''re happy to test this on
> 4.3 and/or do try our hand at a backport to 4.2.
Yes, and I was going to reply cc''ing you and Diana, but we''re
still
having a technical discussion about where the best place is to put the 
fix.  You may wish to wait until we have that resolved before testing 
the patch.

  -George

>>> On 04.06.13 at 11:56, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 06/04/2013 10:46 AM, Roger Pau Monné wrote:
>> Yes, the call in context_switch is now superseded by the one in
>> hvm_do_resume for the HVM case. I will change it and resend the patch,
>> thanks for the review.
> 
> I agree that it's worth trying to avoid calling the same function
twice;
> but since the common case is for pt_restore_timer() to not actually make 
> any substantial changes to hvm_vcpu.stime_offset, I think it would be 
> better if we had the basic "update the system time" call shared
between
> HVM and PV codepaths, and have the uncommon case where 
> hvm_vcpu.stime_offset does change just call it twice.
Makes sense to me.

Jan

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

On 04/06/13 11:47, George Dunlap wrote:
> On 06/04/2013 10:10 AM, 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.
>>
>> This was not done correctly, since in context_switch
>> update_vcpu_system_time was called before vmx_do_resume, which caused
>> the vcpu_info time structure to be updated with the wrong values. This
>> patch fixes this by calling update_vcpu_system_time after the call to
>> hvm_set_guest_time has happened.
> 
> Would it make more sense to actually do this in hvm_set_guest_time()
> instead, so that this window where the vcpu system_time is closed for
> all callers, not just hvm_do_resume?
> 
> You could gate calling update_vcpu_system_time on:
> 1. Whether stime_offset actually changed
> 2. Whether v is currently running
I''m not sure if only updating the vcpu time if the vCPU is running can
lead to a slip in the vcpu time update. As an example, if we call
hvm_set_guest_time with the vCPU not running and change the offset, and
then call it again on resume, but the offset has not changed this time
the vcpu time will not be updated. I would rather update the vcpu time
info every time there''s a change in the offset.

>>> On 04.06.13 at 13:15, Roger Pau Monné<roger.pau@citrix.com>
wrote:
> On 04/06/13 11:47, George Dunlap wrote:
>> You could gate calling update_vcpu_system_time on:
>> 1. Whether stime_offset actually changed
>> 2. Whether v is currently running
> 
> I'm not sure if only updating the vcpu time if the vCPU is running can
> lead to a slip in the vcpu time update. As an example, if we call
> hvm_set_guest_time with the vCPU not running and change the offset, and
> then call it again on resume, but the offset has not changed this time
> the vcpu time will not be updated.
But in that case the call from context_switch() would take care
of it, wouldn't it?

Jan

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

On 04/06/13 13:45, Jan Beulich wrote:
>>>> On 04.06.13 at 13:15, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>> On 04/06/13 11:47, George Dunlap wrote:
>>> You could gate calling update_vcpu_system_time on:
>>> 1. Whether stime_offset actually changed
>>> 2. Whether v is currently running
>>
>> I'm not sure if only updating the vcpu time if the vCPU is running
can
>> lead to a slip in the vcpu time update. As an example, if we call
>> hvm_set_guest_time with the vCPU not running and change the offset, and
>> then call it again on resume, but the offset has not changed this time
>> the vcpu time will not be updated.
> 
> But in that case the call from context_switch() would take care
> of it, wouldn't it?
Sure, I just got messed up with what we discussed before about gating
the call to update_vcpu_system_time in context_switch for the HVM case.
Will resend shortly.


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