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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <IA1PR20MB4953CE8999960BA71B46DE6CBB2A2@IA1PR20MB4953.namprd20.prod.outlook.com>
Date: Wed, 13 Mar 2024 09:31:26 +0800
From: Inochi Amaoto <inochiama@...look.com>
To: Conor Dooley <conor@...nel.org>, Qingfang Deng <dqfext@...il.com>
Cc: Inochi Amaoto <inochiama@...look.com>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Atish Patra <atishp@...shpatra.org>, 
	Anup Patel <anup@...infault.org>, Will Deacon <will@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, Andrew Jones <ajones@...tanamicro.com>, 
	Conor Dooley <conor.dooley@...rochip.com>, Heiko Stuebner <heiko@...ech.de>, Guo Ren <guoren@...nel.org>, 
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] perf: RISC-V: fix IRQ detection on T-Head C908

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@...look.com> 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@andestech.com/
> 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.