From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v5 RESEND 04/17] intel/VPMU: Clean up Intel VPMU code Date: Mon, 28 Apr 2014 10:00:00 -0400 Message-ID: <535E5EE0.8050804@oracle.com> References: <1398257438-4994-1-git-send-email-boris.ostrovsky@oracle.com> <1398257438-4994-5-git-send-email-boris.ostrovsky@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Tian, Kevin" Cc: "keir@xen.org" , "Nakajima, Jun" , "andrew.cooper3@citrix.com" , "Dong, Eddie" , "Dugger, Donald D" , "xen-devel@lists.xen.org" , "dietmar.hahn@ts.fujitsu.com" , "JBeulich@suse.com" , "suravee.suthikulpanit@amd.com" List-Id: xen-devel@lists.xenproject.org On 04/26/2014 04:20 AM, Tian, Kevin wrote: >> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com] >> Sent: Wednesday, April 23, 2014 8:50 PM >> >> Remove struct pmumsr and core2_pmu_enable. Replace static MSR structures >> with >> fields in core2_vpmu_context. >> >> Call core2_get_pmc_count() once, during initialization. >> >> Properly clean up when core2_vpmu_alloc_resource() fails and add routines >> to remove MSRs from VMCS. >> >> >> Signed-off-by: Boris Ostrovsky > it's a good cleanup in general. ack with two small comments later. > > Acked-by: Kevin Tian > > >> --- >> xen/arch/x86/hvm/vmx/vmcs.c | 55 ++++++ >> xen/arch/x86/hvm/vmx/vpmu_core2.c | 310 >> ++++++++++++++----------------- >> xen/include/asm-x86/hvm/vmx/vmcs.h | 2 + >> xen/include/asm-x86/hvm/vmx/vpmu_core2.h | 19 -- >> 4 files changed, 199 insertions(+), 187 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >> index 44f33cb..5f86b17 100644 >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -1205,6 +1205,34 @@ int vmx_add_guest_msr(u32 msr) >> return 0; >> } >> >> +void vmx_rm_guest_msr(u32 msr) >> +{ >> + struct vcpu *curr = current; >> + unsigned int idx, msr_count = curr->arch.hvm_vmx.msr_count; >> + struct vmx_msr_entry *msr_area = curr->arch.hvm_vmx.msr_area; >> + >> + if ( msr_area == NULL ) >> + return; >> + >> + for ( idx = 0; idx < msr_count; idx++ ) >> + if ( msr_area[idx].index == msr ) >> + break; >> + >> + if ( idx == msr_count ) >> + return; >> + >> + for ( ; idx < msr_count - 1; idx++ ) >> + { >> + msr_area[idx].index = msr_area[idx + 1].index; >> + msr_area[idx].data = msr_area[idx + 1].data; >> + } >> + msr_area[msr_count - 1].index = 0; >> + >> + curr->arch.hvm_vmx.msr_count = --msr_count; >> + __vmwrite(VM_EXIT_MSR_STORE_COUNT, msr_count); >> + __vmwrite(VM_ENTRY_MSR_LOAD_COUNT, msr_count); >> +} >> + >> int vmx_add_host_load_msr(u32 msr) >> { >> struct vcpu *curr = current; >> @@ -1235,6 +1263,33 @@ int vmx_add_host_load_msr(u32 msr) >> return 0; >> } >> >> +void vmx_rm_host_load_msr(u32 msr) >> +{ >> + struct vcpu *curr = current; >> + unsigned int idx, msr_count = curr->arch.hvm_vmx.host_msr_count; >> + struct vmx_msr_entry *msr_area = >> curr->arch.hvm_vmx.host_msr_area; >> + >> + if ( msr_area == NULL ) >> + return; >> + >> + for ( idx = 0; idx < msr_count; idx++ ) >> + if ( msr_area[idx].index == msr ) >> + break; >> + >> + if ( idx == msr_count ) >> + return; >> + >> + for ( ; idx < msr_count - 1; idx++ ) >> + { >> + msr_area[idx].index = msr_area[idx + 1].index; >> + msr_area[idx].data = msr_area[idx + 1].data; >> + } >> + msr_area[msr_count - 1].index = 0; >> + >> + curr->arch.hvm_vmx.host_msr_count = --msr_count; >> + __vmwrite(VM_EXIT_MSR_LOAD_COUNT, msr_count); >> +} >> + > suggest to combine common code in above two 'rm' functions. You can pass in > a msr_area pointer/count , and only last several lines actually differ. I then should see if I can merge vmx_add_guest_msr/vmx_add_host_load_msr as well since they have similarities too. > >> void vmx_set_eoi_exit_bitmap(struct vcpu *v, u8 vector) >> { >> if ( !test_and_set_bit(vector, v->arch.hvm_vmx.eoi_exit_bitmap) ) >> diff --git a/xen/arch/x86/hvm/vmx/vpmu_core2.c >> b/xen/arch/x86/hvm/vmx/vpmu_core2.c >> index 1e32ff3..513eca4 100644 >> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c >> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c >> @@ -69,6 +69,26 @@ >> static bool_t __read_mostly full_width_write; >> >> /* >> + * MSR_CORE_PERF_FIXED_CTR_CTRL contains the configuration of all fixed >> + * counters. 4 bits for every counter. >> + */ >> +#define FIXED_CTR_CTRL_BITS 4 >> +#define FIXED_CTR_CTRL_MASK ((1 << FIXED_CTR_CTRL_BITS) - 1) >> + >> +#define VPMU_CORE2_MAX_FIXED_PMCS 4 >> +struct core2_vpmu_context { >> + u64 fixed_ctrl; >> + u64 ds_area; >> + u64 pebs_enable; > just stay 'pebs' to be consistent. The name is was chosen to reflect the MSR name (MSR_IA32_PEBS_ENABLE). -boris > >> + u64 global_ovf_status; >> + u64 fix_counters[VPMU_CORE2_MAX_FIXED_PMCS]; >> + struct arch_msr_pair arch_msr_pair[1]; >> +}; >> +