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

[Inochi Amaoto]

On Thu, Mar 14, 2024 at 08:41:15PM +0000, Conor Dooley wrote:
> 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.
> 

I agree with that not implement standard extension is not a bug. 
And it is necessary to let userspace know if the core have behavior 
that is different from the standard. In fact, I think it may need 
a different name to describe this difference, "errata" is kind of 
unsuitable.

> > 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.

As the cpus evolve, it may have so many different bug. This is why I say
it may need infinite bits. In fact, it is just a extreme case.

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

Instead of reusing, it may be better to store them separately. But if
we store them separately, why not to use a different DT property to 
describe them? This is what I want to say.

> > 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.