Re: [Xen-devel] Re: [PATCH 3/5] x86/pvclock: add vsyscall implementation

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 10/07/09 13:09, Avi Kivity wrote:
> On 10/07/2009 09:29 PM, Jeremy Fitzhardinge wrote:
>>
>>>    I'm a bit worried about the kernel playing with the hypervisor's
>>> version field.
>>>      
>> For Xen I explicitly made it not a problem by adding the notion of a
>> secondary pvclock_vcpu_time_info structure which is updated by copying,
>> aside from the version number which is updated as-is.
>>    
>
> When do you copy?
>
> I'd rather have a single copy for guest and host.

When Xen updates the parameters normally.  The interface never really
needed to share the memory between hypervisor and guest, and I think
avoiding it is a bit more robust.

But for KVM, you already use the MSR to place the pvclock_vcpu_time_info
structure, so you could just place it in the page and use the same
memory for kernel and usermode.

> If the hypervisor does a pvclock->version = somethingelse->version++
> then the guest may get confused.  But I understand you have a
> guest-private ->version?

The guest should never get confused by the version being changed by the
hypervisor.  It's already part of the ABI.  Or did you mean something else?

I'm not sure what you mean by "guest-private version"; the versions are
always guest-private:  te version is part of the pvclock structure,
which is per-vcpu, which is private to each guest.   The guest nevern
maintains a separate long-term copy of the structure, only a transient
snapshot while computing time from the tsc (that's the current pvclock.c
code).

> No need to read them atomically.
>
> cpu1 = vgetcpu()
> hver1 = pvclock[cpu1].hver
> kver1 = pvclock[cpu1].kver
> tsc = rdtsc
> /* multipication magic with pvclock[cpu1]*/
> cpu2 = vgetcpu()
> hver2 = pvclock[cpu2].hver
> kver2 = pvclock[cpu2].kver
> valid = cpu1 == cpu2 && hver1 == hver2 && kver1 == kver2

I don't think that's necessary, but I can certainly live with it if it
makes you happier.

>>> It's sufficient to increment a version counter on thread migration, no
>>> need to do it on context switch.
>>>
>>>      
>> That's true; switch_out is a pessimistic approximation of that.  But is
>> there a convenient hook to test for migration?
>>    
>
> I'd guess not but it's probably easy to add one in the migration thread.

Looking at that now.

    J