From nobody Tue Aug 30 21:58:09 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4MHLls6fzvz4bQQX for ; Tue, 30 Aug 2022 21:58:17 +0000 (UTC) (envelope-from tsoome@me.com) Received: from pv50p00im-hyfv10021501.me.com (pv50p00im-hyfv10021501.me.com [17.58.6.48]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4MHLlr4pTFz3d7H for ; Tue, 30 Aug 2022 21:58:16 +0000 (UTC) (envelope-from tsoome@me.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=me.com; s=1a1hai; t=1661896695; bh=4r4dlJIiwjf1hIneqb9elCftnXvA2QbxaQFW/WTveBM=; h=From:Message-Id:Content-Type:Mime-Version:Subject:Date:To; b=tXMVr6jWRh7rKeT/awzrkitfl/fHAaEyDmfHDNdqNNdXAZTcAjACy9LWMySTXq4MZ jbx77Z7ePXDy4Pt01sBl2wt85TGoOJzEEq1WW3MLEdY2OLK+sfrxBkCX9tc05Up5iC ct/mOKyZlqCfMxcTIVni/WPr+eII6Jf4bB1bDxTIdkDcuNZ+y8QdIoG3EbZCulXCtZ psubb0U5Q3IDs0I5wG7WX3gmrH1ADZYtXCAylQVymahP9tWyhOG4iJHtOAOUXDAj1R /dnQn1YN6n11g5xznlvu9+r1tnJiSOQ7nCZ/rS92k0ijco9Iqxp6BPyXlTdfVa8ayM 925JA8uQ35P3Q== Received: from smtpclient.apple (pv50p00im-dlb-asmtp-mailmevip.me.com [17.56.9.10]) by pv50p00im-hyfv10021501.me.com (Postfix) with ESMTPSA id 75CB82C00B6; Tue, 30 Aug 2022 21:58:12 +0000 (UTC) From: Toomas Soome Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_93551B2D-A9AF-4F83-9150-37ADBCCC9325" List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\)) 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. Date: Wed, 31 Aug 2022 00:58:09 +0300 In-Reply-To: Cc: Jessica Clarke , Toomas Soome , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" To: "Bjoern A. Zeeb" References: <202206262217.25QMHOuH076130@gitrepo.freebsd.org> <1E37449E-B6C8-47A5-AD79-34F24138CC64@freebsd.org> X-Mailer: Apple Mail (2.3696.120.41.1.1) X-Proofpoint-GUID: ShxHEaDmrinxO1zb6RqBKEahaPxAZgRR X-Proofpoint-ORIG-GUID: ShxHEaDmrinxO1zb6RqBKEahaPxAZgRR X-Proofpoint-Virus-Version: =?UTF-8?Q?vendor=3Dfsecure_engine=3D1.1.170-22c6f66c430a71ce266a39bfe25bc?= =?UTF-8?Q?2903e8d5c8f:6.0.138,18.0.572,17.11.64.514.0000000_definitions?= =?UTF-8?Q?=3D2020-02-14=5F11:2020-02-14=5F02,2020-02-14=5F11,2022-02-23?= =?UTF-8?Q?=5F01_signatures=3D0?= X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=999 phishscore=0 mlxscore=0 spamscore=0 suspectscore=0 malwarescore=0 bulkscore=0 clxscore=1011 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000 definitions=main-2208300098 X-Rspamd-Queue-Id: 4MHLlr4pTFz3d7H X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=me.com header.s=1a1hai header.b=tXMVr6jW; dmarc=pass (policy=quarantine) header.from=me.com; spf=pass (mx1.freebsd.org: domain of tsoome@me.com designates 17.58.6.48 as permitted sender) smtp.mailfrom=tsoome@me.com X-Spamd-Result: default: False [-3.60 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_MEDIUM(-1.00)[-0.999]; NEURAL_HAM_SHORT(-1.00)[-0.998]; MV_CASE(0.50)[]; DMARC_POLICY_ALLOW(-0.50)[me.com,quarantine]; R_DKIM_ALLOW(-0.20)[me.com:s=1a1hai]; R_SPF_ALLOW(-0.20)[+ip4:17.58.0.0/16]; RCVD_IN_DNSWL_LOW(-0.10)[17.58.6.48:from]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; FREEFALL_USER(0.00)[tsoome]; TO_DN_EQ_ADDR_SOME(0.00)[]; FROM_HAS_DN(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; ARC_NA(0.00)[]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org]; DWL_DNSWL_NONE(0.00)[me.com:dkim]; FREEMAIL_ENVFROM(0.00)[me.com]; ASN(0.00)[asn:714, ipnet:17.58.0.0/20, country:US]; FREEMAIL_FROM(0.00)[me.com]; TO_DN_SOME(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; RCPT_COUNT_FIVE(0.00)[6]; FROM_EQ_ENVFROM(0.00)[]; DKIM_TRACE(0.00)[me.com:+]; RCVD_COUNT_TWO(0.00)[2]; MIME_TRACE(0.00)[0:+,1:+,2:~]; RCVD_TLS_ALL(0.00)[] X-ThisMailContainsUnwantedMimeParts: N --Apple-Mail=_93551B2D-A9AF-4F83-9150-37ADBCCC9325 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 31. Aug 2022, at 00:51, Bjoern A. Zeeb wrote: >=20 > On Tue, 30 Aug 2022, Jessica Clarke wrote: >=20 > Hi Jessica, >=20 >> On 27 Jun 2022, at 01:58, Bjoern A. Zeeb wrote: >>>=20 >>> On Mon, 27 Jun 2022, Jessica Clarke wrote: >>>=20 >>> Hi, >>>=20 >>>> On 27 Jun 2022, at 01:26, Bjoern A. Zeeb wrote: >>>>>=20 >>>>> On Mon, 27 Jun 2022, Jessica Clarke wrote: >>>>>=20 >>>>>> On 26 Jun 2022, at 23:17, Toomas Soome = wrote: >>>>>>>=20 >>>>>>> The branch main has been updated by tsoome: >>>>>>>=20 >>>>>>> URL: = https://cgit.FreeBSD.org/src/commit/?id=3De3572eb654733a94e1e765fe9e95e057= 9981d851 >>>>>>>=20 >>>>>>> commit e3572eb654733a94e1e765fe9e95e0579981d851 >>>>>>> Author: Aleksandr Rybalko >>>>>>> AuthorDate: 2022-02-16 00:19:19 +0000 >>>>>>> Commit: Toomas Soome >>>>>>> CommitDate: 2022-06-26 18:52:26 +0000 >>>>>>>=20 >>>>>>> Allocate event for DMC-620 and CMN-600 controllers PMU. Add = events supported by DMC-620 and CMN-600 controllers PMU. >>>>>>>=20 >>>>>>> Allocate event for DMC-620 and CMN-600 controllers PMU. >>>>>>> Add events supported by DMC-620 and CMN-600 controllers PMU. >>>>>>>=20 >>>>>>> Reviewed by: bz >>>>>>> Sponsored By: ARM >>>>>>> Sponsored By: Ampere Computing >>>>>>> Differential Revision: https://reviews.freebsd.org/D35609 >>>>>>=20 >>>>>> This includes the following (skipped due to lines) diff: >>>>>>=20 >>>>>>> * 0x14100 0x0100 ARMv8 events >>>>>>> + * 0x14200 0x0020 ARM DMC-620 clkdiv2 events >>>>>>> + * 0x14220 0x0080 ARM DMC-620 clk events >>>>>>> + * 0x14300 0x0100 ARM CMN-600 events >>>>>>=20 >>>>>>=20 >>>>>> 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=E2=80=99s = events. >>>>>> Please relocate these new events well past the end of the = existing >>>>>> Armv8 events so the space can remain contiguous. >>>>>=20 >>>>> Should this be 0x3ff then as well btw? >>>>> = https://github.com/CTSRD-CHERI/cheribsd/commit/4ea869cd8b717ca0b07672eb7ac= c99bf949249de >>>>=20 >>>> 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). >>>>=20 >>>>> 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? >>>>=20 >>>> Yes, if you want to cover all the v8.1 space then you need to go = that >>>> high too, but it=E2=80=99ll get quite sparse in that range so = it=E2=80=99s 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. >>>=20 >>> 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. >>>=20 >>> 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. >>=20 >> Nobody ever got round to addressing this, and it is in fact causing = us >> issues downstream now. However, there=E2=80=99s a rather more glaring = problem: >>=20 >>> @@ -1313,6 +1475,10 @@ pmc_init(void) >>>=20 >>> /* Fill soft events information. */ >>> pmc_class_table[n++] =3D &soft_class_table_descr; >>> + >>> + pmc_class_table[n++] =3D &cmn600_pmu_class_table_descr; >>> + pmc_class_table[n++] =3D &dmc620_pmu_cd2_class_table_descr; >>> + pmc_class_table[n++] =3D &dmc620_pmu_c_class_table_descr; >>=20 >> This doesn=E2=80=99t 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. >=20 > I am just replying really given I am on Cc: hoping that Toomas will = get to this. >=20 > 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? >=20 > 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. >=20 Yea, I was also thinking the same. Of course, we probably need to do the = same about event range and thats ugly=E2=80=A6=20 Anyhow, I would need to look on those bits with fresh head (it is = midnight here), but I=E2=80=99m all ears for suggestions:) rgds, toomas >=20 >> Given >> this has broken libpmc on large swathes of AArch64 hardware (maybe >=20 > That has taken a lot of time for anyone to notice :( >=20 >> 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? >>=20 >> Note that cmn600 only has an ACPI attachment so FDT-based systems = will >> definitely hit this case. >>=20 >> Jess >>=20 >>=20 >=20 > --=20 > Bjoern A. Zeeb = r15:7 --Apple-Mail=_93551B2D-A9AF-4F83-9150-37ADBCCC9325 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On 31. Aug 2022, at 00:51, Bjoern A. Zeeb <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=3De3572eb654733a94e1e76= 5fe9e95e0579981d851

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=E2=80=99s 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/4ea869cd8b717ca0= b07672eb7acc99bf949249de

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=E2=80=99= ll get quite sparse in that range so it=E2=80=99s 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=E2=80=99s a rather more glaring = problem:

@@ -1313,6 +1475,10 @@ pmc_init(void)

= /* Fill soft events information. */
= pmc_class_table[n++] =3D &soft_class_table_descr;
+
+ pmc_class_table[n++] =3D = &cmn600_pmu_class_table_descr;
+ = pmc_class_table[n++] =3D = &dmc620_pmu_cd2_class_table_descr;
+ = pmc_class_table[n++] =3D &dmc620_pmu_c_class_table_descr;

This doesn=E2=80=99t 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.



Yea, I was also thinking the same. Of course, we = probably need to do the same about event range and thats = ugly=E2=80=A6 

Anyhow, I would = need to look on those bits with fresh head (it is midnight here), but = I=E2=80=99m all ears for suggestions:)

rgds,
toomas


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 =             &n= bsp;           &nbs= p;            =             &n= bsp;  r15:7

= --Apple-Mail=_93551B2D-A9AF-4F83-9150-37ADBCCC9325--