Re: [PATCH v7 7/7] perf/amd/iommu: Enable support for multiple IOMMUs

[Borislav Petkov <bp@alien8.de>]

On Mon, Jan 09, 2017 at 09:33:47PM -0600, Suravee Suthikulpanit wrote:
> This patch adds multi-IOMMU support for perf by exposing
> an AMD IOMMU PMU for each IOMMU found in the system via:
> 
>   /sys/device/amd_iommu_x     /* where x is the IOMMU index. */

Straight into the top-level devices hierarchy?

Why not add a proper hierarchy to

/sys/devices/system/iommu/ ...

?

Jörg, don't you have a sane iommu hierarchy already in sysfs?

> This allows users to specify different events to be programed
> onto performance counters of each IOMMU.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c | 119 ++++++++++++++++++++++++--------------------
>  1 file changed, 64 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 2403c78..5fd97b5 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -35,10 +35,13 @@
>  #define _GET_PASID_MASK(ev) ((ev->hw.extra_reg.config >> 16) & 0xFFFFULL)
>  #define _GET_DOMID_MASK(ev) ((ev->hw.extra_reg.config >> 32) & 0xFFFFULL)
>  
> -static struct perf_amd_iommu __perf_iommu;
> +#define PERF_AMD_IOMMU_NAME_SZ 16
>  
>  struct perf_amd_iommu {
> +	struct list_head list;
>  	struct pmu pmu;
> +	uint idx;

This uint thing has crept in:

$ git grep "uint " arch/x86/
arch/x86/crypto/crc32-pclmul_asm.S:101: *      uint crc32_pclmul_le_16(unsigned char const *buffer,
arch/x86/crypto/crc32-pclmul_asm.S:102: *                            size_t len, uint crc32)
arch/x86/events/amd/iommu.h:32:extern u8 amd_iommu_pc_get_max_banks(uint idx);
arch/x86/events/amd/iommu.h:34:extern u8 amd_iommu_pc_get_max_counters(uint idx);
arch/x86/events/amd/iommu.h:36:extern int amd_iommu_pc_set_reg(uint idx, u16 devid, u8 bank, u8 cntr,
arch/x86/events/amd/iommu.h:39:extern int amd_iommu_pc_set_counter(uint idx, u8 bank, u8 cntr, u64 *value);
arch/x86/events/amd/iommu.h:41:extern int amd_iommu_pc_get_counter(uint idx, u8 bank, u8 cntr, u64 *value);

"unsigned int" please.

> +	char name[PERF_AMD_IOMMU_NAME_SZ];
>  	u8 max_banks;
>  	u8 max_counters;
>  	u64 cntr_assign_mask;
> @@ -46,6 +49,8 @@ struct perf_amd_iommu {
>  	const struct attribute_group *attr_groups[4];
>  };
>  
> +LIST_HEAD(perf_amd_iommu_list);

static

> +
>  #define format_group	attr_groups[0]
>  #define cpumask_group	attr_groups[1]
>  #define events_group	attr_groups[2]
> @@ -204,8 +209,7 @@ static int clear_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu,
>  static int perf_iommu_event_init(struct perf_event *event)
>  {
>  	struct hw_perf_event *hwc = &event->hw;
> -	struct perf_amd_iommu *perf_iommu;
> -	u64 config, config1;
> +	struct perf_amd_iommu *pi = container_of(event->pmu, struct perf_amd_iommu, pmu);

Do this deref when you actually need it:

	struct perf_amd_iommu *pi;

because you might exit earlier and end up doing it for nothing.

>  
>  	/* test the event attr type check for PMU enumeration */
>  	if (event->attr.type != event->pmu->type)
> @@ -227,27 +231,17 @@ static int perf_iommu_event_init(struct perf_event *event)
>  	if (event->cpu < 0)
>  		return -EINVAL;
>  
> -	perf_iommu = &__perf_iommu;
> -
> -	if (event->pmu != &perf_iommu->pmu)
> -		return -ENOENT;
> -
> -	if (perf_iommu) {
> -		config = event->attr.config;
> -		config1 = event->attr.config1;
> -	} else {
> -		return -EINVAL;
> -	}
> -
>  	/* update the hw_perf_event struct with the iommu config data */
> -	hwc->config = config;
> -	hwc->extra_reg.config = config1;

---> here:

	pi = container_of(event->pmu, struct perf_amd_iommu, pmu);

> +	hwc->idx = pi->idx;
> +	hwc->config = event->attr.config;
> +	hwc->extra_reg.config = event->attr.config1;

Align vertically on the = sign.

>  
>  	return 0;
>  }
>  
>  static void perf_iommu_enable_event(struct perf_event *ev)
>  {
> +	struct hw_perf_event *hwc = &ev->hw;
>  	u8 csource = _GET_CSOURCE(ev);
>  	u16 devid = _GET_DEVID(ev);
>  	u8 bank = _GET_BANK(ev);

...

>  static void perf_iommu_disable_event(struct perf_event *event)
>  {
> +	struct hw_perf_event *hwc = &event->hw;
>  	u64 reg = 0ULL;
>  
> -	amd_iommu_pc_set_reg(0, _GET_DEVID(event), _GET_BANK(event),
> +	amd_iommu_pc_set_reg(hwc->idx, _GET_DEVID(event), _GET_BANK(event),
>  			     _GET_CNTR(event), IOMMU_PC_COUNTER_SRC_REG, &reg);
>  }
>  
> @@ -301,7 +296,7 @@ static void perf_iommu_start(struct perf_event *event, int flags)
>  
>  	val = local64_read(&hwc->prev_count);
>  
> -	amd_iommu_pc_set_counter(0, _GET_BANK(event), _GET_CNTR(event), &val);
> +	amd_iommu_pc_set_counter(hwc->idx, _GET_BANK(event), _GET_CNTR(event), &val);
>  enable:
>  	perf_iommu_enable_event(event);
>  	perf_event_update_userpage(event);
> @@ -314,7 +309,7 @@ static void perf_iommu_read(struct perf_event *event)
>  	s64 delta;
>  	struct hw_perf_event *hwc = &event->hw;
>  
> -	if (amd_iommu_pc_get_counter(0, _GET_BANK(event), _GET_CNTR(event), &cnt))
> +	if (amd_iommu_pc_get_counter(hwc->idx, _GET_BANK(event), _GET_CNTR(event), &cnt))
>  		return;
>  
>  	/* IOMMU pc counter register is only 48 bits */
> @@ -417,14 +412,20 @@ static __init int _init_events_attrs(struct perf_amd_iommu *perf_iommu)
>  
>  static __init void amd_iommu_pc_exit(void)
>  {
> -	if (__perf_iommu.events_group != NULL) {
> -		kfree(__perf_iommu.events_group);
> -		__perf_iommu.events_group = NULL;
> +	struct perf_amd_iommu *pi, *next;
> +
> +	list_for_each_entry_safe(pi, next, &perf_amd_iommu_list, list) {
> +		list_del(&pi->list);
> +
> +		kfree(pi->events_group);
> +		pi->events_group = NULL;

Why that assignment if you're going to free it in the next line?

> +
> +		kfree(pi);
>  	}
>  }
>  
> -static __init int _init_perf_amd_iommu(
> -	struct perf_amd_iommu *perf_iommu, char *name)
> +static __init int
> +init_one_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, uint idx)
>  {
>  	int ret;
>  
> @@ -441,54 +442,62 @@ static __init int _init_perf_amd_iommu(
>  	if (_init_events_attrs(perf_iommu) != 0)
>  		pr_err("Only support raw events.\n");


Here's also

        /* Init cpumask attributes to only core 0 */
        cpumask_set_cpu(0, &iommu_cpumask);

is that still correct for all IOMMUs?

>  
> -	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
> -	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
> +	snprintf(perf_iommu->name, PERF_AMD_IOMMU_NAME_SZ, "amd_iommu_%u", idx);

Do this ^^

> +	perf_iommu->idx = idx;
> +	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(idx);
> +	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(idx);
>  	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
>  		return -EINVAL;


... here, after the test has passed.

>  	/* Init null attributes */
>  	perf_iommu->null_group = NULL;

No need for this - you're getting zeroed memory from kzalloc().

> +	/* Setting up PMU */
> +	perf_iommu->pmu.event_init = perf_iommu_event_init,
> +	perf_iommu->pmu.add = perf_iommu_add,
> +	perf_iommu->pmu.del = perf_iommu_del,
> +	perf_iommu->pmu.start = perf_iommu_start,
> +	perf_iommu->pmu.stop = perf_iommu_stop,
> +	perf_iommu->pmu.read = perf_iommu_read,
> +	perf_iommu->pmu.task_ctx_nr = perf_invalid_context;
>  	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;

Align vertically on the =

> -	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
> +	ret = perf_pmu_register(&perf_iommu->pmu, perf_iommu->name, -1);
>  	if (ret) {
>  		pr_err("Error initializing AMD IOMMU perf counters.\n");
>  		amd_iommu_pc_exit();
>  	} else {
> -		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
> -			amd_iommu_pc_get_max_banks(0),
> -			amd_iommu_pc_get_max_counters(0));
> +		pr_info("Detected %s, w/ %d banks, %d counters/bank\n",

I'd prefer:

		"Detected AMD IOMMU #%d (%d banks, %d counters/bank)"

where the "#%d" is idx.

>  static __init int amd_iommu_pc_init(void)
>  {
> +	uint i;
> +
>  	/* Make sure the IOMMU PC resource is available */
>  	if (!amd_iommu_pc_supported())
>  		return -ENODEV;
>  
> -	_init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
> +	for (i = 0 ; i < amd_iommu_get_num_iommus(); i++) {
> +		int ret;
> +		struct perf_amd_iommu *pi;
> +
> +		pi = kzalloc(sizeof(struct perf_amd_iommu), GFP_KERNEL);
> +		if (!pi)
> +			return -ENOMEM;
> +
> +		ret = init_one_perf_amd_iommu(pi, i);
> +		if (ret)
> +			return ret;

You're leaking here memory if some IOMMUs fail and that function returns
-EINVAL from the max_banks and counters test above.

So you need to unwind all that setup init_one_perf_amd_iommu() did
for the iommus from 0 up to i - 1; IOW; you need to move the call to
amd_iommu_pc_exit() here and not in the init function.

That'll be all for now. :-)

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.