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

[Gleb Natapov]

On Thu, Aug 01, 2013 at 03:48:29PM +0200, Paolo Bonzini wrote:
>  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?
>
here is one :)
 
Reviewed-by: Gleb Natapov <address@hidden>

--
                        Gleb.