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

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1: Type: text/plain, Size: 3523 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]