Re: [Xen-devel] [PATCH] x86/vtsc: update vcpu_time after hvm_set_guest_time

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

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@xxxxxxxxxx> 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

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