linux-kernel - Re: [PATCH v2 01/17] perf tools: Configure contextID tracing in CPU-wide mode

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20190607182047.GK21245@kernel.org>
Date:   Fri, 7 Jun 2019 15:20:47 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To:     Suzuki K Poulose <suzuki.poulose@....com>
Cc:     mathieu.poirier@...aro.org, Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>, leo.yan@...aro.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        coresight@...ts.linaro.org
Subject: Re: [PATCH v2 01/17] perf tools: Configure contextID tracing in
 CPU-wide mode

Em Fri, Jun 07, 2019 at 10:21:36AM +0100, Suzuki K Poulose escreveu:
> Hi Mathieu,
> 
> On 24/05/2019 18:34, Mathieu Poirier wrote:
> > When operating in CPU-wide mode being notified of contextID changes is
> > required so that the decoding mechanic is aware of the process context
> > switch.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@...aro.org>
> 
> 
> > Reviewed-by: Suzuki K Poulose <suzuki.poulose@....com>
> 
> I am sorry but, I don't remember reviewing this patch in the previous
> postings. But here we go.

Can I keep it as is? I addressed one of your concerns below, please
check.

- Arnaldo
 
> > +++ b/tools/perf/util/cs-etm.h
> > @@ -103,6 +103,18 @@ struct intlist *traceid_list;
> >   #define KiB(x) ((x) * 1024)
> >   #define MiB(x) ((x) * 1024 * 1024)
> > +/*
> > + * Create a contiguous bitmask starting at bit position @l and ending at
> > + * position @h. For example
> > + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> > + *
> > + * Carbon copy of implementation found in $KERNEL/include/linux/bitops.h
> > + */
> > +#define GENMASK(h, l) \
> > +	(((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> > +
> 
> minor nit: Could this be placed in a more generic header file for the other
> parts of the perf tool to consume ?
> 

Yeah, since we have:

Good catch, we have it already:

[acme@...co perf]$ tail tools/include/linux/bits.h
 * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
 */
#define GENMASK(h, l) \
	(((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))

#define GENMASK_ULL(h, l) \
	(((~0ULL) - (1ULL << (l)) + 1) & \
	 (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))

#endif	/* __LINUX_BITS_H */
[acme@...co perf]$
[acme@...co perf]$

So I'm adding this to the pile with a Suggested-by: Suzuki, ok?

commit 3217a621248824fbff8563d8447fdafe69c5316d
Author: Arnaldo Carvalho de Melo <acme@...hat.com>
Date:   Fri Jun 7 15:14:27 2019 -0300

    perf cs-etm: Remove duplicate GENMASK() define, use linux/bits.h instead
    
    Suzuki noticed that this should be more useful in a generic header, and
    after looking I noticed we have it already in our copy of
    include/linux/bits.h in tools/include, so just use it, test built on
    x86-64 and ubuntu 19.04 with:
    
      perfbuilder@...46c9e848e:/$ aarch64-linux-gnu-gcc --version |& head -1
      aarch64-linux-gnu-gcc (Ubuntu/Linaro 8.3.0-6ubuntu1) 8.3.0
      perfbuilder@...46c9e848e:/$
    
    Suggested-by: Suzuki K Poulose <suzuki.poulose@....com>
    Link: https://lkml.kernel.org/r/68c1c548-33cd-31e8-100d-7ffad008c7b2@arm.com
    Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
    Cc: Jiri Olsa <jolsa@...hat.com>
    Cc: Leo Yan <leo.yan@...aro.org>
    Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
    Cc: Namhyung Kim <namhyung@...nel.org>
    Cc: Peter Zijlstra <peterz@...radead.org>
    Cc: coresight@...ts.linaro.org
    Cc: linux-arm-kernel@...ts.infradead.org,
    Link: https://lkml.kernel.org/n/tip-69pd3mqvxdlh2shddsc7yhyv@git.kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>

diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
index 33b57e748c3d..bc848fd095f4 100644
--- a/tools/perf/util/cs-etm.h
+++ b/tools/perf/util/cs-etm.h
@@ -9,6 +9,7 @@
 
 #include "util/event.h"
 #include "util/session.h"
+#include <linux/bits.h>
 
 /* Versionning header in case things need tro change in the future.  That way
  * decoding of old snapshot is still possible.
@@ -161,16 +162,6 @@ struct cs_etm_packet_queue {
 
 #define CS_ETM_INVAL_ADDR 0xdeadbeefdeadbeefUL
 
-/*
- * Create a contiguous bitmask starting at bit position @l and ending at
- * position @h. For example
- * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
- *
- * Carbon copy of implementation found in $KERNEL/include/linux/bitops.h
- */
-#define GENMASK(h, l) \
-	(((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
-
 #define BMVAL(val, lsb, msb)	((val & GENMASK(msb, lsb)) >> lsb)
 
 #define CS_ETM_HEADER_SIZE (CS_HEADER_VERSION_0_MAX * sizeof(u64))