linux-kernel - Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170201101704.GA450@gmail.com>
Date:   Wed, 1 Feb 2017 11:17:04 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Cc:     Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        vince@...ter.net, eranian@...gle.com,
        Arnaldo Carvalho de Melo <acme@...radead.org>,
        Borislav Petkov <bp@...e.de>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing


* Alexander Shishkin <alexander.shishkin@...ux.intel.com> wrote:

> Now that Intel PT supports more types of trace content than just branch
> tracing, it may be useful to allow the user to disable branch tracing
> when it is not needed.
> 
> The special case is BDW, where not setting BranchEn is not supported.
> 
> This patch adds 'no_branch' event format string to PT events, which
> will disable setting BranchEn bit in the hardware trace configuration.

> +	/* trying to unset BRANCH_EN where it is not supported */

Please capitalize comments consistently and use the typical tense. This one should 
be something like:

	/* Try to unset BRANCH_EN where it is not supported: */

>  
>  	reg = pt_config_filters(event);
> -	reg |= RTIT_CTL_TOPA | RTIT_CTL_BRANCH_EN | RTIT_CTL_TRACEEN;
> +	reg |= RTIT_CTL_TOPA | RTIT_CTL_TRACEEN;
> +
> +	/*
> +	 * Previously, we had BRANCH_EN on by default, but now that PT has
> +	 * grown features outside of branch tracing, it is useful to allow
> +	 * the user to disable it. So, to keep compatibility, setting
> +	 * BRANCH_EN bit in the event config (no_branch=1) will have the
> +	 * reverse effect and *not* set BRANCH_EN in the hardware
> +	 * configuration.
> +	 */
> +	if (!(event->attr.config & RTIT_CTL_BRANCH_EN))
> +		reg |= RTIT_CTL_BRANCH_EN;
> +	else
> +		event->attr.config &= ~RTIT_CTL_BRANCH_EN;


So I really hate this ABI hack - it's these unclean approaches how ABIs degrade 
over time, by death of a thousand cuts...

Any reason why we couldn't add a separate pt_feature_branch_disable and 
pt_feature_trace_disable bits and interpret them in a straightforward way, or 
something like that?

( This means two more bits, but that's our punishment for not anticipating 
  extensions to the hardware feature. )

Also, rename "RTIT_CTL_BRANCH_EN" to "RTIT_CTL_PT_EN" (but without changing the 
ABI), to more clearly express what that bit realy does.

I.e. we'd have a hierarchy of flags:

	- the old RTIT_CTL_BRANCH_EN bit (now RTIT_CTL_PT_EN) enables all of PT, 
	  with all features

	- individual feature disabling bits, which default to 0 (i.e. the feature 
	  is enabled) in the attr structure control the finegrained 
	  enabling/disabling of PT features. Currently there are two bits: 
	  pt_feature_branch_disable and pt_feature_branch_enable. More are added 
	  in the future if PT grows even more features.

or so?

Thanks,

	Ingo