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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.13 at 16:39, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 09/25/2013 09:55 AM, Jan Beulich wrote:
>>>>> On 20.09.13 at 11:42, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> +    {
>>> +        msr_area[i].index = msr_area[i + 1].index;
>>> +        rdmsrl(msr_area[i].index, msr_area[i].data);
>> This is clearly a side effect of the function call no-one would
>> expect. Why do you do this?
> 
> I don't understand what you are trying to say here.
> 
> (And this is wrong, instead of rdmsr it should be
>          msr_area[i].data = msr_area[i + 1].data;
> )

That was the very point - doing an MSR read here is clearly
an unexpected side effect.

>>> @@ -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...

>>> @@ -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.

Jan


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