On Tue, 30 Aug 2022, Jessica Clarke wrote:
Hi Jessica,
On 27 Jun 2022, at 01:58, Bjoern A. Zeeb <b...@freebsd.org> wrote:
On Mon, 27 Jun 2022, Jessica Clarke wrote:
Hi,
On 27 Jun 2022, at 01:26, Bjoern A. Zeeb <b...@freebsd.org> wrote:
On Mon, 27 Jun 2022, Jessica Clarke wrote:
On 26 Jun 2022, at 23:17, Toomas Soome <tso...@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 <r...@freebsd.org>
AuthorDate: 2022-02-16 00:19:19 +0000
Commit: Toomas Soome <tso...@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
