Re: [RESEND PATCH 2/2] drivers/perf: hisi: add driver for HNS3 PMU

[John Garry <john.garry@huawei.com>]

On 17/01/2022 01:52, Guangbin Huang wrote:

+linuxarm

> HNS3 PMU End Point device supports to collect performance statistics
> of bandwidth, latency, packet rate, interrupt rate in HiSilicon SoC
> NIC.
> 
> NIC of each IO DIE has one PMU device for it. Driver registers each
> PMU device to perf, and exports information of supported events,
> filter mode of each event, identifier and so on via sysfs.
> 
> Each PMU device has its own control, counter and interrupt registers,
> and supports up to 8 events by hardware.
> 
> Filter options contains:
> event        - select event
> subevent     - select subevent
> port         - select physical port of nic
> tc           - select tc(must be used with port)
> func         - select PF/VF
> queue        - select queue of PF/VF(must be used with func)
> intr         - select interrupt number(must be used with func)
> global       - select all functions of IO DIE
> 
> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
> ---
>   MAINTAINERS                       |    6 +
>   drivers/perf/hisilicon/Kconfig    |    9 +
>   drivers/perf/hisilicon/Makefile   |    1 +
>   drivers/perf/hisilicon/hns3_pmu.c | 1631 +++++++++++++++++++++++++++++
>   include/linux/cpuhotplug.h        |    1 +
>   5 files changed, 1648 insertions(+)
>   create mode 100644 drivers/perf/hisilicon/hns3_pmu.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90a7d7b21982..e8e258bb8f8d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8614,6 +8614,12 @@ F:	Documentation/admin-guide/perf/hisi-pcie-pmu.rst
>   F:	Documentation/admin-guide/perf/hisi-pmu.rst
>   F:	drivers/perf/hisilicon
>   
> +HISILICON HNS3 PMU DRIVER
> +M:	Guangbin Huang <huangguangbin2@huawei.com>
> +S:	Supported
> +F:	Documentation/admin-guide/perf/hns3-pmu.rst
> +F:	drivers/perf/hisilicon/hns3_pmu.c
> +
>   HISILICON QM AND ZIP Controller DRIVER
>   M:	Zhou Wang <wangzhou1@hisilicon.com>
>   L:	linux-crypto@vger.kernel.org
> diff --git a/drivers/perf/hisilicon/Kconfig b/drivers/perf/hisilicon/Kconfig
> index 5546218b5598..8a9661c97ec8 100644
> --- a/drivers/perf/hisilicon/Kconfig
> +++ b/drivers/perf/hisilicon/Kconfig
> @@ -14,3 +14,12 @@ config HISI_PCIE_PMU
>   	  RCiEP devices.
>   	  Adds the PCIe PMU into perf events system for monitoring latency,
>   	  bandwidth etc.
> +
> +config HNS3_PMU
> +	tristate "HNS3 PERF PMU"
> +	depends on ARM64 && PCI

Any reason not to add COMPILE_TEST, like:
	depends on ARM64 || COMPILE_TEST
	depends on PCI


> +	help
> +	  Provide support for HNS3 performance monitoring unit (PMU) IEP
> +	  devices.
> +	  Adds the HNS3 PMU into perf events system for monitoring latency,
> +	  bandwidth etc.
> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
> index 506ed39e3266..13297ec2798f 100644
> --- a/drivers/perf/hisilicon/Makefile
> +++ b/drivers/perf/hisilicon/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o \
>   			  hisi_uncore_pa_pmu.o
>   
>   obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o
> +obj-$(CONFIG_HNS3_PMU) += hns3_pmu.o
> diff --git a/drivers/perf/hisilicon/hns3_pmu.c b/drivers/perf/hisilicon/hns3_pmu.c
> new file mode 100644
> index 000000000000..075b7eefe8d6
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hns3_pmu.c
> @@ -0,0 +1,1631 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * This driver adds support for HNS3 PMU iEP device.


> Related perf events are
> + * bandwidth, latency, packet rate, interrupt rate etc.

I don't think that this is relevant here

> + *
> + * Copyright (C) 2021 HiSilicon Limited
> + */

2022, I suppose now...

