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

[Glauber de Oliveira Costa <gcosta-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Anthony Liguori 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.
>>
>> Signed-off-by: Glauber de Oliveira Costa <gcosta-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/x86/kvm/x86.c         |   98 
>> +++++++++++++++++++++++++++++++++++++++++++-
>>  include/asm-x86/kvm_host.h |    6 +++
>>  include/asm-x86/kvm_para.h |   24 +++++++++++
>>  include/linux/kvm.h        |    1 +
>>  4 files changed, 128 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 8a90403..fd69aa1 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -19,6 +19,7 @@
>>  #include "irq.h"
>>  #include "mmu.h"
>>  
>> +#include <linux/clocksource.h>
>>  #include <linux/kvm.h>
>>  #include <linux/fs.h>
>>  #include <linux/vmalloc.h>
>> @@ -412,7 +413,7 @@ static u32 msrs_to_save[] = {
>>  #ifdef CONFIG_X86_64
>>      MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
>>  #endif
>> -    MSR_IA32_TIME_STAMP_COUNTER,
>> +    MSR_IA32_TIME_STAMP_COUNTER, MSR_KVM_SYSTEM_TIME,
>>  };
>>  
>>  static unsigned num_msrs_to_save;
>> @@ -467,6 +468,73 @@ static int do_set_msr(struct kvm_vcpu *vcpu, 
>> unsigned index, u64 *data)
>>      return kvm_set_msr(vcpu, index, *data);
>>  }
>>  
>> +static void kvm_write_wall_clock(struct kvm_vcpu *v, gpa_t wall_clock)
>> +{
>> +    int version = 1;
>> +    struct 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);
>> +    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);
>> +
>> +    /* 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);
>>   
> 
> Can we get a comment explaining why we only write the version field and 
> then immediately increment the version and write the whole struct?  It's 
> not at all obvious why the first write is needed to me.
If the comment is the only pending thing, can we add the comment in a 
later commit?

>> +}
>> +static void kvm_write_guest_time(struct kvm_vcpu *v)
>> +{
>> +    struct timespec ts;
>> +    unsigned long flags;
>> +    struct kvm_vcpu_arch *vcpu = &v->arch;
>> +    void *shared_kaddr;
>> +
>> +    if ((!vcpu->time_page))
>> +        return;
>> +
>> +    /* Keep irq disabled to prevent changes to the clock */
>> +    local_irq_save(flags);
>> +    kvm_get_msr(v, MSR_IA32_TIME_STAMP_COUNTER,
>> +              &vcpu->hv_clock.tsc_timestamp);
>> +    ktime_get_ts(&ts);
>> +    local_irq_restore(flags);
>> +
>> +    /* With all the info we got, fill in the values */
>> +
>> +    vcpu->hv_clock.system_time = ts.tv_nsec +
>> +                     (NSEC_PER_SEC * (u64)ts.tv_sec);
>> +    /*
>> +     * The interface expects us to write an even number signaling 
>> that the
>> +     * update is finished. Since the guest won't see the intermediate 
>> states,
>> +     * we just write "2" at the end
>> +     */
>> +    vcpu->hv_clock.version = 2;
>> +
>> +    preempt_disable();
>> +
>> +    shared_kaddr = kmap_atomic(vcpu->time_page, KM_USER0);
>> +
>> +    memcpy(shared_kaddr + vcpu->time_offset, &vcpu->hv_clock,
>> +        sizeof(vcpu->hv_clock));
>> +
>> +    kunmap_atomic(shared_kaddr, KM_USER0);
>>   
> 
> Instead of doing a kmap/memcpy, I think it would be better to store the 
> GPA of the time page and do a kvm_write_guest().  Otherwise, you're 
> pinning this page in memory.
this functions end up being called from various contexts. Some with the 
mmap_sem held, some uncontended. kvm_write_guest needs it held, so it 
would turn the code into a big spaguetti. Using the kmap was avi's 
suggestion to get around it, which I personally liked: we only grab the 
semaphore when the msr is registered.


-------------------------------------------------------------------------
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/