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(¤t->mm->mmap_sem);
> + kvm_write_guest(v->kvm, wall_clock, &version, sizeof(version));
> + up_write(¤t->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(¤t->mm->mmap_sem);
> + kvm_write_guest(v->kvm, wall_clock, &wc, sizeof(wc));
> + up_write(¤t->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/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
