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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Tue, Jun 4, 2013 at 2:47 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 04.06.13 at 14:49, 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.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Cc: Keir Fraser <keir@xxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>> ---
>> Changes since v1:
>>  * Perform the call to update_vcpu_system_time in hvm_set_guest_time
>>    if the offset has changed and the vCPU is running.
>> ---
>>  xen/arch/x86/hvm/vpt.c |   13 ++++++++++++-
>>  1 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
>> index 8dee662..d37d214 100644
>> --- a/xen/arch/x86/hvm/vpt.c
>> +++ b/xen/arch/x86/hvm/vpt.c
>> @@ -57,7 +57,18 @@ u64 hvm_get_guest_time(struct vcpu *v)
>>
>>  void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
>>  {
>> -    v->arch.hvm_vcpu.stime_offset += guest_time - hvm_get_guest_time(v);
>> +    u64 offset = guest_time - hvm_get_guest_time(v);
>> +
>> +    if ( offset ) {
>> +        v->arch.hvm_vcpu.stime_offset += offset;
>> +        /*
>> +         * If hvm_vcpu.stime_offset is updated make sure to
>> +         * also update vcpu time, since this value is used to
>> +         * calculate the TSC.
>> +         */
>> +        if ( v->is_running )
>
> I'm afraid this is at least a latent bug: "v->is_running" gets set
> before "current" gets switched, and in the intermediate time
> __hvm_copy() would copy to the wrong VM (or crash, if "current"
> isn't a HVM vCPU).

Good catch.

Should we add an ASSERT() to update_vcpu_system_time() that "v == current"?

 -George

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