From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45382) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4sg2-0001A8-8j for qemu-devel@nongnu.org; Thu, 01 Aug 2013 09:12:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4sfv-0002Wr-W8 for qemu-devel@nongnu.org; Thu, 01 Aug 2013 09:12:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17217) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4sfv-0002Wc-M2 for qemu-devel@nongnu.org; Thu, 01 Aug 2013 09:12:15 -0400 Date: Thu, 1 Aug 2013 16:12:12 +0300 From: Gleb Natapov Message-ID: <20130801131212.GG6042@redhat.com> References: <1374764722-10685-1-git-send-email-pbonzini@redhat.com> <1374764722-10685-3-git-send-email-pbonzini@redhat.com> <20130728125745.GA27246@redhat.com> <51F521DD.5010308@redhat.com> <20130728135456.GJ11772@redhat.com> <51F525A9.7030503@redhat.com> <20130728142425.GK11772@redhat.com> <20130801130312.GG5245@mail.corp.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130801130312.GG5245@mail.corp.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 2/2] kvm: migrate vPMU state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, afaerber@suse.de, ehabkost@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.