Date: Tue, 30 Aug 2022 21:51:26 +0000 (UTC) From: "Bjoern A. Zeeb" <bz@FreeBSD.org> To: Jessica Clarke <jrtc27@freebsd.org> Cc: Toomas Soome <tsoome@FreeBSD.org>, "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: 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. Message-ID: <r9ro55p-7279-6s1s-q8rq-3q5r589768sq@SerrOFQ.bet> In-Reply-To: <1E37449E-B6C8-47A5-AD79-34F24138CC64@freebsd.org> References: <202206262217.25QMHOuH076130@gitrepo.freebsd.org> <F0C6ECC7-E479-4FA8-ADAE-7229E045D771@freebsd.org> <alpine.BSF.2.00.2206262336110.68830@ai.fobar.qr> <CE2DC7A3-CB6B-4AC4-9F62-07302D501B61@freebsd.org> <alpine.BSF.2.00.2206270053420.68830@ai.fobar.qr> <1E37449E-B6C8-47A5-AD79-34F24138CC64@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --1098556516-278802634-1661896286=:94196 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8BIT 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 --1098556516-278802634-1661896286=:94196--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?r9ro55p-7279-6s1s-q8rq-3q5r589768sq>