Re: [RFC PATCH v2 07/10] perf: RISC-V: Move T-Head PMU to CPU feature alternative framework

[Conor Dooley]

On Sun, Oct 22, 2023 at 05:09:09PM +0800, Yu-Chien Peter Lin wrote:
> On Fri, Oct 20, 2023 at 10:05:20AM +0100, Conor Dooley wrote:
> > On Fri, Oct 20, 2023 at 04:54:58PM +0800, Yu-Chien Peter Lin wrote:
> > > On Thu, Oct 19, 2023 at 05:13:00PM +0100, Conor Dooley wrote:
> > > > On Thu, Oct 19, 2023 at 10:01:19PM +0800, Yu Chien Peter Lin wrote:
> > > > 
> > > > $subject: perf: RISC-V: Move T-Head PMU to CPU feature alternative framework
> > > > 
> > > > IMO, this should be "RISC-V, perf:" or just "RISC-V" as the changes
> > > > being made to the arch code are far more meaningful than those
> > > > elsewhere.
> > > 
> > > OK will update the subject to "RISC-V:"
> > > 
> > > > > The custom PMU extension was developed to support perf event sampling
> > > > > prior to the ratification of Sscofpmf. Instead of utilizing the standard
> > > > > bits and CSR of Sscofpmf, a set of custom CSRs is added. So we may
> > > > > consider it as a CPU feature rather than an erratum.
> > > > > 
> > > > > T-Head cores need to append "xtheadpmu" to the riscv,isa-extensions
> > > > > for each cpu node in device tree, and enable CONFIG_THEAD_CUSTOM_PMU
> > > > > for proper functioning as of this commit.
> > > > 
> > > > And in doing so, you regress break perf for existing DTs :(
> > > > You didn't add the property to existing DTS in-kernel either, so if this
> > > > series was applied, perf would just entirely stop working, no?
> > > 
> > > Only `perf record/top` stop working I think.
> > > 
> > > There are too many users out there, and don't have the boards to
> > > test, so leave those DTS unchanged, it would be great if T-Head
> > > community could help to check/update their DTS.
> > 
> > So, there are too many users to add xtheadpmu to the devicetrees, but
> > not too many users to make changes that will cause a regression?
> > I'm not following the logic here, sorry.
> 
> humm, I'll try. I assume that the sun20i-d1s.dtsi is all I need
> to update for T-Head PMU.

I think you can actually add it to all users of T-Head CPUs currently in
mainline since all those cpus report the 0 mimpid and 0 marchid that is
being used as the detection method in the current code.

That said, changing the in-kernel devicetrees doesn't solve the
regression problem. Not every dts lives in the linux codebase, for
example, and just because they don't, doesn't mean we can just not
care about them!

As a result, I don't think that we can just do a conversion here from
one method to another like this, since it's likely to break things for
people. Certainly interested in hearing from those that support the
T-Head IP based SoCs about whether they'd be okay with something like
this.

> > > > > Signed-off-by: Yu Chien Peter Lin <peterlin@xxxxxxxxxxxxx>
> > > > > ---
> > > > > Hi All,
> > > > > 
> > > > > This is in preparation for introducing other PMU alternative.
> > > > > We follow Conor's suggestion [1] to use cpu feature alternative
> > > > > framework rather than errta, if you want to stick with errata
> > > > > alternative or have other issues, please let me know. Thanks.
> > > > 
> > > > Personally, I like this conversion, but it is going to regress support
> > > > for perf on any T-Head cores which may be a bitter pill to get people to
> > > > actually accept...
> > > > Perhaps we could add this "improved" detection in parallel, and
> > > > eventually remove the m*id based stuff in the future.
> > > > 
> > > > > [1] https://patchwork.kernel.org/project/linux-riscv/patch/20230907021635.1002738-4-peterlin@xxxxxxxxxxxxx/#25503860
> > > > > 
> > > > > Changes v1 -> v2:
> > > > >   - New patch
> > > > > ---
> > 
> > > > > @@ -805,7 +816,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
> > > > >  	if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
> > > > >  		riscv_pmu_irq_num = RV_IRQ_PMU;
> > > > >  		riscv_pmu_use_irq = true;
> > > > > -	} else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) &&
> > > > > +	} else if (riscv_isa_extension_available(NULL, XTHEADPMU) &&
> > > > > +		   IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU) &&
> > > > >  		   riscv_cached_mvendorid(0) == THEAD_VENDOR_ID &&
> > > > >  		   riscv_cached_marchid(0) == 0 &&
> > > > >  		   riscv_cached_mimpid(0) == 0) {
> > > > 
> > > > Can all of the m*id checks be removed, since the firmware is now
> > > > explicitly telling us that the T-Head PMU is supported?
> > > 
> > > I can only comfirm that boards with "allwinner,sun20i-d1" compatible
> > > string uses the T-Head PMU device callbacks.
> > 
> > I'm not sure how that is an answer to my question.
> 
> Sorry for that unclear answer.
> Yes, I agree we no longer need to check the m*id here.

> In OpenSBI, it appears that allwinner D1 is the only platform that
> has T-Head PMU support, the other T-Head platforms need to ensure
> that the callbacks [1] are registered in order to work with SBI
> PMU driver in kernel.

> [1] https://github.com/riscv-software-src/opensbi/blob/v1.3.1/platform/generic/allwinner/sun20i-d1.c#L263-L272

There may be forks of OpenSBI, or other SBI providers, in use that
configure this correctly for other SoCs with T-Head cores, so while this
is a good indicator, it can't really be taken as gospel. Although, the
T-Head vendor fork can be ignored, as that doesn't even seem to be
capable of booting mainline kernels, given recent issues with the th1520
systems.

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature