From nobody Tue Aug 30 22:05:31 2022 X-Original-To: dev-commits-src-main@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 4MHLwG55m9z4bQv0 for ; Tue, 30 Aug 2022 22:05:34 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4MHLwG19vqz3db3 for ; Tue, 30 Aug 2022 22:05:34 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: by mail-wr1-f43.google.com with SMTP id e20so15919540wri.13 for ; Tue, 30 Aug 2022 15:05:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-message-state:from:to:cc; bh=iJ2qahycHHqKmUHbcdoKdqZd/W8jOwc07DG5eoJ5gTI=; b=UcUpdLehNWCZYltZGgLycq0dBHgDg87cmyuea1dVLJwurl4pqrAXZBJ802927+DKhk GHMZa02Uw/1js6s52zh7gAH/XvK7UtL+6eLUM/VYSiH/HIWoI72crnIzH4gsIqIniHfx /vRIlXTir/AI/nuEJPpxvI9OGA9C0UBKqapXKGko6LphUPSrSkxZckLtLUPtLcWGBQxL BGf3WVg3H8zwPNyRwL8GFCWyThKLLG5ze7OoqLToUEdmCk4gkO0EJev4hO+in7CoqWQ6 CWsmQXd70FQbXtJ8UnkMMqteWBRUHk//kjPhuI1IwiunSjw682iItJAzTNeDWEhY3H5L GLHg== X-Gm-Message-State: ACgBeo2mvPfvclfwJPv8eHM1GYDZSmZN+35ivXGS75KDeE7ydao3Fnin cAeFB4wQL1DuNQ1E5FivnlomXw== X-Google-Smtp-Source: AA6agR7bhXM88WGalq/hl4mlZ/XxtWAgXllCck9qks/dicXb0pAkcUZQnNzcIs6HeU78fX5EnlYSJQ== X-Received: by 2002:a5d:64cd:0:b0:226:e332:3d1 with SMTP id f13-20020a5d64cd000000b00226e33203d1mr3028495wri.253.1661897132298; Tue, 30 Aug 2022 15:05:32 -0700 (PDT) Received: from smtpclient.apple (global-5-141.n-2.net.cam.ac.uk. [131.111.5.141]) by smtp.gmail.com with ESMTPSA id n19-20020a05600c3b9300b003a846a014c1sm120338wms.23.2022.08.30.15.05.31 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 30 Aug 2022 15:05:31 -0700 (PDT) Content-Type: text/plain; charset=utf-8 List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.80.82.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. From: Jessica Clarke In-Reply-To: Date: Tue, 30 Aug 2022 23:05:31 +0100 Cc: Toomas Soome , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <0F57FEEE-BDC0-4CE1-9349-D7E12556E158@freebsd.org> References: <202206262217.25QMHOuH076130@gitrepo.freebsd.org> <1E37449E-B6C8-47A5-AD79-34F24138CC64@freebsd.org> To: "Bjoern A. Zeeb" X-Mailer: Apple Mail (2.3696.80.82.1.1) X-Rspamd-Queue-Id: 4MHLwG19vqz3db3 X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=pass (mx1.freebsd.org: domain of jrtc27@jrtc27.com designates 209.85.221.43 as permitted sender) smtp.mailfrom=jrtc27@jrtc27.com X-Spamd-Result: default: False [-2.50 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_SHORT(-1.00)[-1.000]; MV_CASE(0.50)[]; FORGED_SENDER(0.30)[jrtc27@freebsd.org,jrtc27@jrtc27.com]; R_SPF_ALLOW(-0.20)[+ip4:209.85.128.0/17:c]; MIME_GOOD(-0.10)[text/plain]; MIME_TRACE(0.00)[0:+]; FROM_HAS_DN(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DMARC_NA(0.00)[freebsd.org]; RCVD_IN_DNSWL_NONE(0.00)[209.85.221.43:from]; PREVIOUSLY_DELIVERED(0.00)[dev-commits-src-main@freebsd.org]; FREEFALL_USER(0.00)[jrtc27]; TO_DN_EQ_ADDR_SOME(0.00)[]; FROM_NEQ_ENVFROM(0.00)[jrtc27@freebsd.org,jrtc27@jrtc27.com]; MLMMJ_DEST(0.00)[dev-commits-src-main@freebsd.org]; RCPT_COUNT_FIVE(0.00)[5]; TO_DN_SOME(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; ARC_NA(0.00)[]; RCVD_COUNT_THREE(0.00)[3]; R_DKIM_NA(0.00)[]; RCVD_TLS_LAST(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; RWL_MAILSPIKE_POSSIBLE(0.00)[209.85.221.43:from] X-ThisMailContainsUnwantedMimeParts: N On 30 Aug 2022, at 22: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. 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=E2=80=99s 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 >=20 > 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=E2=80=99s totally broken = for PIEs. People just aren=E2=80=99t using pmc much, especially on !x86=E2=80=A6 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? >>=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