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

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

Glauber de Oliveira Costa wrote:

> +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;
> +	struct xen_shared_info *shared_kaddr;
> +
> +	if ((!vcpu->shared_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);
> +	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;
> +
> +	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
> +	 */
> +	wc_args[0] = 2;
> +	vcpu->hv_clock.version = 2;
> +
> +	preempt_disable();
> +
> +	shared_kaddr = kmap_atomic(vcpu->shared_page, KM_USER0);
> +
> +	/*
> +	 * We could write everything at once, but it can break future
> +	 * implementations. We're just a tiny and lonely clock, so let's
> +	 * write only what matters here
> +	 */
> +	memcpy(&shared_kaddr->wc_version, wc_args, sizeof(wc_args));
>   


We want to avoid updating wall clock all the time.  As far as I 
understand, wall clock is just a base which doesn't change.  To get the 
real wall clock, you read the shared_info wall clock and add the current 
system time.  This means that you avoid writing to a shared global  
(which is expensive in cache lines).  The shared_info wall clock is only 
updated if the host clocked is moved (other than in the way you expect 
it to).

Also, when you write to the shared clock, you must respect the protocol 
since it can be read concurrently:

- increment the version
- smp_wmb()
- copy the goodies
- smp_wmb()
- increment the version again

[I think this is the protocol, but better read the sources to double-check]

>  		}
> diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
> index d6db0de..9a66b90 100644
> --- a/include/asm-x86/kvm_host.h
> +++ b/include/asm-x86/kvm_host.h
> @@ -261,6 +261,10 @@ struct kvm_vcpu_arch {
>  	/* emulate context */
>  
>  	struct x86_emulate_ctxt emulate_ctxt;
> +
> +	struct xen_vcpu_time_info hv_clock;
> +	gpa_t shared_info;
> +	struct page *shared_page;
>  };
>   

shared_{info,page} is too generic a name for just a clock.

>  
> +/* xen binary-compatible interfaces. See xen headers for details */
> +struct xen_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;
> +	int8_t   pad1[3];
> +};
> +
> +struct xen_vcpu_info {
> +	uint8_t  pad[32];
> +	struct xen_vcpu_time_info time;
> +};
>   

Please drop xen_vcpu_info...

> +
> +#define XEN_MAX_VIRT_CPUS	32
> +
> +struct xen_shared_info {
> +	struct xen_vcpu_info vcpu_info[XEN_MAX_VIRT_CPUS];
> +
> +	unsigned long evt[2];
> +
> +	uint32_t wc_version;      /* Version counter: see vcpu_time_info_t. */
> +	uint32_t wc_sec;          /* Secs  00:00:00 UTC, Jan 1, 1970.  */
> +	uint32_t wc_nsec;         /* Nsecs 00:00:00 UTC, Jan 1, 1970.  */
> +
> +	unsigned long pad[12];
> +};
>   

... and everything non-time-related in here.  Yes, in means we need two 
msrs (for wall clock and system time), but it also means we don't impose 
any layout upon the guest, and do not (for example) restrict the number 
of vcpus.  We could easily put the vcpu clock in a per_cpu() area.


-- 
error compiling committee.c: too many arguments to function


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