Re: [Xen-devel] [PATCH v2] Intel/VPMU: Add support for full-width PMC writes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.08.13 at 16:53, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -64,6 +64,10 @@
>  #define PMU_FIXED_WIDTH_BITS     8  /* 8 bits 5..12 */
>  #define PMU_FIXED_WIDTH_MASK     (((1 << PMU_FIXED_WIDTH_BITS) -1) << 
> PMU_FIXED_WIDTH_SHIFT)
>  
> +/* Alias registers (0x4c1) for full-width writes to PMCs */
> +#define MSR_PMC_ALIAS_MASK       (~0x400)

I'd prefer this to be (~(MSR_IA32_PERFCTR0 ^ MSR_IA32_A_PERFCTR0)),
so there's just a single definition. Alternatively you could put
this in the header and use it for defining MSR_IA32_A_PERFCTR0.

> +bool_t __read_mostly full_width_write;

static.

> @@ -260,6 +266,15 @@ static void core2_vpmu_set_msr_bitmap(unsigned long 
> *msr_bitmap)
>          clear_bit(msraddr_to_bitpos(MSR_IA32_PERFCTR0+i),
>                    msr_bitmap + 0x800/BYTES_PER_LONG);
>      }
> +    if ( full_width_write )
> +    {
> +        for ( i = 0; i < core2_get_pmc_count(); i++ )
> +        {
> +            clear_bit(msraddr_to_bitpos(MSR_IA32_A_PERFCTR0+i), msr_bitmap);
> +            clear_bit(msraddr_to_bitpos(MSR_IA32_A_PERFCTR0+i),
> +                      msr_bitmap + 0x800/BYTES_PER_LONG);
> +        }
> +    }

Perhaps better put these into the earlier loop, rather than having
a second one?

> @@ -324,11 +349,17 @@ static inline void __core2_vpmu_load(struct vcpu *v)
>  {
>      int i;
>      struct core2_vpmu_context *core2_vpmu_cxt = vcpu_vpmu(v)->context;
> +    int pmc_start;
>  
>      for ( i = 0; i < core2_fix_counters.num; i++ )
>          wrmsrl(core2_fix_counters.msr[i], core2_vpmu_cxt->fix_counters[i]);
> +
> +    if ( full_width_write )
> +        pmc_start = MSR_IA32_A_PERFCTR0;
> +    else
> +        pmc_start = MSR_IA32_PERFCTR0;
>      for ( i = 0; i < core2_get_pmc_count(); i++ )
> -        wrmsrl(MSR_IA32_PERFCTR0+i, 
> core2_vpmu_cxt->arch_msr_pair[i].counter);
> +        wrmsrl(pmc_start+i, core2_vpmu_cxt->arch_msr_pair[i].counter);

Even if the old code (wrongly) didn't have them - please add
spaces around the +.

Jan


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