On 31 Jul 2023, at 17:06, Alexandre Ghiti [off-list ref] wrote:

On Mon, Jul 31, 2023 at 5:10 PM Ian Rogers [off-list ref] wrote:

quoted

On Mon, Jul 31, 2023 at 3:27 AM Alexandre Ghiti [off-list ref] wrote:

quoted

On Mon, Jul 31, 2023 at 12:15 PM Alexandre Ghiti [off-list ref] wrote:

quoted

Hi Ian,

On Fri, Jul 28, 2023 at 7:53 PM Ian Rogers [off-list ref] wrote:

quoted

On Thu, Jul 27, 2023 at 7:28 AM Alexandre Ghiti [off-list ref] wrote:

quoted

riscv now supports mmaping hardware counters so add what's needed to
take advantage of that in libperf.

Signed-off-by: Alexandre Ghiti <redacted>
Reviewed-by: Andrew Jones <redacted>
Reviewed-by: Atish Patra <redacted>
---
tools/lib/perf/mmap.c | 65 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)

diff --git a/tools/lib/perf/mmap.c b/tools/lib/perf/mmap.c
index 0d1634cedf44..378a163f0554 100644
--- a/tools/lib/perf/mmap.c
+++ b/tools/lib/perf/mmap.c

@@ -392,6 +392,71 @@ static u64 read_perf_counter(unsigned int counter)

static u64 read_timestamp(void) { return read_sysreg(cntvct_el0); }

+#elif __riscv_xlen == 64

This is something of an odd guard, perhaps:
#elif defined(__riscv) && __riscv_xlen == 64

That way it is more intention revealing that this is riscv code. Could
you add a comment relating to the __riscv_xlen ?

I guess Andrew answered that already.

Not sure. I still think it looks weird:
...
#if defined(__i386__) || defined(__x86_64__)
...
#elif defined(__aarch64__)
...
#elif __riscv_xlen == 64
...
#else
static u64 read_perf_counter(unsigned int counter __maybe_unused) { return 0; }
static u64 read_timestamp(void) { return 0; }
#endif

The first two are clearly #ifdef-ing architecture specific assembly
code, under what conditions I get RISC-V code  ¯\(ツ)/¯ At least worth
a comment like "csrr is only available when you have xlens of 64
because ..."

__riscv_xlen indicates riscv64, which is the only target of this
patchset. But if you prefer, I don't mind adding back the
defined(__riscv) if I re-spin a new version.

I mean, -Wundef is a thing that will scream at you if you evaluate a
macro that’s undefined and get an implicit 0, and parts of the linux
build seem to enable it. So I would strongly recommend against
(ab)using that feature of the preprocessor, especially when it aligns
with greater clarity.

Jess


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel