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

[Yu-Chien Peter Lin <peterlin@andestech.com>]

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.

In my opinion, this is simply a pre-sepc solution to enable missing perf
features before the standard is finalized. 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.

> > Thanks,
> > Conor.
> > 
> > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com>
> > > Signed-off-by: Locus Wei-Han Chen <locus84@andestech.com>
> > 
> > btw, what did Locus Wei-Han Chen do here? Are you missing
> > a Co-developed-by: tag?

Yes I missed the CD-tag, will fix it.
Thanks for the review.

> > > Reviewed-by: Charles Ci-Jyun Wu <dminus@andestech.com>
> > > Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
> > > ---
> > >  arch/riscv/Kconfig.errata            | 13 ++++++++
> > >  arch/riscv/errata/andes/errata.c     | 45 +++++++++++++++++++++++++++-
> > >  arch/riscv/include/asm/errata_list.h | 43 ++++++++++++++++++++++++--
> > >  drivers/perf/riscv_pmu_sbi.c         | 20 +++++++++----
> > >  4 files changed, 111 insertions(+), 10 deletions(-)

Best regards,
Peter Lin