linux-kernel - Re: [PATCH 3/4] riscv: errata: Add Andes PMU errata

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

Date:   Mon, 11 Sep 2023 13:35:21 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Yu-Chien Peter Lin <peterlin@...estech.com>
CC:     Conor Dooley <conor@...nel.org>, <linux-riscv@...ts.infradead.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-perf-users@...r.kernel.org>,
        <paul.walmsley@...ive.com>, <palmer@...belt.com>,
        <aou@...s.berkeley.edu>, <atishp@...shpatra.org>,
        <anup@...infault.org>, <prabhakar.mahadev-lad.rj@...renesas.com>,
        <ajones@...tanamicro.com>, <heiko@...ech.de>,
        <samuel@...lland.org>, <geert+renesas@...der.be>,
        <n.shubin@...ro.com>, <dminus@...estech.com>,
        <ycliang@...estech.com>, <tim609@...estech.com>,
        <locus84@...estech.com>, <dylan@...estech.com>
Subject: Re: [PATCH 3/4] riscv: errata: Add Andes PMU errata

On Mon, Sep 11, 2023 at 10:48:44AM +0800, Yu-Chien Peter Lin wrote:
> On Thu, Sep 07, 2023 at 12:02:46PM +0100, Conor Dooley wrote:
> > On Thu, Sep 07, 2023 at 10:27:03AM +0100, Conor Dooley wrote:
> > > Hey,
> > > 
> > > On Thu, Sep 07, 2023 at 10:16:34AM +0800, Yu Chien Peter Lin wrote:
> > > > Before the ratification of Sscofpmf, the Andes PMU extension
> > > > implements the same mechanism and is compatible with existing
> > > > SBI PMU driver of perf to support event sampling and mode
> > > > filtering with programmable hardware performance counters.
> > > 
> > > If it actually was, you'd not need to modify the driver ;)
> > > 
> > > > This patch adds PMU support for Andes 45-series CPUs by
> > > > introducing a CPU errata.
> > > 
> > > I don't really understand this in all honesty. You don't have Sscofpmf
> > > support with a bug, you have something that is Sscofpmf-adjactent that
> > > predates it. Why claim to support an extension that you do not, only to
> > > have to come along and try to clean things up afterwards, instead of
> > > accurately declaring what you do support from the outset?
> > 
> > Reading this again, I don't think that I have been particularly clear,
> > sorry. My point is that this is not a fix for a bug in your
> > implementation of Sscofpmf, but rather adding probing for what is
> > effectively a custom ISA extension that happens to be very similar to
> > the standard one. As it is not an implementation bug, errata should
> > not be abused to support vendor extensions, and either DT or ACPI should
> > be used to inform the operating system about its presence.
> > 
> > Cheers,
> > Conor.
> > 
> > > 
> > > (and just because someone already got away with it, doesn't mean that
> > > you get a free pass on it, sorry)
> 
> Apologize for any confusion caused by the name. I thought it would make it
> easier to find the related functions and files in OpenSBI, didn't expect
> that it may have misled people to abuse the use of errata, you are right,
> I should have chosen my words more carefully.

I'm not sure what you mean here. The commit message is not the problem,
nor is the comparison in it to Sscofpmf - my issue is with pretending
that things like this are errata and using mvendorid etc to probe them.

> In my opinion, this is simply a pre-sepc solution to enable missing perf
> features before the standard is finalized.

Right, I don't think there's any disagreement about that. What I am
objecting to is adding more cases of pretending that something never
intended to be the standard extension is the standard extension with an
erratum. I know the T-Head stuff does do exactly this, but I don't think
we should continue to allow feature probing using the errata framework,
but instead communicate supported features via DT or ACPI. (If I could
re-review the T-Head PMU patch now, I'd say the same thing there).
Maybe Palmer et al disagree & there is some extenuating circumstance
that applies here.

> The underlying logic remains the
> same, so we can still use the errata to patch a few lines of CSR accesses
> and align with other vendors. This way, we can make minimal changes and
> avoid performance overhead to the driver.

I think you're conflating errata with alternatives. I have no problem
with the use of alternatives for this purpose & the perf parts of the
patch are okay to me, if the overhead of having a platform specific ops
struct is unacceptable.

Thanks,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)