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

[Marcelo Tosatti <marcelo@kvack.org>]

On Wed, Feb 13, 2008 at 12:49:42PM +0200, Avi Kivity 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.
> >
> >   
> 
> > +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).

You need slots_lock in read-mode for kvm_write_guest(). kvm->lock does
not protect against memslot changes and the dirty bitmap.

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