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.
Date: Wed, 31 Aug 2022 11:15:36 UTC
Please see https://reviews.freebsd.org/D36401 <https://reviews.freebsd.org/D36401> thanks, toomas > On 31. Aug 2022, at 01:05, Jessica Clarke <jrtc27@freebsd.org> wrote: > > On 30 Aug 2022, at 22:51, Bjoern A. Zeeb <bz@FreeBSD.org <mailto:bz@FreeBSD.org>> wrote: >> >> 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. > > The only commit to libpmc.c since then was to add the #ifdef. I believe > the kernel bits can stay, though the event numbers still collide with > what should have been reserved for Armv8-A’s full range, and what we > reserve downstream as we have hardware that uses those events for > documented counters. > >>> Given >>> this has broken libpmc on large swathes of AArch64 hardware (maybe >> >> That has taken a lot of time for anyone to notice :( > > It also took a long time for anyone to notice how broken libpmcstat is, > and only a handful of people have noticed it’s totally broken for PIEs. > People just aren’t using pmc much, especially on !x86… > > Jess > >>> 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