Re: [Xen-devel] [PATCH v2 05/13] intel/VPMU: Clean up Intel VPMU code

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 09/25/2013 10:57 AM, Jan Beulich wrote:
> > > > On 25.09.13 at 16:39, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel