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

[Avi Kivity <avi@qumranet.com>]

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.

> +	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?

For mutual exclusion, I suggest taking kvm->lock instead (for 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.

>  
> +/* 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.

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

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


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