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

[Glauber C. <gc...@re...>]

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(&current->mm->mmap_sem);
>> +    kvm_write_guest(v->kvm, wall_clock, &version, sizeof(version));
>> +    up_write(&current->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(&current->mm->mmap_sem);
>> +    kvm_write_guest(v->kvm, wall_clock, &wc, sizeof(wc));
>> +    up_write(&current->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.