linux-kernel - Re: [v4 10/11] riscv: dts: fu740: Add pmu node

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

Message-Id: <EDB030D6-D37A-43D6-9027-222794FDA80D@jrtc27.com>
Date:   Fri, 29 Oct 2021 01:07:02 +0100
From:   Jessica Clarke <jrtc27@...c27.com>
To:     Atish Patra <atishp@...shpatra.org>
Cc:     Atish Patra <atish.patra@....com>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        Anup Patel <anup.patel@....com>,
        David Abdurachmanov <david.abdurachmanov@...ive.com>,
        devicetree <devicetree@...r.kernel.org>,
        Greentime Hu <greentime.hu@...ive.com>,
        Guo Ren <guoren@...ux.alibaba.com>,
        Heinrich Schuchardt <xypron.glpk@....de>,
        Jonathan Corbet <corbet@....net>,
        Linux Doc Mailing List <linux-doc@...r.kernel.org>,
        linux-perf-users@...r.kernel.org,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Nick Kossifidis <mick@....forth.gr>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Rob Herring <robh+dt@...nel.org>,
        Vincent Chen <vincent.chen@...ive.com>
Subject: Re: [v4 10/11] riscv: dts: fu740: Add pmu node

On 29 Oct 2021, at 00:37, Atish Patra <atishp@...shpatra.org> wrote:
> 
> On Thu, Oct 28, 2021 at 1:49 PM Jessica Clarke <jrtc27@...c27.com> wrote:
>> 
>> On Mon, Oct 25, 2021 at 12:53:49PM -0700, Atish Patra wrote:
>>> HiFive unmatched supports HPMCounters but does not implement mcountinhibit
>>> or sscof extension. Thus, perf monitoring can be used on the unmatched
>>> board without sampling.
>>> 
>>> Add the PMU node with compatible string so that Linux perf driver can
>>> utilize this to enable PMU.
>>> 
>>> Signed-off-by: Atish Patra <atish.patra@....com>
>>> ---
>>> arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> 
>>> diff --git a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi
>>> index abbb960f90a0..b35b96b58820 100644
>>> --- a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi
>>> +++ b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi
>>> @@ -140,6 +140,9 @@ soc {
>>>              #size-cells = <2>;
>>>              compatible = "simple-bus";
>>>              ranges;
>>> +             pmu {
>>> +                     compatible = "riscv,pmu";
>>> +             };
>> 
>> This is a property of the user-replaceable firmware, not a property of
>> the hardware,
> 
> It's a property of hardware that indicates that the hardware supports PMU.

All RISC-V hardware provides the CSRs, they’re part of the privileged
spec and not marked optional. How many aren’t hard-wired to zero is up
to the implementation. But even then you can’t know from the hardware
alone what is supported; the firmware has to enable S-mode (and
U-mode)’s ability to read them, so you can’t assume anything in a
static device tree hard-coded in Linux about what firmware has done.
Since you currently have to query the firmware to determine what’s
available to you anyway, I see no benefit from having a node in the
device tree that tells you your firmware *might* have counters you can
use.

> Additionally, the counter overflow interrupt number needs to be
> defined through the DT as well
> so that a clean platform driver can be implemented.

The interrupt number is specified as 13 by the Sscofmpf spec.
But that’s not relevant here, the FU740 predates and doesn’t implement
Sscofmpf, meaning there is no interrupt to even define here. And as I
said on the other patch, don’t conflate “SBI PMU firmware interface is
supported” and “Sscofmpf is implemented in the hardware”; the former
should be discovered by talking to firmware, and the latter should be
discovered like any other extension (however that ends up happening).

>> so having this in the device tree under /soc, let alone
>> hard-coded in Linux, is utterly wrong. Why can this not just be probed
>> like any other SBI interface? The "Probe SBI extension" interface is
>> precisely for this kind of thing.
>> 
> SBI extension is anyways probed to verify if the firmware has PMU
> extension or not.
> However, adding the DT property allows different platforms (with or
> without sscof extension)
> to use the same code path.

You don’t need a device tree for that; that same code path can just be
“use the existing standard firmware interface”. That also has the
benefit that it’s not tied to device tree and so works identically for
ACPI, rather than needing an ACPI version of it.

I see nothing here that can’t be discovered through pre-existing means.
If it can be discovered without use of the device tree then it does not
belong in the device tree; the device tree is purely for things that
cannot otherwise be discovered.

Jess