Re: [PATCH 2/3] perf/x86/intel: fix for MSR_LAST_BRANCH_FROM_x quirk when no TSX

From: Peter Zijlstra
Date: Thu Jun 02 2016 - 04:37:41 EST


On Wed, Jun 01, 2016 at 07:42:02PM -0700, David Carrillo-Cisneros wrote:
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index a5e52ad4..1ce172d 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3309,6 +3309,7 @@ static void intel_snb_check_microcode(void)
> static bool check_msr(unsigned long msr, u64 mask)
> {
> u64 val_old, val_new, val_tmp;
> + u64 (*wr_quirk)(u64);
>
> /*
> * Read the current value, change it and read it back to see if it
> @@ -3322,13 +3323,30 @@ static bool check_msr(unsigned long msr, u64 mask)
> * Only change the bits which can be updated by wrmsrl.
> */
> val_tmp = val_old ^ mask;
> +
> + /* Use wr quirk for lbr msr's. */
> + if ((x86_pmu.lbr_from <= msr &&
> + msr < x86_pmu.lbr_from + x86_pmu.lbr_nr) ||
> + (x86_pmu.lbr_to <= msr &&
> + msr < x86_pmu.lbr_to + x86_pmu.lbr_nr))
> + wr_quirk = lbr_from_signext_quirk_wr;


This is unreadable code..

static inline bool within(unsigned long msr, unsigned long base, unsigned long size)
{
return base <= msr && msr < base + size;
}

static inline bool is_lbr_msr(unsigned long msr)
{
if (within(msr, x86_pmu.lbr_from, x86_pmu.lbr_nr))
return true;
if (within(msr, x86_pmu.lbr_to, x86_pmu.lbr_nr))
return true;
return false;
}

Yes, its a few more lines, but its bloody obvious what it does at any
time of the day.

Also, seeing how the write quirk is for the LBR_FROM only, wth are you
testing against _TO too?