From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Huang Subject: Re: [PATCH V2 3/5] KVM: x86/vPMU: Create vPMU interface for VMX and SVM Date: Thu, 09 Apr 2015 15:03:48 -0500 Message-ID: <5526DB24.8020707@redhat.com> References: <1428509905-32352-1-git-send-email-wei@redhat.com> <1428509905-32352-4-git-send-email-wei@redhat.com> <20150409194344.GC9729@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, pbonzini@redhat.com, gleb@kernel.org To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Return-path: Received: from mx1.redhat.com ([209.132.183.28]:59035 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933018AbbDIUDv (ORCPT ); Thu, 9 Apr 2015 16:03:51 -0400 In-Reply-To: <20150409194344.GC9729@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 4/9/15 14:43, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > 2015-04-08 12:18-0400, Wei Huang: >> This patch converts existing Intel specific vPMU code into a common = vPMU >> interface with the following steps: >> >> - A large portion of Intel vPMU code is now moved to pmu.c file. The= rest >> is re-arranged and hooked up with the newly-defined intel_pmu_ops= =2E >> - Create a corresponding pmu_amd.c file with empty functions for AMD= SVM >> - The PMU function pointer, kvm_pmu_ops, is initialized by calling >> kvm_x86_ops->get_pmu_ops(). >> - To reduce the code size, Intel and AMD modules are now generated >> from their corrponding arch and PMU files; In the meanwhile, due >> to this arrangement several functions are exposed as public ones >> to allow calling from PMU code. >> >> Signed-off-by: Wei Huang >> --- >> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c > | [...] >> +static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type, >> + unsigned config, bool exclude_user, >> + bool exclude_kernel, bool intr, > > This patch introduces a lot of trailing whitespaces, please remove th= em. > (`git am` says 15.) Some of them were carried from original pmu.c file. I will purge them i= n V3. > > | [...] >> +EXPORT_SYMBOL_GPL(reprogram_counter); >> + > > (double line) > OK >> + >> +void kvm_pmu_handle_event(struct kvm_vcpu *vcpu) > | [...] >> +/* check if msr_idx is a valid index to access PMU */ >> +inline int kvm_pmu_check_msr_idx(struct kvm_vcpu *vcpu, unsigned ms= r_idx) > > If we really want it inline, it's better done in header. > (I think GCC would inline this in-module anyway, but other modules st= ill > have to call it.) OK > >> +{ >> + return kvm_pmu_ops->check_msr_idx(vcpu, msr_idx); >> +} >> + > | [...] >> +bool kvm_pmu_is_msr(struct kvm_vcpu *vcpu, u32 msr) > > (Might make sense to inline these trivial wrappers.) > OK, I will move them to pmu.h header as inline. >> +{ >> + return kvm_pmu_ops->is_pmu_msr(vcpu, msr); >> +} >> + >> +int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data) >> +{ >> + return kvm_pmu_ops->get_msr(vcpu, msr, data); >> +} >> + >> +int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_inf= o) >> +{ >> + return kvm_pmu_ops->set_msr(vcpu, msr_info); >> +=09 > > (Technically also a trailing newline.) OK > >> +} > | [...] >> +/* Called before using any PMU functions above */ >> +int kvm_pmu_arch_init(struct kvm_x86_ops *x86_ops) >> +{ >> + kvm_pmu_ops =3D x86_ops->get_pmu_ops(); > > I guess you forgot this line ^^. > Ahh, will fix it. >> + >> + if (x86_ops && (kvm_pmu_ops =3D x86_ops->get_pmu_ops()) !=3D NULL) >> + return 0; >> + else >> + return -EINVAL; >> +} >> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h >> @@ -0,0 +1,98 @@ >> +#ifndef __KVM_X86_PMU_H >> +#define __KVM_X86_PMU_H >> + >> +#define VCPU_TO_PMU(vcpu) (&(vcpu)->arch.pmu) >> +#define PMU_TO_VCPU(pmu) (container_of((pmu), struct kvm_vcpu, arc= h.pmu)) >> +#define PMC_TO_PMU(pmc) (&(pmc)->vcpu->arch.pmu) > > (We are replacing inline functions with these macros and they wouldn'= t > have been in caps, so I would use lower case; looks better too, IMO= =2E) > OK, will make it to lower case. >> +/* retrieve the control fields of IA32_FIXED_CTR_CTRL */ >> +#define fixed_ctr_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*= 4)) & 0xf) > | [...] >> diff --git a/arch/x86/kvm/pmu_intel.c b/arch/x86/kvm/pmu_intel.c >> @@ -328,20 +156,21 @@ bool kvm_pmu_msr(struct kvm_vcpu *vcpu, u32 ms= r) >> ret =3D pmu->version > 1; >> break; >> default: >> - ret =3D get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) >> - || get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0) >> - || get_fixed_pmc(pmu, msr); >> + ret =3D get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) || >> + get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0) || >> + get_fixed_pmc(pmu, msr); >> break; >> } >> + >> return ret; >> } > > (This hunk and the following hunks where you mostly change index -> m= sr > could have been in a separate cleanup patch.) > OK, agreed that such changes can be separated. I will extract them to=20 make your life easier. >> -int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data) >> +static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *d= ata) > | [...] >> -int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_inf= o) >> +static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data= *msr_info) > | [...] >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 32bf19e..4d9e7de 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -28,6 +28,7 @@ >> #include "x86.h" >> #include "cpuid.h" >> #include "assigned-dev.h" >> +#include "pmu.h" >> >> #include >> #include >> @@ -899,7 +900,7 @@ bool kvm_rdpmc(struct kvm_vcpu *vcpu) >> u64 data; >> int err; >> >> - err =3D kvm_pmu_read_pmc(vcpu, ecx, &data); >> + err =3D kvm_pmu_rdpmc(vcpu, ecx, &data); > > (What was the reason behind the new name?) Original kvm_pmu_read_pmc() was only used for RDPMC instruction. Such=20 change make it easier to be correlated with RDPMC directly; plus pmc is= =20 a confusing term in vPMU code (see "Design Note" in pmu.c file), which=20 makes me to think kvm_pmu_read_pmc() should read from "struct kvm_pmc"=20 directly. > >> if (err) >> return err; >> kvm_register_write(vcpu, VCPU_REGS_RAX, (u32)data); >> @@ -2279,7 +2280,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, = struct msr_data *msr_info) >> pr =3D true; >> case MSR_P6_EVNTSEL0: >> case MSR_P6_EVNTSEL1: >> - if (kvm_pmu_msr(vcpu, msr)) >> + if (kvm_pmu_is_msr(vcpu, msr)) > > (I liked kvm_pmu_msr better. The 'is' is in a wrong spot ... we know= it > is "a MSR", we want to know if it is "PMU MSR".) > OK >> @@ -4918,13 +4919,13 @@ static int emulator_set_msr(struct x86_emula= te_ctxt *ctxt, >> static int emulator_check_pmc(struct x86_emulate_ctxt *ctxt, >> u32 pmc) >> { >> - return kvm_pmu_check_pmc(emul_to_vcpu(ctxt), pmc); >> + return kvm_pmu_check_msr_idx(emul_to_vcpu(ctxt), pmc); > > (Why not pmc?) See "Design Note" in pmu.c for a better explanation. I tried to use msr= =20 as real x86 MSR; and msr_idx refers to MSR offset. >