Messages in this thread | | | Date | Thu, 12 Oct 2023 16:28:28 +0530 | Subject | Re: [PATCH] driver: perf: arm_pmuv3: Read PMMIR_EL1 unconditionally | From | Anshuman Khandual <> |
| |
On 10/12/23 14:54, Mark Rutland wrote: > On Mon, Oct 09, 2023 at 09:59:19AM +0100, James Clark wrote: >> On 09/10/2023 08:56, Anshuman Khandual wrote: >>> PMMIR_EL1 needs to be captured in 'armpmu->reg_pmmir', for all appropriate >>> PMU version implementations where the register is available and reading it >>> is valid . Hence checking for bus slot event presence is redundant and can >>> be dropped. >>> >>> Cc: Will Deacon <will@kernel.org> >>> Cc: Mark Rutland <mark.rutland@arm.com> >>> Cc: linux-arm-kernel@lists.infradead.org >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>> --- >>> This applies on v6.6-rc5. >>> >>> drivers/perf/arm_pmuv3.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c >>> index 1e72b486c033..9fc1b6da5106 100644 >>> --- a/drivers/perf/arm_pmuv3.c >>> +++ b/drivers/perf/arm_pmuv3.c >>> @@ -1129,7 +1129,7 @@ static void __armv8pmu_probe_pmu(void *info) >>> pmceid, ARMV8_PMUV3_MAX_COMMON_EVENTS); >>> >>> /* store PMMIR register for sysfs */ >>> - if (is_pmuv3p4(pmuver) && (pmceid_raw[1] & BIT(31))) >>> + if (is_pmuv3p4(pmuver)) >>> cpu_pmu->reg_pmmir = read_pmmir(); >>> else >>> cpu_pmu->reg_pmmir = 0; >> >> >> This does have the side effect of showing non-zero values in caps/slots >> even when the STALL_SLOT event isn't implemented. I think that's the >> scenario that the original commit (f5be3a61fd) was trying to avoid: >> >> /sys/bus/event_source/devices/armv8_pmuv3_0/caps/slots is exposed >> under sysfs. [If] Both ARMv8.4-PMU and STALL_SLOT event are >> implemented, it returns the slots from PMMIR_EL1, otherwise it will >> return 0. > > We check for the STALL_SLOT event becuase (at the time) the ARM ARM said: > > | If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the > | PMMIR System registers are implemented. > > ... and this was necessary to avoid triggering an UNDEFINED exception if we > attempted to read PMMIR on a CPU which didn't actually implement it. > > See: > > https://lore.kernel.org/linux-arm-kernel/20200720101518.GA11516@willie-the-truck/ > https://lore.kernel.org/linux-arm-kernel/20200720105019.GA54220@C02TD0UTHF1T.local/ > https://lore.kernel.org/linux-arm-kernel/20200720105410.GD11516@willie-the-truck/ > > As I promised in that thread, I did raise that with our architects. According > to the bug I filed against the architecture, this was tightened such that > ARMv8.4-PMU gauaranteed the presence of PMMIR, and that should have changed > between the G.a and G.b releases of the ARM ARM. > > Anshuman, can you go and check that the wording did chaange between G.a and G.b?
G.a was the last ARM ARM version to have that STALL_SLOT event dependency for PMMIR_EL1 system register which got dropped in G.b version ARM ARM.
> > Assuming it did (and the wording in the latest J.a release is also fine), > please update the commit message to describe the history above.
Sure, will update the commit message as follows.
driver: perf: arm_pmuv3: Read PMMIR_EL1 unconditionally
PMMIR_EL1 needs to be captured in 'armpmu->reg_pmmir', for all appropriate PMU version implementations i.e FEAT_PMUv3p4 onward, where the register is available and reading it is valid. However STALL_SLOT event dependency was included earlier as per previous version ARM ARM wordings [1]. But later i.e ARM ARM version G.a onward, STALL_SLOT event dependency for PMMIR_EL1 register has been dropped. Checking for that dependency is now redundant, and should be dropped.
[1] https://lore.kernel.org/linux-arm-kernel/20200720105410.GD11516@willie-the-truck/
| |