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.
