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

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.

quoted

quoted

quoted

quoted

+
+/* TODO: implement rv32 support */
+
+#define CSR_CYCLE      0xc00
+#define CSR_TIME       0xc01
+
+#define csr_read(csr)                                          \
+({                                                             \
+       register unsigned long __v;                             \
+               __asm__ __volatile__ ("csrr %0, " #csr          \
+                : "=r" (__v) :                                 \
+                : "memory");                                   \

To avoid the macro pasting that could potentially go weird, could this be:

__asm__ __volatile__ ("csrr %0, %1",
  : "=r"(__v) /* outputs */
  : "i"(csr) /* inputs */
  : "memory" /* clobbers */)

Forgot to answer this one: it compiles, but I have to admit that I
don't understand the difference and if that's correct (all macros in
arch/riscv/include/asm/csr.h use # to do this) and what benefits it
brings. Can you elaborate more on things that could "go weird"?

So rather than use an input constraint for the asm block you are using
the C preprocessor to paste in the csr argument. If csr is something
like "1" then it looks good and you'll get "csrr %0,1". If you pass
something like "1 << 31" then that will be pasted as "csrr %0, 1 <<
31" and that starts to get weird in the context of being in the
assembler where it is unlikely the C operators work. Using the input
constraint avoids this, causes the C compiler to check the type of the
argument and you'll probably get more intelligible error messages as a
consequence.

Thanks. So if I'm not mistaken, in this exact context, given we only
use csr_read() through the csr_read_num() function, it seems ok right?

quoted

Thanks again,

Alex

quoted

quoted

Also, why is this clobbering memory? Worth adding a comment.

No idea, I see that it is also done this way in
arch/riscv/include/asm/csr.h. @Atish Kumar Patra , @Palmer Dabbelt ?

It would seem to make sense then not to have a memory constraint until
we know why we're doing it?

I have just had the answer internally (thanks to @Brendan Sweeney):
csr modifications can alter how the memory is accessed (satp which
changes the address space, sum which allows/disallows userspace
access...), so we need the memory barrier to make sure the compiler
does not reorder the memory accesses.

Thanks,

Alex

Thanks,
Ian

quoted

quoted

Thanks for your comments!

Alex

quoted

Thanks,
Ian

quoted

+                __v;                                           \
+})
+
+static unsigned long csr_read_num(int csr_num)
+{
+#define switchcase_csr_read(__csr_num, __val)           {\
+       case __csr_num:                                 \
+               __val = csr_read(__csr_num);            \
+               break; }
+#define switchcase_csr_read_2(__csr_num, __val)         {\
+       switchcase_csr_read(__csr_num + 0, __val)        \
+       switchcase_csr_read(__csr_num + 1, __val)}
+#define switchcase_csr_read_4(__csr_num, __val)         {\
+       switchcase_csr_read_2(__csr_num + 0, __val)      \
+       switchcase_csr_read_2(__csr_num + 2, __val)}
+#define switchcase_csr_read_8(__csr_num, __val)         {\
+       switchcase_csr_read_4(__csr_num + 0, __val)      \
+       switchcase_csr_read_4(__csr_num + 4, __val)}
+#define switchcase_csr_read_16(__csr_num, __val)        {\
+       switchcase_csr_read_8(__csr_num + 0, __val)      \
+       switchcase_csr_read_8(__csr_num + 8, __val)}
+#define switchcase_csr_read_32(__csr_num, __val)        {\
+       switchcase_csr_read_16(__csr_num + 0, __val)     \
+       switchcase_csr_read_16(__csr_num + 16, __val)}
+
+       unsigned long ret = 0;
+
+       switch (csr_num) {
+       switchcase_csr_read_32(CSR_CYCLE, ret)
+       default:
+               break;
+       }
+
+       return ret;
+#undef switchcase_csr_read_32
+#undef switchcase_csr_read_16
+#undef switchcase_csr_read_8
+#undef switchcase_csr_read_4
+#undef switchcase_csr_read_2
+#undef switchcase_csr_read
+}
+
+static u64 read_perf_counter(unsigned int counter)
+{
+       return csr_read_num(CSR_CYCLE + counter);
+}
+
+static u64 read_timestamp(void)
+{
+       return csr_read_num(CSR_TIME);
+}
+
 #else
 static u64 read_perf_counter(unsigned int counter __maybe_unused) { return 0; }
 static u64 read_timestamp(void) { return 0; }
--
2.39.2

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