> +};
> +
> +#define HNS3_PMU_SET_HW_FILTER(_hwc, _mode) \
> +	((_hwc)->addr_filters = (void *)&hns3_pmu_hw_filter_modes[(_mode)])
> +
> +static ssize_t identifier_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct hns3_pmu *hns3_pmu;
> +
> +	hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev));

Can the declaration and assignment be on the same line?

> +
> +	return sysfs_emit(buf, "0x%x\n", hns3_pmu->identifier);
> +}
> +static DEVICE_ATTR_RO(identifier);
> +
> +static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev));
> +
> +	return sysfs_emit(buf, "%d\n", hns3_pmu->on_cpu);
> +}
> +static DEVICE_ATTR_RO(cpumask);
> +
> +static ssize_t bdf_min_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev));
> +
> +	return sysfs_emit(buf, "0x%4x\n", hns3_pmu->bdf_min);
> +}
> +static DEVICE_ATTR_RO(bdf_min);
> +
> +static ssize_t bdf_max_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev));
> +
> +	return sysfs_emit(buf, "0x%4x\n", hns3_pmu->bdf_max);

are these same files as in HISI PCI PMU driver?

> +}
> +static DEVICE_ATTR_RO(bdf_max);
> +
> +static struct attribute *hns3_pmu_events_attr[] = {
> +	/* bandwidth events */


> +
> +static struct attribute_group hns3_pmu_events_group = {
> +	.name = "events",
> +	.attrs = hns3_pmu_events_attr,
> +};
> +
> +static struct attribute_group hns3_pmu_filter_mode_group = {
> +	.name = "filtermode",
> +};
> +

> +}
> +
> +static bool hns3_pmu_valid_bdf(struct hns3_pmu *hns3_pmu, u16 bdf)
> +{
> +	struct pci_dev *pdev;
> +
> +	if (bdf < hns3_pmu->bdf_min || bdf > hns3_pmu->bdf_max) {
> +		pci_err(hns3_pmu->pdev, "Invalid EP device: %#x!\n", bdf);
> +		return false;
> +	}
> +
> +	pdev = pci_get_domain_bus_and_slot(pci_domain_nr(hns3_pmu->pdev->bus),
> +					   PCI_BUS_NUM(bdf),
> +					   GET_PCI_DEVFN(bdf));
> +	if (!pdev) {
> +		pci_err(hns3_pmu->pdev, "Nonexistent EP device: %#x!\n", bdf);
> +		return false;
> +	}
> +
> +	pci_dev_put(pdev);
> +	return true;
> +}

if any of this code is same as hisi pci pmu driver then please put in a 
common lib

> +
> +static void hns3_pmu_set_qid_para(struct hns3_pmu *hns3_pmu, u32 idx, u16 bdf,
> +				  u16 queue)
> +{
> +	u32 val;
> +
> +	val = GET_PCI_DEVFN(bdf);
> +	val |= (u32)queue << HNS3_PMU_QID_PARA_QUEUE_S;
> +	hns3_pmu_writel(hns3_pmu, HNS3_PMU_REG_EVENT_QID_PARA, idx, val);
> +}
> +
> +static bool hns3_pmu_qid_req_start(struct hns3_pmu *hns3_pmu, u32 idx)
> +{
> +	bool queue_id_valid = false;
> +	u32 reg_qid_ctrl, val;
> +	int err;
> +
> +	/* enable queue id request */
> +	hns3_pmu_writel(hns3_pmu, HNS3_PMU_REG_EVENT_QID_CTRL, idx,
> +			HNS3_PMU_QID_CTRL_REQ_ENABLE);
> +

...

> +}
> +
> +static bool hns3_pmu_validate_event_group(struct perf_event *event)
> +{
> +	struct perf_event *leader = event->group_leader;
> +	struct perf_event *sibling;
> +	int counters = 1;
> +
> +	if (!is_software_event(leader)) {
> +		/*
> +		 * We must NOT create groups containing mixed PMUs, although
> +		 * software events are acceptable
> +		 */
> +		if (leader->pmu != event->pmu)
> +			return false;
> +
> +		/* Increase counter for the leader */
> +		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;
> +
> +		/* Increase counter for each sibling */
> +		counters++;
> +	}
> +
> +	return counters <= HNS3_PMU_MAX_COUNTERS;
> +}
> +

This is exact same effectively as hisi_validate_event_group(), apart from:

