Roger Pau Monne
2013-Jun-04 09:10 UTC
[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
Jan Beulich
2013-Jun-04 09:24 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
>>> 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
Roger Pau Monné
2013-Jun-04 09:46 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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.
George Dunlap
2013-Jun-04 09:47 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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
George Dunlap
2013-Jun-04 09:56 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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
Roger Pau Monné
2013-Jun-04 09:58 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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 runningThis 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.
George Dunlap
2013-Jun-04 10:00 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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
Roger Pau Monné
2013-Jun-04 10:12 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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
Alex Bligh
2013-Jun-04 10:24 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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
George Dunlap
2013-Jun-04 10:28 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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
Jan Beulich
2013-Jun-04 10:41 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
>>> 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
Roger Pau Monné
2013-Jun-04 11:15 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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 runningI''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.
Jan Beulich
2013-Jun-04 11:45 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
>>> 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
Roger Pau Monné
2013-Jun-04 12:48 UTC
Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time
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
Apparently Analagous Threads
- [hybrid]: hang in update_wall_time
- [PATCH] x86/hvm: fix corrupt ACPI PM-Timer during live migration
- [PATCH RFC v13 00/20] Introduce PVH domU support
- vmx_update_guest_cr() losing EXCEPTION_BITMAP setting
- [PATCH 10/14] Nested Virtualization: svm specific implementation