linux-kernel - Re: [PATCH v4 09/10] tools: lib: perf: Implement riscv mmap support

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]

Date:   Mon, 31 Jul 2023 20:37:24 +0100
From:   Jessica Clarke <jrtc27@...c27.com>
To:     Alexandre Ghiti <alexghiti@...osinc.com>
Cc:     Ian Rogers <irogers@...gle.com>,
        Brendan Sweeney <brs@...osinc.com>,
        Palmer Dabbelt <palmer@...osinc.com>,
        Jonathan Corbet <corbet@....net>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Atish Patra <atishp@...shpatra.org>,
        Anup Patel <anup@...infault.org>,
        Will Deacon <will@...nel.org>, Rob Herring <robh@...nel.org>,
        Andrew Jones <ajones@...tanamicro.com>,
        Rémi Denis-Courmont <remi@...lab.net>,
        linux-doc@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-perf-users@...r.kernel.org,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Atish Patra <atishp@...osinc.com>
Subject: Re: [PATCH v4 09/10] tools: lib: perf: Implement riscv mmap support

On 31 Jul 2023, at 17:06, Alexandre Ghiti <alexghiti@...osinc.com> wrote:
> 
> On Mon, Jul 31, 2023 at 5:10 PM Ian Rogers <irogers@...gle.com> wrote:
>> 
>> On Mon, Jul 31, 2023 at 3:27 AM Alexandre Ghiti <alexghiti@...osinc.com> wrote:
>>> 
>>> On Mon, Jul 31, 2023 at 12:15 PM Alexandre Ghiti <alexghiti@...osinc.com> wrote:
>>>> 
>>>> Hi Ian,
>>>> 
>>>> On Fri, Jul 28, 2023 at 7:53 PM Ian Rogers <irogers@...gle.com> wrote:
>>>>> 
>>>>> On Thu, Jul 27, 2023 at 7:28 AM Alexandre Ghiti <alexghiti@...osinc.com> wrote:
>>>>>> 
>>>>>> riscv now supports mmaping hardware counters so add what's needed to
>>>>>> take advantage of that in libperf.
>>>>>> 
>>>>>> Signed-off-by: Alexandre Ghiti <alexghiti@...osinc.com>
>>>>>> Reviewed-by: Andrew Jones <ajones@...tanamicro.com>
>>>>>> Reviewed-by: Atish Patra <atishp@...osinc.com>
>>>>>> ---
>>>>>> 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