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

[Peter Zijlstra]

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?