Re: [PATCH v7 1/2] perf: Fujitsu: Add the Uncore MAC PMU driver

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 15 Aug 2025 12:47:28 +0900
Koichi Okuno <fj2767dz@fujitsu.com> wrote:

> This adds a new dynamic PMU to the Perf Events framework to program and
> control the Uncore MAC PMUs in Fujitsu chips.
> 
> This driver was created with reference to drivers/perf/qcom_l3_pmu.c.
> 
> This driver exports formatting and event information to sysfs so it can
> be used by the perf user space tools with the syntaxes:
> 
> perf stat -e mac_iod0_mac0_ch0/ea-mac/ ls
> perf stat -e mac_iod0_mac0_ch0/event=0x80/ ls
> 
> FUJITSU-MONAKA PMU Events Specification v1.1 URL:
> https://github.com/fujitsu/FUJITSU-MONAKA
> 
> Signed-off-by: Koichi Okuno <fj2767dz@fujitsu.com>

Hi, just one slight doubt inline around interrupt affinities.
I can't quite remember what breaks when those don't move with the
perf state and so can't immediately see if relevant here.

Anyhow, if that is resolved (either by you saying it's definitely
not an issue) or adding the irq_set_affinity() calls, then

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Jonathan

> diff --git a/drivers/perf/fujitsu_mac_pmu.c b/drivers/perf/fujitsu_mac_pmu.c
> new file mode 100644
> index 000000000000..1031e0221bb2
> --- /dev/null
> +++ b/drivers/perf/fujitsu_mac_pmu.c
> @@ -0,0 +1,552 @@

> +#define MAC_EVENT_CYCLES                        0x000
Maybe drop the lading 0 on all these values? 0x00
etc as we don't go beyond 0xa0

> +#define MAC_EVENT_READ_COUNT                    0x010
> +#define MAC_EVENT_READ_COUNT_REQUEST            0x011
> +#define MAC_EVENT_READ_COUNT_RETURN             0x012
> +#define MAC_EVENT_READ_COUNT_REQUEST_PFTGT      0x013
> +#define MAC_EVENT_READ_COUNT_REQUEST_NORMAL     0x014
> +#define MAC_EVENT_READ_COUNT_RETURN_PFTGT_HIT   0x015
> +#define MAC_EVENT_READ_COUNT_RETURN_PFTGT_MISS  0x016
> +#define MAC_EVENT_READ_WAIT                     0x017
> +#define MAC_EVENT_WRITE_COUNT                   0x020
> +#define MAC_EVENT_WRITE_COUNT_WRITE             0x021
> +#define MAC_EVENT_WRITE_COUNT_PWRITE            0x022
> +#define MAC_EVENT_MEMORY_READ_COUNT             0x040
> +#define MAC_EVENT_MEMORY_WRITE_COUNT            0x050
> +#define MAC_EVENT_MEMORY_PWRITE_COUNT           0x060
> +#define MAC_EVENT_EA_MAC                        0x080
> +#define MAC_EVENT_EA_MEMORY                     0x090
> +#define MAC_EVENT_EA_MEMORY_MAC_WRITE           0x092
> +#define MAC_EVENT_EA_HA                         0x0a0
> +
> +struct mac_pmu {
> +	struct pmu		pmu;
> +	struct hlist_node	node;
> +	void __iomem		*regs;
> +	struct perf_event	*events[MAC_NUM_COUNTERS];
> +	unsigned long		used_mask[BITS_TO_LONGS(MAC_NUM_COUNTERS)];
> +	cpumask_t		cpumask;
> +};

> +static void fujitsu_mac_counter_update(struct perf_event *event)
> +{
> +	struct mac_pmu *macpmu = to_mac_pmu(event->pmu);
> +	int idx = event->hw.idx;
> +	u64 prev, new;
> +
> +	do {
> +		prev = local64_read(&event->hw.prev_count);
> +		new = readq_relaxed(macpmu->regs + MAC_PM_EVCNTR(idx));
> +	} while (local64_cmpxchg(&event->hw.prev_count, prev, new) != prev);
> +
> +	local64_add(new - prev, &event->count);
> +}



> +static int fujitsu_mac_pmu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct acpi_device *acpi_dev;
> +	struct mac_pmu *macpmu;
> +	struct resource *memrc;
> +	char *name;
> +	int ret;
> +	u64 uid;
> +
> +	acpi_dev = ACPI_COMPANION(dev);
> +	if (!acpi_dev)
> +		return -ENODEV;
> +
> +	ret = acpi_dev_uid_to_integer(acpi_dev, &uid);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "unable to read ACPI uid\n");
> +
> +	macpmu = devm_kzalloc(dev, sizeof(*macpmu), GFP_KERNEL);
> +	if (!macpmu)
> +		return -ENOMEM;
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL, "mac_iod%llu_mac%llu_ch%llu",
> +			      (uid >> 8) & 0xF, (uid >> 4) & 0xF, uid & 0xF);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	macpmu->pmu = (struct pmu) {
> +		.parent		= dev,
> +		.task_ctx_nr	= perf_invalid_context,
> +
> +		.pmu_enable	= fujitsu_mac__pmu_enable,
> +		.pmu_disable	= fujitsu_mac__pmu_disable,
> +		.event_init	= fujitsu_mac__event_init,
> +		.add		= fujitsu_mac__event_add,
> +		.del		= fujitsu_mac__event_del,
> +		.start		= fujitsu_mac__event_start,
> +		.stop		= fujitsu_mac__event_stop,
> +		.read		= fujitsu_mac__event_read,
> +
> +		.attr_groups	= fujitsu_mac_pmu_attr_grps,
> +		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE | PERF_PMU_CAP_NO_INTERRUPT,
> +	};
> +
> +	macpmu->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &memrc);
> +	if (IS_ERR(macpmu->regs))
> +		return PTR_ERR(macpmu->regs);
> +
> +	fujitsu_mac__init(macpmu);
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_request_irq(dev, ret, fujitsu_mac__handle_irq, 0,
> +			       name, macpmu);

It's been a while since I wrote a perf driver, but I'm a bit surprised to not
see irq affinity management in here.  I remember running into issues with
per cpu variables deep in perf when we didn't ensure the perf state and
the irq remain on the same cpu core.  

There might be something different here though.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Request for IRQ failed for slice @%pa\n",
> +				     &memrc->start);
> +
> +	/* Add this instance to the list used by the offline callback */
> +	ret = cpuhp_state_add_instance(mac_pmu_cpuhp_state, &macpmu->node);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Error registering hotplug");
> +
> +	ret = perf_pmu_register(&macpmu->pmu, name, -1);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to register MAC PMU\n");
> +
> +	dev_dbg(dev, "Registered %s, type: %d\n", name, macpmu->pmu.type);
> +
> +	return 0;
> +}