From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v2 05/13] intel/VPMU: Clean up Intel VPMU code Date: Wed, 25 Sep 2013 11:37:12 -0400 Message-ID: <52430328.2040008@oracle.com> References: <1379670132-1748-1-git-send-email-boris.ostrovsky@oracle.com> <1379670132-1748-6-git-send-email-boris.ostrovsky@oracle.com> <5243078302000078000F6461@nat28.tlf.novell.com> <5242F5B4.9000505@oracle.com> <524315E002000078000F656A@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VOr7Q-00033p-OV for xen-devel@lists.xenproject.org; Wed, 25 Sep 2013 15:35:13 +0000 In-Reply-To: <524315E002000078000F656A@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: suravee.suthikulpanit@amd.com, George.Dunlap@eu.citrix.com, jacob.shin@amd.com, eddie.dong@intel.com, dietmar.hahn@ts.fujitsu.com, jun.nakajima@intel.com, xen-devel List-Id: xen-devel@lists.xenproject.org On 09/25/2013 10:57 AM, Jan Beulich wrote: >>>> On 25.09.13 at 16:39, Boris Ostrovsky wrote: >>>> >>>> @@ -248,13 +230,13 @@ static void core2_vpmu_set_msr_bitmap(unsigned long *msr_bitmap) >>>> int i; >>>> >>>> /* Allow Read/Write PMU Counters MSR Directly. */ >>>> - for ( i = 0; i < core2_fix_counters.num; i++ ) >>>> + for ( i = 0; i < fixed_pmc_cnt; i++ ) >>>> { >>>> - clear_bit(msraddr_to_bitpos(core2_fix_counters.msr[i]), msr_bitmap); >>>> - clear_bit(msraddr_to_bitpos(core2_fix_counters.msr[i]), >>>> + clear_bit(msraddr_to_bitpos(MSR_CORE_PERF_FIXED_CTR0 + i), >> msr_bitmap); >>>> + clear_bit(msraddr_to_bitpos(MSR_CORE_PERF_FIXED_CTR0 + i), >>> Dropping the static array will make the handling here quite a bit more >>> complicated should there ever appear a second dis-contiguous MSR >>> range. >> Fixed counters range should always be contiguous per Intel SDM. > Until the current range runs out... Well, there are 58 free addresses currently available in this range... >>>> @@ -262,32 +244,37 @@ static void core2_vpmu_set_msr_bitmap(unsigned long *msr_bitmap) >>>> } >>>> >>>> /* Allow Read PMU Non-global Controls Directly. */ >>>> - for ( i = 0; i < core2_ctrls.num; i++ ) >>>> - clear_bit(msraddr_to_bitpos(core2_ctrls.msr[i]), msr_bitmap); >>>> - for ( i = 0; i < core2_get_pmc_count(); i++ ) >>>> + for ( i = 0; i < arch_pmc_cnt; i++ ) >>>> clear_bit(msraddr_to_bitpos(MSR_P6_EVNTSEL0+i), msr_bitmap); >>>> + >>>> + clear_bit(msraddr_to_bitpos(MSR_CORE_PERF_FIXED_CTR_CTRL), msr_bitmap); >>>> + clear_bit(msraddr_to_bitpos(MSR_IA32_PEBS_ENABLE), msr_bitmap); >>>> + clear_bit(msraddr_to_bitpos(MSR_IA32_DS_AREA), msr_bitmap); >>> As you can see, this is already the case here. >> This is a different set of MSRs from from what you've commented on above. > Sure, but the effect of breaking up a loop into individual operations > is seen quite nicely here. Yes, but unlike fixed counters above, the registers in what used to be in core2_ctrls.msr are responsible for different things. And in certain cases we want to access one register but not the other. An example is in current version of vpmu_dump(): val = core2_vpmu_cxt->ctrls[MSR_CORE_PERF_FIXED_CTR_CTRL_IDX]; We had to add another macro (MSR_CORE_PERF_FIXED_CTR_CTRL_IDX). So I think that separating these registers explicitly makes sense. -boris