Re: [PATCH 1/2] kvmclock - the host part.

[Avi Kivity <avi@qumranet.com>]

Glauber Costa wrote:
>>
>>> +static void kvm_write_wall_clock(struct kvm_vcpu *v, gpa_t wall_clock)
>>> +{
>>> +    int version = 1;
>>> +    struct kvm_wall_clock wc;
>>> +    unsigned long flags;
>>> +    struct timespec wc_ts;
>>> +
>>> +    local_irq_save(flags);
>>> +    kvm_get_msr(v, MSR_IA32_TIME_STAMP_COUNTER,
>>> +              &v->arch.hv_clock.tsc_timestamp);
>>>   
>>
>> Why is this needed? IIRC the wall clock is not tied to any vcpu.
>>
>> If we can remove this, the argument to the function should be kvm, 
>> not kvm_vcpu. We can remove the irq games as well.
>
> After some new thoughts, I don't agree. The time calculation in the 
> guest will be in the format wallclock + delta tsc. Everything that has 
> a tsc _is_ tied to a cpu. Although we can store the area globally, I 
> think the best semantics is to have a vcpu to always issue an msr 
> write to the area before reading it, in order to have the tsc updated.
>

No.  Guest time is (wall_clock + system_time) == (wall_clock + 
system_time_base + tsc_adjust).  The wall clock part is vcpu indepenent 
(or it would be unusable by more than one vcpus).

wall_clock is the system boot time; it only changes when the host time 
is adjusted (by ntp or user command).

>
>> For mutual exclusion, I suggest taking kvm->lock instead (for the 
>> entire function).
>
> This function is called too often. since we only need to guarantee 
> mutual exclusion in a tiny part, it seems preferable to me. Do you 
> have any extra reason for kvm->lock'ing the entire function?

Wall clock adjustment should only be called as a result of 
settimeofday() or similar.  Only vcpu time needs frequent adjustment 
(due to vcpu migration or tsc frequency change).

>
>
>>>  
>>> +/* xen binary-compatible interface. See xen headers for details */
>>> +struct kvm_vcpu_time_info {
>>> +    uint32_t version;
>>> +    uint32_t pad0;
>>> +    uint64_t tsc_timestamp;
>>> +    uint64_t system_time;
>>> +    uint32_t tsc_to_system_mul;
>>> +    int8_t   tsc_shift;
>>> +}; /* 32 bytes */
>>> +
>>> +struct kvm_wall_clock {
>>> +    uint32_t wc_version;
>>> +    uint32_t wc_sec;
>>> +    uint32_t wc_nsec;
>>> +};
>>> +
>>>   
>>
>> These structures are dangerously sized. __Suggest__ 
>> __attribute__((__packed__)). (or some padding at the end of 
>> kvm_vcpu_time_info.
>
> They are sized so to have same size as xen's. If it concerns you,
> packed should be better.

Okay, so let's pack kvm_wall_clock to avoid surprises.  The Xen sources 
I have (v2.6.25-rc1:include/xen/interface/xen.h) do have three bytes 
padding in vcpu_time_info to make it 32 bytes.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/