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/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
