From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time Date: Tue, 4 Jun 2013 10:56:41 +0100 Message-ID: <51ADB9D9.4030705@eu.citrix.com> References: <1370337050-2444-1-git-send-email-roger.pau@citrix.com> <51ADCE5602000078000DAFE1@nat28.tlf.novell.com> <51ADB759.2070805@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <51ADB759.2070805@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= Cc: Keir Fraser , Jan Beulich , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 06/04/2013 10:46 AM, Roger Pau Monn=E9 wrote: > On 04/06/13 11:24, Jan Beulich wrote: >>>>> On 04.06.13 at 11:10, 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. >> >> 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