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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.06.13 at 18:27, George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
> 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"?

Yes, probably, now that we have a caller of it that wasn't at the
first glance obviously meeting that requirement.

Jan

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