lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <TYCPR01MB671711BEBA248EDD29542476C150A@TYCPR01MB6717.jpnprd01.prod.outlook.com>
Date: Fri, 18 Jul 2025 06:00:48 +0000
From: "Koichi Okuno (Fujitsu)" <fj2767dz@...itsu.com>
To: 'Jonathan Cameron' <Jonathan.Cameron@...wei.com>
CC: Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
	Jonathan Corbet <corbet@....net>, Catalin Marinas <catalin.marinas@....com>,
	Gowthami Thiagarajan <gthiagarajan@...vell.com>, Linu Cherian
	<lcherian@...vell.com>, Robin Murphy <robin.murphy@....com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, Bjorn Andersson
	<quic_bjorande@...cinc.com>, Geert Uytterhoeven <geert+renesas@...der.be>,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Konrad Dybcio
	<konradybcio@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>, Arnd
 Bergmann <arnd@...db.de>, NĂ­colas "F. R. A. Prado"
	<nfraprado@...labora.com>, Thomas Gleixner <tglx@...utronix.de>, Peter
 Zijlstra <peterz@...radead.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Koichi Okuno (Fujitsu)"
	<fj2767dz@...itsu.com>
Subject: RE: [PATCH v6 1/2] perf: Fujitsu: Add the Uncore MAC PMU driver

Hi, Jonathan

> > This adds two new dynamic PMUs to the Perf Events framework to program
> > and control the Uncore MAC PMUs in Fujitsu chips.
> > 
> > These drivers were created with reference to drivers/perf/qcom_l3_pmu.c.
> > 
> > These drivers export formatting and event information to sysfs so they 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@...itsu.com>
> Hi
> 
> We may have discussed on an earlier version and I've forgotten about it
> (in which case sorry!) but why can't this be modular?
> 
> Given the chances are we will want it in standard distributions, a
> bool kconfig seems to ensure bloat that people are not going to want to
> see.

It follows qcom_l3_pmu.c which is the base of this driver, so I would like
to keep it that way.

> A few other minor comments inline.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../admin-guide/perf/fujitsu_mac_pmu.rst      |  73 +++
> >  Documentation/admin-guide/perf/index.rst      |   1 +
> >  drivers/perf/Kconfig                          |   9 +
> >  drivers/perf/Makefile                         |   1 +
> >  drivers/perf/fujitsu_mac_pmu.c                | 569 ++++++++++++++++++
> >  5 files changed, 653 insertions(+)
> >  create mode 100644 Documentation/admin-guide/perf/fujitsu_mac_pmu.rst
> >  create mode 100644 drivers/perf/fujitsu_mac_pmu.c
> > 
> > diff --git a/Documentation/admin-guide/perf/fujitsu_mac_pmu.rst b/Documentation/admin-guide/perf/fujitsu_mac_pmu.rst
> > new file mode 100644
> > index 000000000000..383f3c1bbde7
> > --- /dev/null
> > +++ b/Documentation/admin-guide/perf/fujitsu_mac_pmu.rst
> > @@ -0,0 +1,73 @@
> > +====================================================
> > +Fujitsu Uncore MAC Performance Monitoring Unit (PMU)
> > +====================================================
> > +
> > +This driver supports the Uncore MAC PMUs found in Fujitsu chips.
> > +Each MAC PMU on these chips is exposed as a uncore perf PMU with device name
> > +mac_iod<iod>_mac<mac>_ch<ch>.
> > +
> > +The driver provides a description of its available events and configuration
> > +options in sysfs, see /sys/bus/event_sources/devices/mac_iod<iod>_mac<mac>_ch<ch>/.
> > +This driver exports:
> > +- formats, used by perf user space and other tools to configure events
> > +- events, used by perf user space and other tools to create events
> > +  symbolically, e.g.:
> > +    perf stat -a -e mac_iod0_mac0_ch0/event=0x21/ ls
> > +- cpumask, used by perf user space and other tools to know on which CPUs
> > +  to open the events
> > +
> > +This driver supports the following events:
> > +- cycles
> > +  This event counts MAC cycles at MAC frequency.
> 
> Trivial but you could shorten all these by rewording to drop the This Event and counts
> as both obvious from context.
> 
>   - cycles
>     MAC Cycles at MAC frequency.
>   - read-count
>     Number of read requests to MAC.
> etc

