From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932370AbcFBIhg (ORCPT <rfc822;w@1wt.eu>);
	Thu, 2 Jun 2016 04:37:36 -0400
Received: from merlin.infradead.org ([205.233.59.134]:50626 "EHLO
	merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S932143AbcFBIhd (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 2 Jun 2016 04:37:33 -0400
Date: Thu, 2 Jun 2016 10:37:28 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: David Carrillo-Cisneros <davidcc@google.com>
Cc: linux-kernel@vger.kernel.org, "x86@kernel.org" <x86@kernel.org>,
        Ingo Molnar <mingo@redhat.com>, "Yan, Zheng" <zheng.z.yan@intel.com>,
        Andi Kleen <ak@linux.intel.com>, Kan Liang <kan.liang@intel.com>,
        Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH 2/3] perf/x86/intel: fix for MSR_LAST_BRANCH_FROM_x quirk
 when no TSX
Message-ID: <20160602083728.GR3193@twins.programming.kicks-ass.net>
References: <1464835323-33872-1-git-send-email-davidcc@google.com>
 <1464835323-33872-3-git-send-email-davidcc@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1464835323-33872-3-git-send-email-davidcc@google.com>
User-Agent: Mutt/1.5.21 (2012-12-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?