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

[Samuel Holland <samuel@sholland.org>]

On 10/23/23 03:26, Conor Dooley wrote:
> 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.

PMU support is not required to boot, and it didn't really work correctly anyway
until OpenSBI commit c9a296d0edc9 ("platform: generic: allwinner: fix OF process
for T-HEAD c9xx pmu"), which is still not in any released OpenSBI version.

So I am fine with requiring a devicetree update for continued PMU support.

Regards,
Samuel