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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.