return counters <= HNS3_PMU_MAX_COUNTERS;

vs

return counters <= hisi_pmu->num_counters;


Can we factor all this out? Less code to review makes reviewing easier 
and quicker and less daunting ...

> +}
> +
> +/*
> + * The performance calculation formula of all types of event is
> + *                 performance = data / data_ext.
> + * The arguments data and data_ext have different meanings for different types
> + * of events as follow:
> + * event_type             data                     data_ext
> + * bandwidth              byte number              cycles number
> + * packet rate            packet number            cycles number
> + * interrupt rate         interrupt number         cycles number
> + * latency                cycles number            packet number
> + */
> +static u64 hns3_pmu_get_performance(u8 event_type, u32 hw_clk_freq, u64 data,
> +				    u64 data_ext)
> +{
> +	u64 result;
> +
> +	if (!hw_clk_freq || !data || !data_ext)
> +		return 0;
> +
> +	switch (event_type) {
> +	case HNS3_PMU_EVENT_TYPE_BANDWIDTH:
> +		/* process data to set unit of bandwidth as "KBits/s" */
> +		result = data / hns3_pmu_tick_to_ms(hw_clk_freq, data_ext);
> +		result = BYTES_TO_BITS(result);
> +		break;
> +
> +	case HNS3_PMU_EVENT_TYPE_PACKET_RATE:
> +	case HNS3_PMU_EVENT_TYPE_INTR_RATE:
> +		/* process data to set unit as "pps" */
> +		result = data * 1000 /
> +			 hns3_pmu_tick_to_ms(hw_clk_freq, data_ext);
> +		break;
> +
> +	case HNS3_PMU_EVENT_TYPE_LATENCY:
> +		/* process data to set unit of latency as "ns" */
> +		result = data / data_ext;
> +		result = result * 1000000000 / hw_clk_freq;
> +		break;
> +
> +	default:
> +		result = data / data_ext;
> +		break;
> +	}
> +
> +	return result;
> +}
> +
> +static int hns3_pmu_event_init(struct perf_event *event)
> +{
> +	struct hns3_pmu *hns3_pmu = to_hns3_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int idx;
> +	int ret;
> +
> +	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;
> +
> +	event->cpu = hns3_pmu->on_cpu;
> +
> +	idx = hns3_pmu_get_event_idx(hns3_pmu);
> +	if (idx < 0) {
> +		pci_err(hns3_pmu->pdev, "Up to %u events are supported!\n",
> +			HNS3_PMU_MAX_COUNTERS);
> +		return -EBUSY;
> +	}
> +
> +	hwc->idx = idx;
> +
> +	ret = hns3_pmu_select_filter_mode(event, hns3_pmu);
> +	if (ret) {
> +		pci_err(hns3_pmu->pdev, "Invalid filter, ret = %d.\n", ret);
> +		return ret;
> +	}
> +
> +	if (!hns3_pmu_validate_event_group(event)) {
> +		pci_err(hns3_pmu->pdev, "Invalid event group.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void hns3_pmu_read(struct perf_event *event)
> +{
> +	struct hns3_pmu *hns3_pmu = to_hns3_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hw_perf_event_extra *hwc_ext;
> +	u64 new_cnt_ext, prev_cnt_ext;
> +	u64 new_cnt, prev_cnt, delta;
> +
> +	hwc_ext = &hwc->extra_reg;
> +	do {
> +		prev_cnt = local64_read(&hwc->prev_count);
> +		prev_cnt_ext = hwc_ext->config;
> +		hns3_pmu_read_counter(event, &new_cnt, &new_cnt_ext);
> +	} while (local64_cmpxchg(&hwc->prev_count, prev_cnt, new_cnt) !=
> +		 prev_cnt);
> +
> +	hwc_ext->config = new_cnt_ext;
> +
> +	delta = hns3_pmu_get_performance(hns3_get_event(event),
> +					 hns3_pmu->hw_clk_freq,
> +					 new_cnt - prev_cnt,
> +					 new_cnt_ext - prev_cnt_ext);

This looks very much like what was originally had in the hisi PCI PMU 
driver:

https://lore.kernel.org/linux-arm-kernel/20210616134257.GA22905@willie-the-truck/

https://lore.kernel.org/linux-arm-kernel/20210617175704.GF24813@willie-the-truck/

Please consult with liuqi

If it is, I figure that we're going to need to expose the 2x values and 
do the calculation in userspace.

> +
> +	local64_add(delta, &event->count);
> +}
> +
> +static void hns3_pmu_start(struct perf_event *event, int flags)
> +{
> +	struct hns3_pmu *hns3_pmu = to_hns3_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> +		return;
> +
> +	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));

...

> +
> +	val = readl(hns3_pmu->base + HNS3_PMU_REG_DEVICE_ID);
> +	device_id = val & 0xffff;
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hns3_pmu_%u", device_id);

would adding sccl to the name help it make it more clear that this is 
per sccl, like hns3_pmu_sccl%u?

> +	if (!name) {
> +		pci_err(pdev, "failed to allocate name for HNS3 PMU.\n");

I don't think that you need this -ENOMEM messages

> +		ret = -ENOMEM;
> +		goto err_alloc_dev_name;
> +	}
> +
> +	hns3_pmu->pdev = pdev;
> +	hns3_pmu->on_cpu = HNS3_PMU_INVALID_CPU_ID;

can you possibly just initially target the probe CPU?

> +	hns3_pmu->identifier = readl(hns3_pmu->base + HNS3_PMU_REG_VERSION);
> +	hns3_pmu->pmu = (struct pmu) {
> +		.name		= name,
> +		.module		= THIS_MODULE,
> +		.event_init	= hns3_pmu_event_init,
> +		.pmu_enable	= hns3_pmu_enable,
> +		.pmu_disable	= hns3_pmu_disable,
> +		.add		= hns3_pmu_add,
> +		.del		= hns3_pmu_del,
> +		.start		= hns3_pmu_start,
> +		.stop		= hns3_pmu_stop,
> +		.read		= hns3_pmu_read,
> +		.task_ctx_nr	= perf_invalid_context,
> +		.attr_groups	= hns3_pmu_attr_groups,
> +		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
> +	};
> +
> +	return 0;
> +
> +err_alloc_dev_name:
> +	iounmap(hns3_pmu->base);
> +err_ioremap_bar:
> +	pci_release_regions(pdev);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t hns3_pmu_irq(int irq, void *data)
> +{
> +	struct hns3_pmu *hns3_pmu = data;
> +	u32 intr_status, idx;
> +
> +	for (idx = 0; idx < HNS3_PMU_MAX_COUNTERS; idx++) {
> +		intr_status = hns3_pmu_readl(hns3_pmu,
> +					     HNS3_PMU_REG_EVENT_INTR_STATUS,
> +					     idx);
> +
> +		/*
> +		 * As each counter will restart from 0 when it is overflowed,
> +		 * extra processing is no need, just clear interrupt status.
> +		 */
> +		if (intr_status)
> +			hns3_pmu_clear_intr_status(hns3_pmu, idx);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int hns3_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct hns3_pmu *hns3_pmu;
> +
> +	hns3_pmu = hlist_entry_safe(node, struct hns3_pmu, node);
> +	if (!hns3_pmu)
> +		return -ENODEV;
> +
> +	if (hns3_pmu->on_cpu == HNS3_PMU_INVALID_CPU_ID) {
> +		hns3_pmu->on_cpu = cpu;
> +		irq_set_affinity_hint(hns3_pmu->irq, cpumask_of(cpu));
> +	}
> +
> +	return 0;
> +}
> +
> +static int hns3_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct hns3_pmu *hns3_pmu;
> +	unsigned int target;
> +
> +	hns3_pmu = hlist_entry_safe(node, struct hns3_pmu, node);
> +	if (!hns3_pmu)
> +		return -ENODEV;
> +
> +	/* Nothing to do if this CPU doesn't own the PMU */
> +	if (hns3_pmu->on_cpu != cpu)
> +		return 0;
> +
> +	/* Choose a new CPU from all online cpus */
> +	target = cpumask_any_but(cpu_online_mask, cpu);
> +	if (target >= nr_cpu_ids)
> +		return 0;
> +
> +	perf_pmu_migrate_context(&hns3_pmu->pmu, cpu, target);
> +	hns3_pmu->on_cpu = target;
> +	irq_set_affinity_hint(hns3_pmu->irq, cpumask_of(target));
> +
> +	return 0;
> +}
> +
> +static int hns3_pmu_irq_register(struct pci_dev *pdev,
> +				 struct hns3_pmu *hns3_pmu)
> +{
> +	int irq, ret;
> +
> +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);

I don't think that a pcim exists here, so can use devm_add_action() to 
free them [maybe the name is wrong I mentioned]

> +	if (ret < 0) {
> +		pci_err(pdev, "failed to enable MSI vectors, ret = %d.\n", ret);
> +		return ret;
> +	}
> +
> +	irq = pci_irq_vector(pdev, 0);
> +	ret = request_irq(irq, hns3_pmu_irq, 0, hns3_pmu->pmu.name, hns3_pmu);

devm_request_irq()

> +	if (ret) {
> +		pci_err(pdev, "failed to register irq, ret = %d.\n", ret);
> +		pci_free_irq_vectors(pdev);
> +		return ret;
> +	}
> +
> +	hns3_pmu->irq = irq;
> +
> +	return 0;
> +}
> +
> +static void hns3_pmu_irq_unregister(struct pci_dev *pdev,
> +				    struct hns3_pmu *hns3_pmu)
> +{
> +	free_irq(hns3_pmu->irq, hns3_pmu);
> +	pci_free_irq_vectors(pdev);
> +}
> +
> +static int hns3_pmu_init(struct pci_dev *pdev, struct hns3_pmu *hns3_pmu)
> +{
> +	int ret;
> +
> +	ret = hns3_pmu_alloc_pmu(pdev, hns3_pmu);
> +	if (ret)
> +		return ret;
> +
> +	ret = hns3_pmu_irq_register(pdev, hns3_pmu);
> +	if (ret)
> +		goto err_irq_register;
> +
> +	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HNS3_PMU_ONLINE,
> +				       &hns3_pmu->node);
> +	if (ret) {
> +		pci_err(pdev, "failed to register hotplug, ret = %d.\n", ret);
> +		goto err_register_hotplug;
> +	}
> +
> +	ret = perf_pmu_register(&hns3_pmu->pmu, hns3_pmu->pmu.name, -1);
> +	if (ret) {
> +		pci_err(pdev, "failed to register HNS3 PMU, ret = %d.\n", ret);

do you need to mention "HNS3 PMU", since pdev is passed

> +		goto err_register_pmu;
> +	}
> +
> +	return ret;
> +
> +err_register_pmu:
> +	cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HNS3_PMU_ONLINE,
> +				    &hns3_pmu->node);
> +	irq_set_affinity_hint(hns3_pmu->irq, NULL);
> +
> +err_register_hotplug:
> +	hns3_pmu_irq_unregister(pdev, hns3_pmu);
> +
> +err_irq_register:
> +	iounmap(hns3_pmu->base);
> +
> +	return ret;
> +}
> +
> +static void hns3_pmu_uninit(struct pci_dev *pdev)
> +{
> +	struct hns3_pmu *hns3_pmu = (struct hns3_pmu *)pci_get_drvdata(pdev);

no need to cast a void ptr

> +
> +	perf_pmu_unregister(&hns3_pmu->pmu);
> +	cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HNS3_PMU_ONLINE,
> +				    &hns3_pmu->node);
> +	irq_set_affinity_hint(hns3_pmu->irq, NULL);
> +	hns3_pmu_irq_unregister(pdev, hns3_pmu);
> +	iounmap(hns3_pmu->base);
> +}
> +
> +static int hns3_pmu_init_dev(struct pci_dev *pdev)
> +{
> +	int ret;
> +
> +	ret = pci_enable_device(pdev);

isn't there pci-managed versions of this so that you don't need worry 
about undo'ing this manually?

> +	if (ret) {
> +		pci_err(pdev, "failed to enable pci device, ret = %d.\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pci_request_mem_regions(pdev, "hns3_pmu");
> +	if (ret < 0) {
> +		pci_err(pdev, "failed to request pci mem regions, ret = %d.\n",
> +			ret);
> +		pci_disable_device(pdev);
> +		return ret;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	return 0;
> +}
> +
> +static void hns3_pmu_uninit_dev(struct pci_dev *pdev)
> +{
> +	pci_clear_master(pdev);
> +	pci_release_mem_regions(pdev);
> +	pci_disable_device(pdev);
> +}
> +

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