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

[Inochi Amaoto]

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.
> 
> Thanks,
> Conor.

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.
2. Using pseudo isa is more like an abuse of reserved isa bits, which means
kernel may need infinite bits to handle the errata. 

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.

Regards,
Inochi.