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

[Paolo Bonzini]

 On Aug 01 2013, Gleb Natapov wrote:
> 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?
> 
> 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,

Yeah, I found it now.

> I am OK with your patch, it is a little bit unfortunate that userspase
> should care about such low level details though.

Is it a Reviewed-by?

Paolo