Re: [Qemu-devel] [PATCH v2 2/2] kvm: migrate vPMU state

[Gleb Natapov <gleb@redhat.com>]

On Thu, Aug 01, 2013 at 03:03:12PM +0200, Paolo Bonzini wrote:
> > KVM disabled HW counters when outside of a guest mode (otherwise result
> > will be useless), so I do not see how the problem you describe can
> > happen.
> 
> Yes, you're right.
> 
> > On the other hand MPU emulation assumes that counter have to be disabled
> > while MSR_IA32_PERFCTR0 is written since write to MSR_IA32_PERFCTR0 does
> > not reprogram perf evens, so we need either disable/enable counters to
> > write MSR_IA32_PERFCTR0 or have this patch in the kernel:
> > 
> > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > index 5c4f631..bf14e42 100644
> > --- a/arch/x86/kvm/pmu.c
> > +++ b/arch/x86/kvm/pmu.c
> > @@ -412,6 +412,8 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  			if (!msr_info->host_initiated)
> >  				data = (s64)(s32)data;
> >  			pmc->counter += data - read_pmc(pmc);
> > +			if (msr_info->host_initiated)
> > +				reprogram_gp_counter(pmc, pmc->eventsel);
> >  			return 0;
> >  		} else if ((pmc = get_gp_pmc(pmu, index, MSR_P6_EVNTSEL0))) {
> >  			if (data == pmc->eventsel)
> 
> Why do you need "if (msr_info->host_initiated)"?  I could not find any
> hint in the manual that the overflow counter will still use the value
> of the counter that was programmed first.
> 
Not sure I understand. What "overflow counter will still use the value
of the counter that was programmed first" means?

Strictly speaking we do need "if (msr_info->host_initiated)" here,
there is no harm in calling reprogram_gp_counter() unconditionally,
but spec says in no vague terms that counter should be disabled before
writing into the MSR and it means that reprogram_gp_counter() will be
called again when guest will enable counter later, so the invocation
here is redundant and since during profiling this happens a lot avoiding
call to reprogram_gp_counter() is a win.

> If we need to do it always, I agree it's better to modify the QEMU
> patch and not disable/enable the counters.  But if we need to restrict
> it to host-initiated writes, I would rather have the QEMU patch as I
> posted it.  So far we always had less side-effects from host_initiated,
> not more, and I think it's a good rule of thumb.
> 
I am OK with your patch, it is a little bit unfortunate that userspase
should care about such low level details though.

--
			Gleb.