Re: git: e3572eb65473 - main - Allocate event for DMC-620 and CMN-600 controllers PMU. Add events supported by DMC-620 and CMN-600 controllers PMU.

On Tue, 30 Aug 2022, Jessica Clarke wrote:

Hi Jessica,

> On 27 Jun 2022, at 01:58, Bjoern A. Zeeb <bz@FreeBSD.org> wrote:
>>
>> On Mon, 27 Jun 2022, Jessica Clarke wrote:
>>
>> Hi,
>>
>>> On 27 Jun 2022, at 01:26, Bjoern A. Zeeb <bz@FreeBSD.org> wrote:
>>>>
>>>> On Mon, 27 Jun 2022, Jessica Clarke wrote:
>>>>
>>>>> On 26 Jun 2022, at 23:17, Toomas Soome <tsoome@FreeBSD.org> wrote:
>>>>>>
>>>>>> The branch main has been updated by tsoome:
>>>>>>
>>>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=e3572eb654733a94e1e765fe9e95e0579981d851
>>>>>>
>>>>>> commit e3572eb654733a94e1e765fe9e95e0579981d851
>>>>>> Author: Aleksandr Rybalko <ray@freebsd.org>
>>>>>> AuthorDate: 2022-02-16 00:19:19 +0000
>>>>>> Commit: Toomas Soome <tsoome@FreeBSD.org>
>>>>>> CommitDate: 2022-06-26 18:52:26 +0000
>>>>>>
>>>>>> Allocate event for DMC-620 and CMN-600 controllers PMU. Add events supported by DMC-620 and CMN-600 controllers PMU.
>>>>>>
>>>>>> Allocate event for DMC-620 and CMN-600 controllers PMU.
>>>>>> Add events supported by DMC-620 and CMN-600 controllers PMU.
>>>>>>
>>>>>> Reviewed by: bz
>>>>>> Sponsored By: ARM
>>>>>> Sponsored By: Ampere Computing
>>>>>> Differential Revision: https://reviews.freebsd.org/D35609
>>>>>
>>>>> This includes the following (skipped due to lines) diff:
>>>>>
>>>>>> * 0x14100	0x0100		ARMv8 events
>>>>>> + * 0x14200	0x0020		ARM DMC-620 clkdiv2 events
>>>>>> + * 0x14220	0x0080		ARM DMC-620 clk events
>>>>>> + * 0x14300	0x0100		ARM CMN-600 events
>>>>>
>>>>>
>>>>> Not enough space was allocated for Armv8 events as it goes up to 0x3ff
>>>>> in Armv8 (and beyond in later versions of the architecture). Downstream
>>>>> we extend this range in CheriBSD as required for Morello’s events.
>>>>> Please relocate these new events well past the end of the existing
>>>>> Armv8 events so the space can remain contiguous.
>>>>
>>>> Should this be 0x3ff then as well btw?
>>>> https://github.com/CTSRD-CHERI/cheribsd/commit/4ea869cd8b717ca0b07672eb7acc99bf949249de
>>>
>>> Well, 0x400 for count not max, but yes. We only extended as far as we
>>> needed, not to cover the entire range (but intended to eventually
>>> upstream it as the full v8 range).
>>>
>>>> Looking more closely it seems from ARMv8.1 onwards it goes up to 0xFFFF
>>>> if I read 'Table D8-7 Allocation of the PMU event number space' of ARM
>>>> DDI 0487H.a correctly?
>>>
>>> Yes, if you want to cover all the v8.1 space then you need to go that
>>> high too, but it’ll get quite sparse in that range so it’s unclear if
>>> we want to go ahead and do that already or try and be smarter (the
>>> current EVENT_xH list would get a bit silly). We should probably
>>> reserve all of it though at least so we can if we want to in future.
>>
>> I'll let you and Toomas sort that out. I am just trying to fix the
>> build breakage as I kind-of pushed him to get the remaining bits in
>> by accepting that review after scrolling through and it looking
>> reasonable and addressing all comments from the previous review.
>> That was all to unbreak an already earlier build breakage.
>>
>> Given it wasn't too late for me I was trying to get through it
>> before falling asleep soon as well, especially now that the
>> thunderstorms seems to have mostly passed.
>
> Nobody ever got round to addressing this, and it is in fact causing us
> issues downstream now. However, there’s a rather more glaring problem:
>
>> @@ -1313,6 +1475,10 @@ pmc_init(void)
>>
>> 	/* Fill soft events information. */
>> 	pmc_class_table[n++] = &soft_class_table_descr;
>> +
>> +	pmc_class_table[n++] = &cmn600_pmu_class_table_descr;
>> +	pmc_class_table[n++] = &dmc620_pmu_cd2_class_table_descr;
>> +	pmc_class_table[n++] = &dmc620_pmu_c_class_table_descr;
>
> This doesn’t work (even if you ifdef it appropriately like now exists
> upstream). If there is no CMN-600 etc then PMC_CLASS_TABLE_SIZE, i.e.
> cpu_info.pm_nclass, is not going to include those, so you cannot add
> them to pmc_class_table otherwise you have a buffer overflow.

I am just replying really given I am on Cc: hoping that Toomas will get to this.

pmc_init() is libpmc, right?  Does a simple #if 0 around these avert all
issues for now or do the kernel bits also need backing out?

I only have vague memories of multiple commit to unbreak this one from
that night (which tried to fix a previous different breakage).
Backing out everything might be more tedious than just reverting the
commit hence asking if "disabling" it does fix the problems.


> Given
> this has broken libpmc on large swathes of AArch64 hardware (maybe

That has taken a lot of time for anyone to notice :(

> without CHERI the memory corruption happens to not trample over
> anything important for now, but who knows), can we please revert this
> patch until a fixed version exists, with both the event numbers
> reallocated and libpmc made suitably dynamic so as to not introduce
> buffer overflows?
>
> Note that cmn600 only has an ACPI attachment so FDT-based systems will
> definitely hit this case.
>
> Jess
>
>

-- 
Bjoern A. Zeeb                                                     r15:7