Re: [PATCH] perf: RISC-V: fix IRQ detection on T-Head C908

[Conor Dooley]

On Wed, Mar 13, 2024 at 09:31:26AM +0800, Inochi Amaoto wrote:
> On Tue, Mar 12, 2024 at 02:07:31PM +0000, Conor Dooley wrote:
> > On Mon, Mar 11, 2024 at 03:56:29PM +0800, Qingfang Deng wrote:
> > > Hi Inochi,
> > > 
> > > On Mon, Mar 11, 2024 at 3:13 PM Inochi Amaoto <inochiama@xxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Mar 11, 2024 at 02:30:18PM +0800, Qingfang Deng wrote:
> > > > > T-Head C908 has the same IRQ num and CSR as previous C9xx cores, but
> > > > > reports non-zero marchid and mimpid. Remove the ID checks.
> > > > >
> > > >
> > > > Hi, Qingfang,
> > > >
> > > > IIRC, the existed C908 SoC (such as K230) have an early version
> > > > of C908 core. But C908 core itself may support Sscofpmf.
> > > > So I do not think removing the ID checks is a good idea. Instead,
> > > > I suggest adding CPUID of your SoC to this check.
> > > 
> > > As of Feb 2024, the latest C908 revision does not support Sscofpmf.
> > > You may Google "C908R1S0" to see its user manual.
> > > But I think you're right. Even though C908 does not have Sscofpmf,
> > > T-Head may release new SoCs which do have Sscofpmf, and the check will
> > > break. I will submit a new patch with your suggested changes.
> > 
> > If on an SoC where they have updated vector to 1.0 and implemented both
> > Zicbom and Svpbmt instead of their custom stuff they did not implement
> > Sscofpmf I think we can expect they won't move away from their custom
> > implementation soon.
> > I do agree that we should not remove the ID checks entirely, but I also
> > do not want to be adding an ID for every SoC that needs this. I think we
> > should be getting this information from DT going forward.
> > The DT parsing is done prior to the application of boot time
> > alternatives, so I think we could apply the "erratum" based on the DT.
> > 
> > I'm also pretty sure that we can also modify the existing code for the
> > archid == impid == 0x0 case to set a pseudo isa extension so that the
> > perf driver could do call riscv_isa_eextension_available() and not worry
> > about the specfic conditions in which that is true. It'd be something
> > like this patch:
> > https://lore.kernel.org/linux-riscv/20240110073917.2398826-8-peterlin@xxxxxxxxxxxxx/
> > Just without removing the archid == impid == 0x0 case from the errata
> > code. If you're lost after reading that, I can probably throw together
> > some untested code for it.
> 
> I agree to use something to replace the existing check, but using a pseudo
> isa extension is not a good idea. There are two reasons: 
> 
> 1. Pseudo isa is misleading. As it is not the real isa, setting this in isa
> list may make userspace think errata a feather.

If we wanted to suppress sharing this information to userspace we could,
but I don't even see what the harm is in userspace knowing.
I also disagree with this even really being an erratum in the first place
as they implemented something before a spec was created. It's not an
implementation of Sscofpmf with a bug. I don't think that calling it a
"real" vendor extension is problematic either, just I am the one calling
it that, not T-Head themselves.

> 2. Using pseudo isa is more like an abuse of reserved isa bits, which means
> kernel may need infinite bits to handle the errata.

What makes it an abuse of a "reserved" bit? There's no rule on
disallowing something vendor-specific there. Given the amount of standard
ISA extensions that are being created compared to CPU errata, I don't
think that I'm particularly worried about vendor specific stuff being
the reason for infinite bits being needed either.

If there do come to be issues with the number of extensions we care
about, we could split things per vendor I suppose.

> IMHO, it may be better to use a new DT property like "riscv,cpu-errata" or
> "<vendor>,cpu-errata". It can achieve almost everything like using pseudo
> isa. And the only cost I think is a small amount code to parse this.

I suppose we could do that, but accounting for vendor specifics was one
of the goals for the property I only just added and that I am suggesting
to use here.

Attachment: signature.asc
Description: PGP signature