Avi Kivity wrote:
> Glauber de Oliveira Costa wrote:
>> This is the host part of kvm clocksource implementation. As it does
>> not include clockevents, it is a fairly simple implementation. We
>> only have to register a per-vcpu area, and start writting to it
>> periodically.
>>
>> The area is binary compatible with xen, as we use the same shadow_info
>> structure.
>>
>>
>
>> +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.
>> + wc_ts = current_kernel_time();
>> + local_irq_restore(flags);
>> +
>> + down_write(¤t->mm->mmap_sem);
>> + kvm_write_guest(v->kvm, wall_clock, &version, sizeof(version));
>> + up_write(¤t->mm->mmap_sem);
>>
>
> Why down_write? accidentally or on purpose?
accidentally. Marcelo has pointed it out to me, and I do have a version
without it now.
> 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?
>> +
>> + /* With all the info we got, fill in the values */
>> + wc.wc_sec = wc_ts.tv_sec;
>> + wc.wc_nsec = wc_ts.tv_nsec;
>> + wc.wc_version = ++version;
>> +
>> + down_write(¤t->mm->mmap_sem);
>> + kvm_write_guest(v->kvm, wall_clock, &wc, sizeof(wc));
>> + up_write(¤t->mm->mmap_sem);
>>
>
> Should be in three steps: write version, write data, write version.
> kvm_write_guest doesn't guarantee any order. It may fail as well, and we
> need to handle that.
Ok, I see. This is fundamentally different from the system time case,
because multiple cpus can
be running over the same area.
>>
>> +/* 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.
>> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
>> index 4de4fd2..78ce53f 100644
>> --- a/include/linux/kvm.h
>> +++ b/include/linux/kvm.h
>> @@ -232,6 +232,7 @@ #define KVM_CAP_USER_MEMORY 3
>> #define KVM_CAP_SET_TSS_ADDR 4
>> #define KVM_CAP_EXT_CPUID 5
>> #define KVM_CAP_VAPIC 6
>> +#define KVM_CAP_CLOCKSOURCE 7
>>
>>
>
> Please refresh against kvm.git, this has changed a bit.
ok.
-------------------------------------------------------------------------
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
