Hi, Will
On 2021/8/24 22:31, Will Deacon wrote:

Hi,

On Wed, Aug 18, 2021 at 01:12:46PM +0800, Qi Liu wrote:

quoted

PCIe PMU Root Complex Integrated End Point(RCiEP) device is supported
to sample bandwidth, latency, buffer occupation etc.

Each PMU RCiEP device monitors multiple Root Ports, and each RCiEP is
registered as a PMU in /sys/bus/event_source/devices, so users can
select target PMU, and use filter to do further sets.

Filtering options contains:
event     - select the event.
port      - select target Root Ports. Information of Root Ports are
             shown under sysfs.
bdf       - select requester_id of target EP device.
trig_len  - set trigger condition for starting event statistics.
trig_mode - set trigger mode. 0 means starting to statistic when bigger
             than trigger condition, and 1 means smaller.
thr_len   - set threshold for statistics.
thr_mode  - set threshold mode. 0 means count when bigger than threshold,
             and 1 means smaller.

I think this is getting there now, thanks for sticking with it. Just a
couple of comments below..

quoted

+static bool hisi_pcie_pmu_validate_event_group(struct perf_event *event)
+{
+	struct perf_event *sibling, *leader = event->group_leader;
+	int counters = 1;
+
+	if (!is_software_event(leader)) {
+		if (leader->pmu != event->pmu)
+			return false;
+
+		if (leader != event)
+			counters++;
+	}
+
+	for_each_sibling_event(sibling, event->group_leader) {
+		if (is_software_event(sibling))
+			continue;
+
+		if (sibling->pmu != event->pmu)
+			return false;
+
+		counters++;
+	}
+
+	return counters <= HISI_PCIE_MAX_COUNTERS;
+}

Given that this function doesn't look at the event numbers, doesn't this
over-provision the counter registers? For example, if I create a group
containing 4 of the same event, then we'll allocate four counters but only
use one. Similarly, if I create a group containing two events, one for the
normal counter and one for the extended counter, then we'll again allocate
two counters instead of one.

Yes, we should add some check in hisi_pcie_pmu_validate_event_group() 
function to avoid over-provision. I'll use a array to record events and 
do this check.

Thanks for your review, I'll fix this.

Have I misunderstood?

quoted

+static int hisi_pcie_pmu_event_init(struct perf_event *event)
+{
+	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+
+	event->cpu = pcie_pmu->on_cpu;
+
+	if (EXT_COUNTER_IS_USED(hisi_pcie_get_event(event)))
+		hwc->event_base = HISI_PCIE_EXT_CNT;
+	else
+		hwc->event_base = HISI_PCIE_CNT;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	/* Sampling is not supported. */
+	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
+		return -EOPNOTSUPP;
+
+	if (!hisi_pcie_pmu_valid_filter(event, pcie_pmu)) {
+		pci_err(pcie_pmu->pdev, "Invalid filter!\n");

Please remove this message, as it's triggerable from userspace.

got it, will remove this.

Thanks,
Qi

Will
.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel