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

[George Dunlap <george.dunlap@eu.citrix.com>]

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