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

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

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.
>
> @@ -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_PARAVIRT_CLOCK,
>  };
>   

Let us be good citizens and put the msr into our private MSR_KVM_* 
namespace.

>  
>  
> +#define WC_OFFSET offsetof(struct shared_info, wc_version)
> +
> +static void kvm_write_guest_time(struct kvm_vcpu *v)
> +{
> +	struct timespec ts, wc_ts;
> +	int wc_args[3]; /* version, wc_sec, wc_nsec */
> +	unsigned long flags;
> +	struct kvm_vcpu_arch *vcpu = &v->arch;
> +
> +	if (!vcpu->shared_info)
> +		return;
> +
> +	/* Make the update of both version numbers visible to guest */
> +	wc_args[0] = ++vcpu->wc_version;
> +	kvm_write_guest(v->kvm, vcpu->shared_info + WC_OFFSET, wc_args,
> +			sizeof(wc_args));
> +	vcpu->hv_clock.version++;
> +	kvm_write_guest(v->kvm, vcpu->clock_addr, &vcpu->hv_clock,
> +			sizeof(vcpu->hv_clock));
> +
> +	/* 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);
> +	wc_ts = current_kernel_time();
> +	ktime_get_ts(&ts);
> +	local_irq_restore(flags);
> +
> +	/* With all the info we got, fill in the values */
> +	wc_args[1] = wc_ts.tv_sec;
> +	wc_args[2] = wc_ts.tv_nsec;
> +	wc_args[0] = ++vcpu->wc_version;
> +
> +	vcpu->hv_clock.system_time = ts.tv_nsec +
> +				     (NSEC_PER_SEC * (u64)ts.tv_sec);
> +	vcpu->hv_clock.version++;
> +
> +	/* And finally, let the guest see them */
> +	kvm_write_guest(v->kvm, vcpu->shared_info + WC_OFFSET, wc_args,
> +			sizeof(wc_args));
> +	kvm_write_guest(v->kvm, vcpu->clock_addr, &vcpu->hv_clock,
> +			sizeof(vcpu->hv_clock));
> +}
> +
>   

Why write things twice?  This is per-vcpu, and the guest cannot see 
anything between the two writes.

Also, kvm_write_guest() needs mmap_sem for read.  Suggest you grab the 
page using gfn_to_page() and kmap_atomic() it instead.

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


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace