linux-kernel - Re: [RFC] Support Intel-PT code build in 32-bit arches

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] [day] [month] [year] [list]

Date:   Fri, 22 Oct 2021 15:23:53 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     John Garry <john.garry@...wei.com>, peterz@...radead.org,
        mark.rutland@....com, alexander.shishkin@...ux.intel.com,
        jolsa@...hat.com, namhyung@...nel.org, mingo@...hat.com,
        irogers@...gle.com, linux-perf-users@...r.kernel.org,
        linux-kernel@...r.kernel.org, James Clark <James.Clark@....com>
Subject: Re: [RFC] Support Intel-PT code build in 32-bit arches

On 21/10/2021 23:31, Arnaldo Carvalho de Melo wrote:
> Em Wed, Oct 20, 2021 at 02:25:43PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Wed, Oct 20, 2021 at 03:41:01PM +0100, John Garry escreveu:
>>> I suppose the best thing now is to send a patch on top once perf/core
>>> contains that commit. Let me know otherwise.
>  
>> You can send a v2, as:
>  
>>   29     8.60 debian:experimental-x-mipsel  : FAIL gcc version 11.2.0 (Debian 11.2.0-9)
>>     util/intel-pt.c: In function 'intel_pt_synth_pebs_sample':
>>     util/intel-pt.c:2146:33: error: passing argument 1 of 'find_first_bit' from incompatible pointer type [-Werror=incompatible-pointer-types]
>>      2146 |         for_each_set_bit(hw_id, &items->applicable_counters, INTEL_PT_MAX_PEBS) {
>>     /git/perf-5.15.0-rc4/tools/include/linux/bitops.h:37:38: note: in definition of macro 'for_each_set_bit'
>>        37 |         for ((bit) = find_first_bit((addr), (size));            \
>>           |                                      ^~~~
>>     In file included from /git/perf-5.15.0-rc4/tools/include/asm-generic/bitops.h:21,
>>                      from /git/perf-5.15.0-rc4/tools/include/linux/bitops.h:34,
>>                      from /git/perf-5.15.0-rc4/tools/include/linux/bitmap.h:6,
>>                      from util/header.h:10,
>>                      from util/session.h:7,
>>                      from util/intel-pt.c:16:
>>     /git/perf-5.15.0-rc4/tools/include/asm-generic/bitops/find.h:109:51: note: expected 'const long unsigned int *' but argument is of type 'const uint64_t *' {aka 'const long long unsigned int *'}
>>
>>  Adrian, this is on:
>>
>>  commit 803a3c9233990e1adac8ea2421e3759c2d380cf8
>> Author: Adrian Hunter <adrian.hunter@...el.com>
>> Date:   Tue Sep 7 19:39:03 2021 +0300
>>
>>     perf intel-pt: Add support for PERF_RECORD_AUX_OUTPUT_HW_ID
>>
>>     Originally, software only supported redirecting at most one PEBS event to
>>     Intel PT (PEBS-via-PT) because it was not able to differentiate one event
>>     from another. To overcome that, add support for the
>>     PERF_RECORD_AUX_OUTPUT_HW_ID side-band event.
>>
>>     Reviewed-by: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
>>     Reviewed-by: Andi Kleen <ak@...ux.intel.com>
>>
>>
>> That is still just on tmp.perf/core, so we can fix it, probably its just
>> making that uint64_t into a unsigned long, will check later if you don't
>> beat me to it.
> 
> Adrian,
> 
> 	Probably we should just disable Intel PT support from being
> built on 32-bit arches, right? I don't know if anybody is interested on
> that, my tests just try to figure out if it continues to build, and if
> fixing any problem isn't costly, which in this case is more than my
> threshold, wdyt?

It feels like bad form not to compile on 32-bit.

In this case we could just cast it, and I see that has been done
in other places for for_each_set_bit(), but (and maybe I've got
it wrong) that wouldn't work for a 32-bit big endian system?

The following might cover all cases, do you think?

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 1073c56a512c..7c979ffefade 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -2129,9 +2129,17 @@ static int intel_pt_synth_single_pebs_sample(struct intel_pt_queue *ptq)
 	return intel_pt_do_synth_pebs_sample(ptq, evsel, id);
 }
 
+/* For 32-bit big endian, put the longs from u64 in correct order */
+#define U64_BITS(var, val)		\
+	unsigned long var[] = {		\
+		[0] = (val),		\
+		[1] = (val) >> 32,	\
+	}
+
 static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
 {
 	const struct intel_pt_blk_items *items = &ptq->state->items;
+	U64_BITS(ac_bits, items->applicable_counters);
 	struct intel_pt_pebs_event *pe;
 	struct intel_pt *pt = ptq->pt;
 	int err = -EINVAL;
@@ -2143,7 +2151,7 @@ static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
 		return intel_pt_synth_single_pebs_sample(ptq);
 	}
 
-	for_each_set_bit(hw_id, &items->applicable_counters, INTEL_PT_MAX_PEBS) {
+	for_each_set_bit(hw_id, ac_bits,  INTEL_PT_MAX_PEBS) {
 		pe = &ptq->pebs[hw_id];
 		if (!pe->evsel) {
 			if (!pt->single_pebs)