Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.600.7\))
Subject: Re: [PATCH v4 09/10] tools: lib: perf: Implement riscv mmap support
From:   Jessica Clarke <jrtc27@jrtc27.com>
In-Reply-To: <CAHVXubjhM9C1fw_Us=8+RuSJbW0pacFAk9gp7j2=BtMUPy_Byw@mail.gmail.com>
Date:   Mon, 31 Jul 2023 20:37:24 +0100
Cc:     Ian Rogers <irogers@google.com>,
        Brendan Sweeney <brs@rivosinc.com>,
        Palmer Dabbelt <palmer@rivosinc.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Atish Patra <atishp@atishpatra.org>,
        Anup Patel <anup@brainfault.org>,
        Will Deacon <will@kernel.org>, Rob Herring <robh@kernel.org>,
        Andrew Jones <ajones@ventanamicro.com>,
        =?utf-8?Q?R=C3=A9mi_Denis-Courmont?= <remi@remlab.net>,
        linux-doc@vger.kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-perf-users@vger.kernel.org,
        linux-riscv <linux-riscv@lists.infradead.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Atish Patra <atishp@rivosinc.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <3736D0AC-C87B-45D8-AB20-3A5A55D4FA64@jrtc27.com>
References: <20230727141428.962286-1-alexghiti@rivosinc.com>
 <20230727141428.962286-10-alexghiti@rivosinc.com>
 <CAP-5=fU5XYXrVnRUidpwjV2LiTsdebfidL43_Qo4Z7TBxMsVGA@mail.gmail.com>
 <CAHVXubgVAe1WsiZx5Ay+3KPK4u24k_vsnTwFFmBeVsHrGXwhfw@mail.gmail.com>
 <CAHVXubj80rQRShuDS09BeTrfR6nux0A68EMWLbeat8fd_Y3YdA@mail.gmail.com>
 <CAP-5=fWwzuGZ6a6Z38ndsb7gw7_uwS0a2VGXx08hMeiK8eZ91w@mail.gmail.com>
 <CAHVXubjhM9C1fw_Us=8+RuSJbW0pacFAk9gp7j2=BtMUPy_Byw@mail.gmail.com>
To:     Alexandre Ghiti <alexghiti@rivosinc.com>
Precedence: bulk

On 31 Jul 2023, at 17:06, Alexandre Ghiti <alexghiti@rivosinc.com> =
wrote:
>=20
> On Mon, Jul 31, 2023 at 5:10=E2=80=AFPM Ian Rogers =
<irogers@google.com> wrote:
>>=20
>> On Mon, Jul 31, 2023 at 3:27=E2=80=AFAM Alexandre Ghiti =
<alexghiti@rivosinc.com> wrote:
>>>=20
>>> On Mon, Jul 31, 2023 at 12:15=E2=80=AFPM Alexandre Ghiti =
<alexghiti@rivosinc.com> wrote:
>>>>=20
>>>> Hi Ian,
>>>>=20
>>>> On Fri, Jul 28, 2023 at 7:53=E2=80=AFPM Ian Rogers =
<irogers@google.com> wrote:
>>>>>=20
>>>>> On Thu, Jul 27, 2023 at 7:28=E2=80=AFAM Alexandre Ghiti =
<alexghiti@rivosinc.com> wrote:
>>>>>>=20
>>>>>> riscv now supports mmaping hardware counters so add what's needed =
to
>>>>>> take advantage of that in libperf.
>>>>>>=20
>>>>>> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>>>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>>>>> Reviewed-by: Atish Patra <atishp@rivosinc.com>
>>>>>> ---
>>>>>> tools/lib/perf/mmap.c | 65 =
+++++++++++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 65 insertions(+)
>>>>>>=20
>>>>>> 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)
>>>>>>=20
>>>>>> static u64 read_timestamp(void) { return read_sysreg(cntvct_el0); =
}
>>>>>>=20
>>>>>> +#elif __riscv_xlen =3D=3D 64
>>>>>=20
>>>>> This is something of an odd guard, perhaps:
>>>>> #elif defined(__riscv) && __riscv_xlen =3D=3D 64
>>>>>=20
>>>>> That way it is more intention revealing that this is riscv code. =
Could
>>>>> you add a comment relating to the __riscv_xlen ?
>>>>=20
>>>> I guess Andrew answered that already.
>>>>=20
>>=20
>> Not sure. I still think it looks weird:
>> ...
>> #if defined(__i386__) || defined(__x86_64__)
>> ...
>> #elif defined(__aarch64__)
>> ...
>> #elif __riscv_xlen =3D=3D 64
>> ...
>> #else
>> static u64 read_perf_counter(unsigned int counter __maybe_unused) { =
return 0; }
>> static u64 read_timestamp(void) { return 0; }
>> #endif
>>=20
>> The first two are clearly #ifdef-ing architecture specific assembly
>> code, under what conditions I get RISC-V code  =C2=AF\(=E3=83=84)/=C2=AF=
 At least worth
>> a comment like "csrr is only available when you have xlens of 64
>> because ..."
>=20
> __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=E2=80=99s 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