These descriptions follow the FUJITSU-MONAKA PMU Events Specification(v1.1).
https://github.com/fujitsu/FUJITSU-MONAKA
I'd like to keep the current format.

> > +- read-count
> > +  This event counts the number of read requests to MAC.
> > +- read-count-request
> > +  This event counts the number of read requests including retry to MAC.
> > +- read-count-return
> > +  This event counts the number of responses to read requests to MAC.
> > +- read-count-request-pftgt
> > +  This event counts the number of read requests including retry with PFTGT
> > +  flag.
> > +- read-count-request-normal
> > +  This event counts the number of read requests including retry without PFTGT
> > +  flag.
> > +- read-count-return-pftgt-hit
> > +  This event counts the number of responses to read requests which hit the
> > +  PFTGT buffer.
> > +- read-count-return-pftgt-miss
> > +  This event counts the number of responses to read requests which miss the
> > +  PFTGT buffer.
> > +- read-wait
> > +  This event counts outstanding read requests issued by DDR memory controller
> > +  per cycle.
> > +- write-count
> > +  This event counts the number of write requests to MAC (including zero write,
> > +  full write, partial write, write cancel).
> > +- write-count-write
> > +  This event counts the number of full write requests to MAC (not including
> > +  zero write).
> > +- write-count-pwrite
> > +  This event counts the number of partial write requests to MAC.
> > +- memory-read-count
> > +  This event counts the number of read requests from MAC to memory.
> > +- memory-write-count
> > +  This event counts the number of full write requests from MAC to memory.
> > +- memory-pwrite-count
> > +  This event counts the number of partial write requests from MAC to memory.
> > +- ea-mac
> > +  This event counts energy consumption of MAC.
> > +- ea-memory
> > +  This event counts energy consumption of memory.
> > +- ea-memory-mac-write
> > +  This event counts the number of write requests from MAC to memory.
> > +- ea-ha
> > +  This event counts energy consumption of HA.
> > +
> > +  'ea' is the abbreviation for 'Energy Analyzer'.
> > +
> > +Examples for use with perf::
> > +
> > +  perf stat -e mac_iod0_mac0_ch0/ea-mac/ ls
> > +
> > +Given that these are uncore PMUs the driver does not support sampling, therefore
> > +"perf record" will not work. Per-task perf sessions are not supported.
> 
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> > index 278c929dc87a..269bf72a512a 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -178,6 +178,15 @@ config FSL_IMX9_DDR_PMU
> >  	 can give information about memory throughput and other related
> >  	 events.
> >  
> > +config FUJITSU_MAC_PMU
> > +	bool "Fujitsu Uncore MAC PMU"
> 
> We may have discussed this in an earlier version but why not
> tristate? + module_init() 

Same as first answer, but follows qcom_l3_pmu.c, which is the base of this driver,
so I would like to keep it that way.

> > +	depends on (ARM64 && (ACPI || COMPILE_TEST))
> > +	help
> > +	 Provides support for the Uncore MAC performance monitor unit (PMU)
> > +	 in Fujitsu processors.
> > +	 Adds the Uncore MAC PMU into the perf events subsystem for
> > +	 monitoring Uncore MAC events.
> > +
> >  config QCOM_L2_PMU
> >  	bool "Qualcomm Technologies L2-cache PMU"
> >  	depends on ARCH_QCOM && ARM64 && ACPI
> 
> > diff --git a/drivers/perf/fujitsu_mac_pmu.c b/drivers/perf/fujitsu_mac_pmu.c
> > new file mode 100644
> > index 000000000000..3c5e838b9894
> > --- /dev/null
> > +++ b/drivers/perf/fujitsu_mac_pmu.c
> > @@ -0,0 +1,569 @@
> 
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/bitops.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/list.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/perf_event.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* Number of counters on each PMU */
> > +#define MAC_NUM_COUNTERS  8
> > +/* Mask for the event type field within perf_event_attr.config and EVTYPE reg */
> > +#define MAC_EVTYPE_MASK   0xFF
> > +
> > +/* Perfmon registers */
> > +#define MAC_PM_EVCNTR(__cntr) (0x000 + (__cntr) * 8)
> > +#define MAC_PM_CNTCTL(__cntr) (0x100 + (__cntr) * 8)
> > +#define MAC_PM_EVTYPE(__cntr) (0x200 + (__cntr) * 8)
> > +#define MAC_PM_CR         0x400
> > +#define MAC_PM_CNTENSET   0x410
> > +#define MAC_PM_CNTENCLR   0x418
> > +#define MAC_PM_INTENSET   0x420
> > +#define MAC_PM_INTENCLR   0x428
> > +#define MAC_PM_OVSR       0x440
> 
> > +
> > +/* MAC_PM_INTENCLR */
> > +#define PMINTENCLR(__cntr)    BIT(__cntr)
> > +#define PM_INTENCLR_RESET     0xFF
> > +
> > +/* MAC_PM_OVSR */
> 
> I'd rather see the macros named to make it clear which register
> they apply to sufficiently that the comments aren't needed.
> Also in my view good to put them directly under the register definine.

I agree.
I rename the macros name to make it clear which register it applies to.
Place it directly below the register definition and remove the comment.

> > +#define PMOVSRCLR(__cntr)     BIT(__cntr)
> Not used.

Understood.
Delete not used macro.

> > +#define PMOVSRCLR_RESET       0xFF
> 
> 
> > +/*
> > + * We must NOT create groups containing events from multiple hardware PMUs,
> > + * although mixing different software and hardware PMUs is allowed.
> > + */
> > +static bool fujitsu_mac__validate_event_group(struct perf_event *event)
> > +{
> > +	struct perf_event *leader = event->group_leader;
> > +	struct perf_event *sibling;
> > +	int counters = 0;
> 
> Value never used.

Understood.
I'll change the counter to not set to 0.

> > +
> > +	if (leader->pmu != event->pmu && !is_software_event(leader))
> > +		return false;
> > +
> > +	/* The sum of the counters used by the event and its leader event */
> > +	counters = 2;
> > +
> > +	for_each_sibling_event(sibling, leader) {
> > +		if (is_software_event(sibling))
> > +			continue;
> > +		if (sibling->pmu != event->pmu)
> > +			return false;
> > +		counters++;
> > +	}
> > +
> > +	/*
> > +	 * If the group requires more counters than the HW has, it
> > +	 * cannot ever be scheduled.
> > +	 */
> > +	return counters <= MAC_NUM_COUNTERS;
> > +}
> 
> > +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);
> 
> Unusual indentation.  Normally second line of parametes should just be
> aligned under first set of parameters.  E.g.
> 
> 	ret = devm_request_irq(dev, ret, fujitsu_mac__handle_irq, 0,
> 			       name, macpmu);
> 

This line was adjusted in Tab4, so I will change it to be adjusted in Tab8.

> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Request for IRQ failed for slice @%pa\n",
> > +							 &memrc->start);
> 
> Similar for this second line set of parameters.

Understood.

> > +
> > +	/* 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;
> > +}
> 
> 
> > +static int __init register_fujitsu_mac_pmu_driver(void)
> > +{
> > +	int ret;
> > +
> > +	/* Install a hook to update the reader CPU in case it goes offline */
> > +	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
> > +				      "perf/fujitsu/mac:online",
> > +				      fujitsu_mac_pmu_online_cpu,
> > +				      fujitsu_mac_pmu_offline_cpu);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mac_pmu_cpuhp_state = ret;
> > +	return platform_driver_register(&fujitsu_mac_pmu_driver);
> > +}
> > +device_initcall(register_fujitsu_mac_pmu_driver);
> 
> Perhaps add a comment on why this can be a module? Would need a trivial
> exit plus to use module_init() / module_exit()


Best Regards,
Koichi Okuno

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