perf: Support Arm Bus Monitor Unit
Subsystems (2)
cover perf: Support Arm Bus Monitor Unit
Hi all,
Here we have the delight of another new PMU driver. While not am Arm product in its own right, the BMU is found in the latest Neoverse Compute Subsystems, including the recently-announced AGI CPU.
For now it is expected that BMU users will be ACPI-based (binding in the latest "ACPI for Arm Components" release[1]), so the token DT binding is more for completeness and maybe very early bringup hacking, but there is a chance it may end up being used by more embedded/client- focused subsystems in future.
And yes, I did initially look at trying to factor out the PMU register accesses that functionally overlap the existing arm_cspmu code, but it very quickly became clear that that would end up far bigger and more complex. This is not a big driver, even with a few dozen lines of nominally-duplicate (but also simplified since the counter size etc. is fixed and known) code, so this way it went.
Thanks,
Robin.
[1] https://developer.arm.com/documentation/den0093/latest/
Robin Murphy (2):
dt-bindings: perf: Add Arm Bus Monitor Unit
perf: Add Arm Bus Monitor Unit driver
.../bindings/perf/arm,bus-monitor-unit.yaml | 33 ++ drivers/perf/arm_cspmu/Kconfig | 6 + drivers/perf/arm_cspmu/Makefile | 2 + drivers/perf/arm_cspmu/arm-bmu.c | 538 ++++++++++++++++++ 4 files changed, 579 insertions(+) create mode 100644 Documentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml create mode 100644 drivers/perf/arm_cspmu/arm-bmu.c
Signature · 1 line
2.54.0.dirty
PATCH 1/2 dt-bindings: perf: Add Arm Bus Monitor Unit
Arm's Bus Monitor Unit is a low-level performance analysis tool for matching and counting transactions at interconnect interfaces. Typical platforms will have multiple BMUs each measuring multiple interfaces in various parts of the system, hence for the sake of usability we make it mandatory for the platform to label each instance such that the end user can correlate with platform documentation to know what's what.
--- .../bindings/perf/arm,bus-monitor-unit.yaml | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 Documentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml
Documentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml new file +33 -0
@@ -0,0 +1,33 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/perf/arm,bus-monitor-unit.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Arm Bus Monitor Unit + +maintainers: + - Robin Murphy <[email protected]> + +properties: + compatible: + const: arm,bus-monitor-unit + + reg: + items: + - description: Full BMU register space + + interrupts: + items: + - description: PMU overflow interrupt + + label: + description: BMU instance identifier (same as ACPI _UID), per the + platform optimisation guide + +required: + - compatible + - reg + - label + +additionalProperties: false
Signature · 1 line
2.54.0.dirty
[email protected]
AI reviewRe: [PATCH 1/2] dt-bindings: perf: Add Arm Bus Monitor Unit
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The YAML device tree binding schema is missing the mandatory `examples` section. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The YAML device tree binding schema is missing the mandatory examples section. --
--- Patch [1]: [PATCH 1/2] dt-bindings: perf: Add Arm Bus Monitor Unit --- commit d43448a74f2d7de7e06899d55b1561b64c21375c
Author: Robin Murphy <[email protected]>
dt-bindings: perf: Add Arm Bus Monitor Unit
This commit adds a new device tree binding schema for the Arm Bus Monitor Unit. It defines the required properties, including a mandatory label to allow end users to correlate instances with platform documentation.
Quoted 5 lines
Documentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml new file +0
-0
[ ... ]
Documentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml line 33
Quoted 6 lines
Documentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml new file +6
-0+required: + - compatible + - reg + - label + +additionalProperties: false
[Severity: Low]
Does this binding schema omit the mandatory examples section? Standard device tree bindings typically require an examples block at the end of the file to provide a reference node implementation and allow full validation by the dt_binding_check tool.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH 1/2] dt-bindings: perf: Add Arm Bus Monitor Unit
In reply to Robin MurphyDocumentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml line 33
Quoted 6 lines · Robin Murphy
Documentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml new file +6
-0+required: + - compatible + - reg + - label + +additionalProperties: false
You need an example DTS, unless this is a subdevice and part of other device as subschema.
Best regards,
Krzysztof
Re: [PATCH 1/2] dt-bindings: perf: Add Arm Bus Monitor Unit
Quoted 10 lines · Krzysztof Kozlowski
Documentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml line 33
Quoted 6 lines · Robin Murphy
Documentation/devicetree/bindings/perf/arm,bus-monitor-unit.yaml new file +6
-0+required: + - compatible + - reg + - label + +additionalProperties: falseYou need an example DTS, unless this is a subdevice and part of other device as subschema.
I still fundamentally disagree with the necessity of "validating" my ability to understand my own trivial 3-property binding that I wrote, but whatever... v2 can have this which I've made up out of thin air since I have no real-world reference to hand:
examples:
- |
pmu@12340000 {
compatible = "arm,bus-monitor-unit";
reg = <0x12340000 0x40000>;
label = "ddr_ch_1_2";
};Thanks,
Robin.
PATCH 2/2 perf: Add Arm Bus Monitor Unit driver
In reply to Robin MurphyArm's Bus Monitor Unit is a low-level performance analysis tool for matching and counting transactions at interconnect interfaces. Each BMU consists of a number of "interface monitoring units", with some global controls to synchronise them and so permit accurate calculation of cross-interface metrics. Each IMU contains, among other things, its own PMU largely based on the CoreSight PMU architecture, but with some functional incompatibilities due to the nature of how much per-event filtering needs to be crammed in to categorise AXI/CHI transactions at the protocol level. As such it gets an honorary home in arm_cspmu/ in order to share the common register definitions, but for simplicity does not try to bend the functional abstraction to fit.
--- drivers/perf/arm_cspmu/Kconfig | 6 + drivers/perf/arm_cspmu/Makefile | 2 + drivers/perf/arm_cspmu/arm-bmu.c | 538 +++++++++++++++++++++++++++++++ 3 files changed, 546 insertions(+) create mode 100644 drivers/perf/arm_cspmu/arm-bmu.c
drivers/perf/arm_cspmu/Kconfig +6 -0
@@ -27,3 +27,9 @@ config AMPERE_CORESIGHT_PMU_ARCH_SYSTEM_PMU In the first phase, the driver enables support on MCU PMU used in AmpereOne SoC family. + +config ARM_BMU + tristate "ARM Bus Monitor Unit" + depends on ARM64 || COMPILE_TEST + help + Support perf event counting on Arm Bus Monitor Unit devices.
drivers/perf/arm_cspmu/Makefile +2 -0
@@ -8,3 +8,5 @@ arm_cspmu_module-y := arm_cspmu.o obj-$(CONFIG_NVIDIA_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += nvidia_cspmu.o obj-$(CONFIG_AMPERE_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += ampere_cspmu.o + +obj-$(CONFIG_ARM_BMU) += arm-bmu.o
drivers/perf/arm_cspmu/arm-bmu.c new file +538 -0
538 lines not on this page · raw message
Signature · 1 line
2.54.0.dirty
[email protected]
AI reviewRe: [PATCH 2/2] perf: Add Arm Bus Monitor Unit driver
Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider: - [High] Uninitialized variable `err` is returned when `devm_request_mem_region` fails. - [High] Event…
Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Uninitialized variable err is returned when devm_request_mem_region fails.
- [High] Event validation erroneously pre-increments the event count for all IMUs in arm_bmu_validate_group(), rejecting valid groups.
- [High] Improper error checking of devm_ioremap return value causes NULL pointer dereference on failure.
- [High] Incorrect CPU hotplug registration order causes NULL pointer dereference or Use-After-Free.
- [High] The driver fails to verify that the hardware-provided PMU offsets and sizes fall within the mapped MMIO resource boundary.
- [Medium] Negative IRQ error code from platform_get_irq() is stored and later passed to irq_set_affinity().
- [Medium] The PMU IRQ handler resets the running hardware counter, permanently losing events.
- [Medium] Spurious WARN_ON triggers if an event is deleted while an overflow is pending.
- [Medium] CPU selection races with CPU hotplug because cpus_read_lock() is not held.
- [Low] Format string type mismatch passing 32-bit resource_size_t to %llx specifier.
- [Low] Using %*pbl format specifier with a pointer to u64 reads incorrectly on 32-bit architectures.
--
--- Patch [2]: [PATCH 2/2] perf: Add Arm Bus Monitor Unit driver --- commit 2a4c03f8d2ffe5343511aa668d1b1b9f0e2a6b53
Author: Robin Murphy <[email protected]>
Subject: perf: Add Arm Bus Monitor Unit driver
This commit introduces the Arm Bus Monitor Unit (BMU) driver to support performance event counting on Arm BMU devices, placing it under arm_cspmu to share common register definitions.
Quoted 5 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +0
-0
[ ... ]
drivers/perf/arm_cspmu/arm-bmu.c line 89
Quoted 10 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +10
-0+static ssize_t arm_bmu_format_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct arm_bmu_format_attr *fmt =container_of (attr,typeof (*fmt), attr); + +if (!fmt->config) +return sysfs_emit (buf,"config:%*pbl\n" ,64 , &fmt->field); + +return sysfs_emit (buf,"config%d:%*pbl\n" , fmt->config,64 , &fmt->field); +}
[Severity: Low]
Does passing a pointer to a u64 variable into the %*pbl format specifier read incorrectly on 32-bit architectures?
The %*pbl specifier expects a pointer to an array of unsigned longs. On 32-bit big-endian systems under COMPILE_TEST, this might result in reversed words.
[ ... ]
Quoted 5 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +5
-0+static int arm_bmu_validate_group (struct perf_event *event) +{ +struct arm_bmu *bmu =to_bmu (event->pmu); +struct perf_event *sibling, *leader = event->group_leader; +int num[MAX_IMUS ] = {0 };
[ ... ]
drivers/perf/arm_cspmu/arm-bmu.c line 174
Quoted 6 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +6
-0+for (int i =0 ; i < bmu->num_imus; i++) { +if (++num[i] > bmu->imus[i].num_counters) +return -EINVAL ; + } +return 0 ; +}
[Severity: High]
Does this loop unconditionally increment the event count requirement for every IMU on the device? It appears ++num[i] will be evaluated for all iterations, meaning valid groups might be rejected if they don't use all available IMUs.
[ ... ]
Quoted 8 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +8
-0+static void arm_bmu_event_stop (struct perf_event *event,int flags) +{ +struct arm_bmu_pmu *pmu =to_bmu_pmu (event); + +writel_relaxed (1ULL << event->hw.idx, pmu->base +PMCNTENCLR ); +if (flags &PERF_EF_UPDATE ) +arm_bmu_event_read (event); +}
[ ... ]
Quoted 7 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +7
-0+static void arm_bmu_event_del (struct perf_event *event,int flags) +{ +struct arm_bmu_pmu *pmu =to_bmu_pmu (event); + +arm_bmu_event_stop (event,PERF_EF_UPDATE ); + pmu->evcnt[event->hw.idx] =NULL ; +}
[ ... ]
drivers/perf/arm_cspmu/arm-bmu.c line 286
Quoted 10 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +10
-0+static void arm_bmu_pmu_irq (struct arm_bmu_pmu *pmu) +{ +u32 reg =readl_relaxed (pmu->base +PMOVSCLR ); +u64 __iomem *pmevcnt = pmu->base +PMEVCNTR_LO ; + +for (int i =0 ; i <PMU_MAX_COUNTERS ; i++) { +if (!(reg & (1U << i))) +continue ; +if (WARN_ON (!pmu->evcnt[i])) +continue ;
[Severity: Medium]
Could an uncleared overflow interrupt trigger this WARN_ON spuriously?
When an event is stopped, the hardware counter is halted, but pending overflow interrupts in PMOVSCLR are not cleared. If an overflow fires just before the counter is stopped and deleted, the IRQ handler might observe a NULL pointer here and unnecessarily spam the logs.
drivers/perf/arm_cspmu/arm-bmu.c line 292
Quoted 6 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +6
-0+arm_bmu_event_read (pmu->evcnt[i]); +local64_set (&pmu->evcnt[i]->hw.prev_count,S64_MIN ); +lo_hi_writeq_relaxed (S64_MIN , pmevcnt + i); + } +writel_relaxed (reg, pmu->base +PMOVSCLR ); +}
[Severity: Medium]
Does writing S64_MIN back to the hardware counter permanently lose events?
The hardware counter is still running while it is being read in the handler. Blindly resetting it without first disabling it via PMCNTENCLR might overwrite and drop any counts that accumulated between the read and this reset.
[ ... ]
Quoted 2 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +2
-0+static int arm_bmu_probe (struct platform_device *pdev) +{
[ ... ]
drivers/perf/arm_cspmu/arm-bmu.c line 326
Quoted 4 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +4
-0+/* PMUs and MPAM MSCs are intermingled so we can't claim the whole resource */ + base =devm_ioremap (dev, res->start,resource_size (res)); +if (IS_ERR (base)) +return PTR_ERR (base);
[Severity: High]
Will IS_ERR(base) ever evaluate to true here?
devm_ioremap() returns NULL on failure, not an ERR_PTR. Checking it with IS_ERR() means allocation failures are silently ignored, leading to a NULL pointer dereference on the subsequent writel_relaxed().
[ ... ]
drivers/perf/arm_cspmu/arm-bmu.c line 343
Quoted 9 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +9
-0+ cfg =lo_hi_readq_relaxed (base +MCU_CONFIG ); + num =1 +FIELD_GET (MCUCFG_NUM_IMU_MONITORS , cfg); +/* We don't expect to have dual-page complications to worry about */ + sz =FIELD_GET (MCUCFG_PMU_ELEMENT_SIZE , cfg); +if (sz !=1 ) +return dev_err_probe (dev, -EINVAL ,"PMU_ELEMENT_SIZE 0x%x not supported\n" , sz); + +/* The PMU pages *are* exclusively ours */ + off =SZ_4K *FIELD_GET (MCUCFG_PMU_ELEMENT_START , cfg);
[Severity: High]
Should the hardware-provided offset be validated against the mapped MMIO resource boundary?
Adding off to base without verifying that the resulting region falls within resource_size(res) could lead to an out-of-bounds memory read if the hardware reports an unexpectedly large value.
drivers/perf/arm_cspmu/arm-bmu.c line 345
Quoted 2 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +2
-0+if (!devm_request_mem_region (dev, res->start + off, num *SZ_4K ,dev_name (dev))) +return dev_err_probe (dev, err,"Unable to request PMU region\n" );
[Severity: High]
Is err uninitialized in this error path?
If devm_request_mem_region() fails, it returns NULL and doesn't modify err. This might cause the probe function to return random stack garbage, or 0 (success), leaving the driver only partially initialized.
[ ... ]
drivers/perf/arm_cspmu/arm-bmu.c line 380
Quoted 1 line
drivers/perf/arm_cspmu/arm-bmu.c new file +1
-0+ bmu->cpu =cpumask_local_spread (atomic_fetch_inc (&n),dev_to_node (dev));
[Severity: Medium]
Could the selected CPU go offline before the PMU's hotplug state is registered?
Since cpus_read_lock() is not held around this selection, a race could leave the driver permanently bound to an offline CPU if a hotplug event occurs right after this assignment.
drivers/perf/arm_cspmu/arm-bmu.c line 392
Quoted 12 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +12
-0+ bmu->irq =platform_get_irq (pdev,0 ); +if (bmu->irq >0 ) { + err =devm_request_irq (dev, bmu->irq, arm_bmu_handle_irq, +IRQF_NOBALANCING |IRQF_NO_THREAD , +dev_name (dev), bmu); +if (err) + bmu->irq =0 ; +else +irq_set_affinity (bmu->irq,cpumask_of (bmu->cpu)); + } +if (!bmu->irq) +dev_info (dev,"Continuing without IRQ\n" );
[Severity: Medium]
If platform_get_irq() fails and returns a negative error code, will it bypass the !bmu->irq check and retain the negative value?
Later during a CPU hotplug migration, arm_bmu_migrate() checks if (bmu->irq) and might pass this negative error code directly into irq_set_affinity().
[ ... ]
drivers/perf/arm_cspmu/arm-bmu.c line 418
Quoted 4 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +4
-0+if (name) + name =devm_kasprintf (dev,GFP_KERNEL ,"arm_bmu_%s" , name); +else + name =devm_kasprintf (dev,GFP_KERNEL ,"arm_bmu_%llx" , res->start >>12 );
[Severity: Low]
Could using %llx here cause format string corruption on 32-bit builds?
Under COMPILE_TEST, res->start is a resource_size_t, which is a 32-bit integer on 32-bit architectures. Passing a 32-bit integer to a 64-bit format specifier will consume extra bits from the variadic arguments.
drivers/perf/arm_cspmu/arm-bmu.c line 431
Quoted 13 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +13
-0+if (!name) +return -ENOMEM ; + + err =cpuhp_state_add_instance_nocalls (arm_bmu_cpuhp_state, &bmu->cpuhp_node); +if (err) +return err; + + err =perf_pmu_register (&bmu->pmu, name,-1 ); +if (err) +cpuhp_state_remove_instance_nocalls (arm_bmu_cpuhp_state, &bmu->cpuhp_node); + +return err; +}
[Severity: High]
Does registering with CPU hotplug before perf_pmu_register() create a race window?
If a hotplug event fires between these two calls, it would trigger perf_pmu_migrate_context() on an uninitialized PMU context, which might result in a NULL pointer dereference.
drivers/perf/arm_cspmu/arm-bmu.c line 442
Quoted 10 lines
drivers/perf/arm_cspmu/arm-bmu.c new file +10
-0+static void arm_bmu_remove (struct platform_device *pdev) +{ +struct arm_bmu *bmu =platform_get_drvdata (pdev); + +for (int i =0 ; i < bmu->num_imus; i++) +writel_relaxed (U32_MAX , bmu->imus[i].base +PMINTENCLR ); + +perf_pmu_unregister (&bmu->pmu); +cpuhp_state_remove_instance_nocalls (arm_bmu_cpuhp_state, &bmu->cpuhp_node); +}
[Severity: High]
Similarly, does unregistering the PMU before removing the hotplug instance create a use-after-free window if a hotplug event occurs right after perf_pmu_unregister() returns?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2