[PATCH V4 0/6] perf/amd/iommu: Enable multi-IOMMU support

[PATCH V4 0/6] perf/amd/iommu: Enable multi-IOMMU support

[Suravee Suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

This patch series modifies the existing perf_event_amd_iommu driver
to support systems with multiple IOMMUs. It introduces new AMD IOMMU APIs,
which are used by the AMD IOMMU Perf driver to access performance
counters in multiple IOMMUs.

In addition, this series should also fix current AMD IOMMU PMU driver
initialization issue in some existing KV and CZ platform, where it fails
to write to IOMMU perf counter as reported by Andreas Hartmann here
(http://comments.gmane.org/gmane.linux.kernel.pci/49147).

Git branch containing this patch series is available here:

    https://github.com/ssuthiku/linux.git  perf-iommu-v4

Changes from V3 (https://lkml.org/lkml/2016/2/9/845)
  * Rebase the code to tip/master per Boris suggestion
  * Most changes are in patch 5/6:
    * Fix several spelling and styling issues per Boris review comment
    * Remove unnecessary pr_debug in the perf amd iommu driver (per Boris)
    * Rename several function to make it less confusing (per Boris)
    * Properly handle errors when fails to set registers/counters
      on multiple IOMMUs. (per Boris)

Changes from V2 ( https://lkml.org/lkml/2016/1/1/141)
  * Ported to 4.5.0-rc2
  * Add reviewed by Joerg for patch 1 and 2
  * Remove EXPORT_SYMBOL from patch 3 (per Joerg suggestion)
  * Merge patch 4/6 and 6/6 from V2 into 5/5 in V3 and add
    more description in the commit message and in code comment.
  * Patch 5: modify the logic to update counts to get rid off
    un-necessary local64_cmpxchg().

Changes from V1 (https://lkml.org/lkml/2015/12/22/535):
  * Update patch3 and 6 to use amd_iommus_present instead of introducing
    amd_iommu_cnt static variable since they are the same thing

Suravee Suthikulpanit (6):
  perf/amd/iommu: Consolidate and move perf_event_amd_iommu header
  perf/amd/iommu: Modify functions to query max banks and counters
  iommu/amd: Introduce amd_iommu_get_num_iommus()
  perf/amd/iommu: Introduce get_iommu_bnk_cnt_evt_idx
  perf/amd/iommu: Enable support for multiple IOMMUs
  perf/amd/iommu: Declar pr_fmt and remove unnecessary pr_debug

 arch/x86/events/amd/iommu.c           | 197 ++++++++++++++++++++++------------
 arch/x86/events/amd/iommu.h           |  40 -------
 arch/x86/include/asm/perf/amd/iommu.h |  42 ++++++++
 drivers/iommu/amd_iommu_init.c        | 136 +++++++++++++++++++----
 drivers/iommu/amd_iommu_proto.h       |   7 --
 5 files changed, 286 insertions(+), 136 deletions(-)
 delete mode 100644 arch/x86/events/amd/iommu.h
 create mode 100644 arch/x86/include/asm/perf/amd/iommu.h

-- 
2.5.0

[PATCH V4 1/6] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Suravee Suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

First, this patch move arch/x86/events/amd/iommu.h to
arch/x86/include/asm/perf/amd/iommu.h so that we easily include
it in both perf-amd-iommu and amd-iommu drivers.

Then, we consolidate declarion of AMD IOMMU performance counter
APIs into one file.

Reviewed-by: Joerg Roedel <jroedel@suse.de>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c           |  2 +-
 arch/x86/events/amd/iommu.h           | 40 ---------------------------------
 arch/x86/include/asm/perf/amd/iommu.h | 42 +++++++++++++++++++++++++++++++++++
 drivers/iommu/amd_iommu_init.c        |  2 ++
 drivers/iommu/amd_iommu_proto.h       |  7 ------
 5 files changed, 45 insertions(+), 48 deletions(-)
 delete mode 100644 arch/x86/events/amd/iommu.h
 create mode 100644 arch/x86/include/asm/perf/amd/iommu.h

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 629bc70..2f96072 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -15,9 +15,9 @@
 #include <linux/module.h>
 #include <linux/cpumask.h>
 #include <linux/slab.h>
+#include <asm/perf/amd/iommu.h>
 
 #include "../../kernel/cpu/perf_event.h"
-#include "iommu.h"
 
 #define COUNTER_SHIFT		16
 
diff --git a/arch/x86/events/amd/iommu.h b/arch/x86/events/amd/iommu.h
deleted file mode 100644
index 845d173..0000000
--- a/arch/x86/events/amd/iommu.h
+++ /dev/null
@@ -1,40 +0,0 @@
-/*
- * Copyright (C) 2013 Advanced Micro Devices, Inc.
- *
- * Author: Steven Kinney <Steven.Kinney@amd.com>
- * Author: Suravee Suthikulpanit <Suraveee.Suthikulpanit@amd.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#ifndef _PERF_EVENT_AMD_IOMMU_H_
-#define _PERF_EVENT_AMD_IOMMU_H_
-
-/* iommu pc mmio region register indexes */
-#define IOMMU_PC_COUNTER_REG			0x00
-#define IOMMU_PC_COUNTER_SRC_REG		0x08
-#define IOMMU_PC_PASID_MATCH_REG		0x10
-#define IOMMU_PC_DOMID_MATCH_REG		0x18
-#define IOMMU_PC_DEVID_MATCH_REG		0x20
-#define IOMMU_PC_COUNTER_REPORT_REG		0x28
-
-/* maximun specified bank/counters */
-#define PC_MAX_SPEC_BNKS			64
-#define PC_MAX_SPEC_CNTRS			16
-
-/* iommu pc reg masks*/
-#define IOMMU_BASE_DEVID			0x0000
-
-/* amd_iommu_init.c external support functions */
-extern bool amd_iommu_pc_supported(void);
-
-extern u8 amd_iommu_pc_get_max_banks(u16 devid);
-
-extern u8 amd_iommu_pc_get_max_counters(u16 devid);
-
-extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr,
-			u8 fxn, u64 *value, bool is_write);
-
-#endif /*_PERF_EVENT_AMD_IOMMU_H_*/
diff --git a/arch/x86/include/asm/perf/amd/iommu.h b/arch/x86/include/asm/perf/amd/iommu.h
new file mode 100644
index 0000000..72f64b7
--- /dev/null
+++ b/arch/x86/include/asm/perf/amd/iommu.h
@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) 2013 Advanced Micro Devices, Inc.
+ *
+ * Author: Steven Kinney <Steven.Kinney@amd.com>
+ * Author: Suravee Suthikulpanit <Suraveee.Suthikulpanit@amd.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _PERF_EVENT_AMD_IOMMU_H_
+#define _PERF_EVENT_AMD_IOMMU_H_
+
+/* iommu pc mmio region register indexes */
+#define IOMMU_PC_COUNTER_REG			0x00
+#define IOMMU_PC_COUNTER_SRC_REG		0x08
+#define IOMMU_PC_PASID_MATCH_REG		0x10
+#define IOMMU_PC_DOMID_MATCH_REG		0x18
+#define IOMMU_PC_DEVID_MATCH_REG		0x20
+#define IOMMU_PC_COUNTER_REPORT_REG		0x28
+
+/* maximum specified bank/counters */
+#define PC_MAX_SPEC_BNKS			64
+#define PC_MAX_SPEC_CNTRS			16
+
+/* iommu pc reg masks*/
+#define IOMMU_BASE_DEVID			0x0000
+
+/* amd_iommu_init.c external support functions */
+bool amd_iommu_pc_supported(void);
+
+u8 amd_iommu_pc_get_max_banks(u16 devid);
+
+u8 amd_iommu_pc_get_max_counters(u16 devid);
+
+int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value);
+
+int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
+				 u64 *value, bool is_write);
+
+#endif /*_PERF_EVENT_AMD_IOMMU_H_*/
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 013bdff..d30f4b2 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -27,6 +27,8 @@
 #include <linux/amd-iommu.h>
 #include <linux/export.h>
 #include <linux/iommu.h>
+#include <asm/perf/amd/iommu.h>
+
 #include <asm/pci-direct.h>
 #include <asm/iommu.h>
 #include <asm/gart.h>
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index 0bd9eb3..ac2da91 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -55,13 +55,6 @@ extern int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, int pasid,
 extern int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, int pasid);
 extern struct iommu_domain *amd_iommu_get_v2_domain(struct pci_dev *pdev);
 
-/* IOMMU Performance Counter functions */
-extern bool amd_iommu_pc_supported(void);
-extern u8 amd_iommu_pc_get_max_banks(u16 devid);
-extern u8 amd_iommu_pc_get_max_counters(u16 devid);
-extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
-				    u64 *value, bool is_write);
-
 #ifdef CONFIG_IRQ_REMAP
 extern int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
 #else
-- 
2.5.0

[PATCH V4 2/6] perf/amd/iommu: Modify functions to query max banks and counters

[Suravee Suthikulpanit]

Currently, amd_iommu_pc_get_max_[banks|counters]() require devid,
which should not be the case. Also, these don't properly support
multi-IOMMU system.

Current and future AMD systems with IOMMU that support perf counter
would likely contain homogeneous IOMMUs where multiple IOMMUs are
availalbe. So, this patch modifies these function to iterate all IOMMU
to check the max banks and counters reported by the hardware.

Reviewed-by: Joerg Roedel <jroedel@suse.de>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c           | 17 +++++++----------
 arch/x86/include/asm/perf/amd/iommu.h |  7 ++-----
 drivers/iommu/amd_iommu_init.c        | 20 ++++++++++++--------
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 2f96072..debf22d 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -232,14 +232,6 @@ static int perf_iommu_event_init(struct perf_event *event)
 		return -EINVAL;
 	}
 
-	/* integrate with iommu base devid (0000), assume one iommu */
-	perf_iommu->max_banks =
-		amd_iommu_pc_get_max_banks(IOMMU_BASE_DEVID);
-	perf_iommu->max_counters =
-		amd_iommu_pc_get_max_counters(IOMMU_BASE_DEVID);
-	if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0))
-		return -EINVAL;
-
 	/* update the hw_perf_event struct with the iommu config data */
 	hwc->config = config;
 	hwc->extra_reg.config = config1;
@@ -450,6 +442,11 @@ static __init int _init_perf_amd_iommu(
 	if (_init_events_attrs(perf_iommu) != 0)
 		pr_err("perf: amd_iommu: Only support raw events.\n");
 
+	perf_iommu->max_banks = amd_iommu_pc_get_max_banks();
+	perf_iommu->max_counters = amd_iommu_pc_get_max_counters();
+	if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0))
+		return -EINVAL;
+
 	/* Init null attributes */
 	perf_iommu->null_group = NULL;
 	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
@@ -460,8 +457,8 @@ static __init int _init_perf_amd_iommu(
 		amd_iommu_pc_exit();
 	} else {
 		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
-			amd_iommu_pc_get_max_banks(IOMMU_BASE_DEVID),
-			amd_iommu_pc_get_max_counters(IOMMU_BASE_DEVID));
+			amd_iommu_pc_get_max_banks(),
+			amd_iommu_pc_get_max_counters());
 	}
 
 	return ret;
diff --git a/arch/x86/include/asm/perf/amd/iommu.h b/arch/x86/include/asm/perf/amd/iommu.h
index 72f64b7..97e1be5 100644
--- a/arch/x86/include/asm/perf/amd/iommu.h
+++ b/arch/x86/include/asm/perf/amd/iommu.h
@@ -24,15 +24,12 @@
 #define PC_MAX_SPEC_BNKS			64
 #define PC_MAX_SPEC_CNTRS			16
 
-/* iommu pc reg masks*/
-#define IOMMU_BASE_DEVID			0x0000
-
 /* amd_iommu_init.c external support functions */
 bool amd_iommu_pc_supported(void);
 
-u8 amd_iommu_pc_get_max_banks(u16 devid);
+u8 amd_iommu_pc_get_max_banks(void);
 
-u8 amd_iommu_pc_get_max_counters(u16 devid);
+u8 amd_iommu_pc_get_max_counters(void);
 
 int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value);
 
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index d30f4b2..a62b5ce 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2251,15 +2251,17 @@ EXPORT_SYMBOL(amd_iommu_v2_supported);
  *
  ****************************************************************************/
 
-u8 amd_iommu_pc_get_max_banks(u16 devid)
+u8 amd_iommu_pc_get_max_banks(void)
 {
 	struct amd_iommu *iommu;
 	u8 ret = 0;
 
-	/* locate the iommu governing the devid */
-	iommu = amd_iommu_rlookup_table[devid];
-	if (iommu)
+	for_each_iommu(iommu) {
+		if (!iommu->max_banks ||
+		    (ret && (iommu->max_banks != ret)))
+			return 0;
 		ret = iommu->max_banks;
+	}
 
 	return ret;
 }
@@ -2271,15 +2273,17 @@ bool amd_iommu_pc_supported(void)
 }
 EXPORT_SYMBOL(amd_iommu_pc_supported);
 
-u8 amd_iommu_pc_get_max_counters(u16 devid)
+u8 amd_iommu_pc_get_max_counters(void)
 {
 	struct amd_iommu *iommu;
 	u8 ret = 0;
 
-	/* locate the iommu governing the devid */
-	iommu = amd_iommu_rlookup_table[devid];
-	if (iommu)
+	for_each_iommu(iommu) {
+		if (!iommu->max_counters ||
+		    (ret && (iommu->max_counters != ret)))
+			return 0;
 		ret = iommu->max_counters;
+	}
 
 	return ret;
 }
-- 
2.5.0

Re: [PATCH V4 2/6] perf/amd/iommu: Modify functions to query max banks and counters

[Borislav Petkov]

On Thu, Feb 11, 2016 at 04:15:23PM +0700, Suravee Suthikulpanit wrote:
> Currently, amd_iommu_pc_get_max_[banks|counters]() require devid,
> which should not be the case.

Why?

Commit message could use an explanation.

> Also, these don't properly support
> multi-IOMMU system.
> 
> Current and future AMD systems with IOMMU that support perf counter

				"with an IOMMU that supports performance counters"

> would likely contain homogeneous IOMMUs where multiple IOMMUs are

What are homogeneous IOMMUs?

> availalbe. So, this patch modifies these function to iterate all IOMMU

Please integrate a spellchecker in your patch creation workflow:

s/availalbe/available/

> to check the max banks and counters reported by the hardware.
> 
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c           | 17 +++++++----------
>  arch/x86/include/asm/perf/amd/iommu.h |  7 ++-----
>  drivers/iommu/amd_iommu_init.c        | 20 ++++++++++++--------
>  3 files changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 2f96072..debf22d 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -232,14 +232,6 @@ static int perf_iommu_event_init(struct perf_event *event)
>  		return -EINVAL;
>  	}
>  
> -	/* integrate with iommu base devid (0000), assume one iommu */
> -	perf_iommu->max_banks =
> -		amd_iommu_pc_get_max_banks(IOMMU_BASE_DEVID);
> -	perf_iommu->max_counters =
> -		amd_iommu_pc_get_max_counters(IOMMU_BASE_DEVID);
> -	if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0))
> -		return -EINVAL;
> -
>  	/* update the hw_perf_event struct with the iommu config data */
>  	hwc->config = config;
>  	hwc->extra_reg.config = config1;
> @@ -450,6 +442,11 @@ static __init int _init_perf_amd_iommu(

Btw, that _init_perf_amd_iommu is unnecessarily split from
amd_iommu_pc_init(). You should merge those two. But that's another
patch. In that same cleanup patch you could do

s/perf_iommu/pi/g

or so because that perf_iommu local var is unnecesarily long and impairs
readability.

>  	if (_init_events_attrs(perf_iommu) != 0)
>  		pr_err("perf: amd_iommu: Only support raw events.\n");
>  
> +	perf_iommu->max_banks = amd_iommu_pc_get_max_banks();
> +	perf_iommu->max_counters = amd_iommu_pc_get_max_counters();
> +	if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0))

Simplify:

	if (!perf_iommu->max_banks ||
	    !perf_iommu->max_counters)

> +		return -EINVAL;
> +
>  	/* Init null attributes */
>  	perf_iommu->null_group = NULL;
>  	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
> @@ -460,8 +457,8 @@ static __init int _init_perf_amd_iommu(
>  		amd_iommu_pc_exit();
>  	} else {
>  		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
> -			amd_iommu_pc_get_max_banks(IOMMU_BASE_DEVID),
> -			amd_iommu_pc_get_max_counters(IOMMU_BASE_DEVID));
> +			amd_iommu_pc_get_max_banks(),
> +			amd_iommu_pc_get_max_counters());
>  	}
>  
>  	return ret;
> diff --git a/arch/x86/include/asm/perf/amd/iommu.h b/arch/x86/include/asm/perf/amd/iommu.h
> index 72f64b7..97e1be5 100644
> --- a/arch/x86/include/asm/perf/amd/iommu.h
> +++ b/arch/x86/include/asm/perf/amd/iommu.h
> @@ -24,15 +24,12 @@
>  #define PC_MAX_SPEC_BNKS			64
>  #define PC_MAX_SPEC_CNTRS			16
>  
> -/* iommu pc reg masks*/
> -#define IOMMU_BASE_DEVID			0x0000
> -
>  /* amd_iommu_init.c external support functions */
>  bool amd_iommu_pc_supported(void);
>  
> -u8 amd_iommu_pc_get_max_banks(u16 devid);
> +u8 amd_iommu_pc_get_max_banks(void);
>  
> -u8 amd_iommu_pc_get_max_counters(u16 devid);
> +u8 amd_iommu_pc_get_max_counters(void);
>  
>  int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value);
>  
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index d30f4b2..a62b5ce 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -2251,15 +2251,17 @@ EXPORT_SYMBOL(amd_iommu_v2_supported);
>   *
>   ****************************************************************************/
>  
> -u8 amd_iommu_pc_get_max_banks(u16 devid)
> +u8 amd_iommu_pc_get_max_banks(void)
>  {
>  	struct amd_iommu *iommu;
>  	u8 ret = 0;
>  
> -	/* locate the iommu governing the devid */
> -	iommu = amd_iommu_rlookup_table[devid];
> -	if (iommu)
> +	for_each_iommu(iommu) {
> +		if (!iommu->max_banks ||
> +		    (ret && (iommu->max_banks != ret)))

What is that supposed to do?

Check that the max_banks of a previous IOMMU are == to the max_banks of
the current IOMMU?

Why? Could definitely use a comment.

> +			return 0;
>  		ret = iommu->max_banks;
> +	}
>  
>  	return ret;
>  }
> @@ -2271,15 +2273,17 @@ bool amd_iommu_pc_supported(void)
>  }
>  EXPORT_SYMBOL(amd_iommu_pc_supported);
>  
> -u8 amd_iommu_pc_get_max_counters(u16 devid)
> +u8 amd_iommu_pc_get_max_counters(void)
>  {
>  	struct amd_iommu *iommu;
>  	u8 ret = 0;
>  
> -	/* locate the iommu governing the devid */
> -	iommu = amd_iommu_rlookup_table[devid];
> -	if (iommu)
> +	for_each_iommu(iommu) {
> +		if (!iommu->max_counters ||
> +		    (ret && (iommu->max_counters != ret)))

Ditto.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH V4 2/6] perf/amd/iommu: Modify functions to query max banks and counters

[Suravee Suthikulpanit]

Hi,

On 02/18/2016 06:11 PM, Borislav Petkov wrote:
> On Thu, Feb 11, 2016 at 04:15:23PM +0700, Suravee Suthikulpanit wrote:
>> Currently, amd_iommu_pc_get_max_[banks|counters]() require devid,
>> which should not be the case.
>
> Why?
>
> Commit message could use an explanation.
>
>> Also, these don't properly support
>> multi-IOMMU system.
>>
>> Current and future AMD systems with IOMMU that support perf counter
>
> 				"with an IOMMU that supports performance counters"
>
>> would likely contain homogeneous IOMMUs where multiple IOMMUs are
>
> What are homogeneous IOMMUs?

I intended to mean the same IOMMU version/capability for all IOMMUs in 
the system.

>
>> availalbe. So, this patch modifies these function to iterate all IOMMU
>
> Please integrate a spellchecker in your patch creation workflow:
>
> s/availalbe/available/
>

Thanks. I have now rephrased and spell check the new commit message for 
the V5.

>>
>> Reviewed-by: Joerg Roedel <jroedel@suse.de>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> ---
>>   arch/x86/events/amd/iommu.c           | 17 +++++++----------
>>   arch/x86/include/asm/perf/amd/iommu.h |  7 ++-----
>>   drivers/iommu/amd_iommu_init.c        | 20 ++++++++++++--------
>>   3 files changed, 21 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
>> index 2f96072..debf22d 100644
>> --- a/arch/x86/events/amd/iommu.c
>> +++ b/arch/x86/events/amd/iommu.c
>> @@ -232,14 +232,6 @@ static int perf_iommu_event_init(struct perf_event *event)
>>   		return -EINVAL;
>>   	}
>>
>> -	/* integrate with iommu base devid (0000), assume one iommu */
>> -	perf_iommu->max_banks =
>> -		amd_iommu_pc_get_max_banks(IOMMU_BASE_DEVID);
>> -	perf_iommu->max_counters =
>> -		amd_iommu_pc_get_max_counters(IOMMU_BASE_DEVID);
>> -	if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0))
>> -		return -EINVAL;
>> -
>>   	/* update the hw_perf_event struct with the iommu config data */
>>   	hwc->config = config;
>>   	hwc->extra_reg.config = config1;
>> @@ -450,6 +442,11 @@ static __init int _init_perf_amd_iommu(
>
> Btw, that _init_perf_amd_iommu is unnecessarily split from
> amd_iommu_pc_init(). You should merge those two. But that's another
> patch. In that same cleanup patch you could do
>
> s/perf_iommu/pi/g
>
> or so because that perf_iommu local var is unnecesarily long and impairs
> readability.
>

Sure, I'll clean up both of these.

>>   	if (_init_events_attrs(perf_iommu) != 0)
>>   		pr_err("perf: amd_iommu: Only support raw events.\n");
>>
>> +	perf_iommu->max_banks = amd_iommu_pc_get_max_banks();
>> +	perf_iommu->max_counters = amd_iommu_pc_get_max_counters();
>> +	if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0))
>
> Simplify:
>
> 	if (!perf_iommu->max_banks ||
> 	    !perf_iommu->max_counters)

Ok

[....]
>> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
>> index d30f4b2..a62b5ce 100644
>> --- a/drivers/iommu/amd_iommu_init.c
>> +++ b/drivers/iommu/amd_iommu_init.c
>> @@ -2251,15 +2251,17 @@ EXPORT_SYMBOL(amd_iommu_v2_supported);
>>    *
>>    ****************************************************************************/
>>
>> -u8 amd_iommu_pc_get_max_banks(u16 devid)
>> +u8 amd_iommu_pc_get_max_banks(void)
>>   {
>>   	struct amd_iommu *iommu;
>>   	u8 ret = 0;
>>
>> -	/* locate the iommu governing the devid */
>> -	iommu = amd_iommu_rlookup_table[devid];
>> -	if (iommu)
>> +	for_each_iommu(iommu) {
>> +		if (!iommu->max_banks ||
>> +		    (ret && (iommu->max_banks != ret)))
>
> What is that supposed to do?
>
> Check that the max_banks of a previous IOMMU are == to the max_banks of
> the current IOMMU?
>
> Why? Could definitely use a comment.

Current AMD IOMMU perf implementation assumes that all IOMMUs must have
the same value of max_counters. Therefore, this logic iterates through 
all IOMMUs to check this. I'll add the comment here.

Thanks,
Suravee

[PATCH V4 3/6] iommu/amd: Introduce amd_iommu_get_num_iommus()

[Suravee Suthikulpanit]

This patch introduces amd_iommu_get_num_iommus(). Initially, this is
intended to be used by Perf AMD IOMMU driver.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/include/asm/perf/amd/iommu.h | 2 ++
 drivers/iommu/amd_iommu_init.c        | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/perf/amd/iommu.h b/arch/x86/include/asm/perf/amd/iommu.h
index 97e1be5..40919eb 100644
--- a/arch/x86/include/asm/perf/amd/iommu.h
+++ b/arch/x86/include/asm/perf/amd/iommu.h
@@ -25,6 +25,8 @@
 #define PC_MAX_SPEC_CNTRS			16
 
 /* amd_iommu_init.c external support functions */
+int amd_iommu_get_num_iommus(void);
+
 bool amd_iommu_pc_supported(void);
 
 u8 amd_iommu_pc_get_max_banks(void);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index a62b5ce..ffa057e 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1030,7 +1030,7 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h)
 
 	/* Add IOMMU to internal data structures */
 	list_add_tail(&iommu->list, &amd_iommu_list);
-	iommu->index             = amd_iommus_present++;
+	iommu->index = amd_iommus_present++;
 
 	if (unlikely(iommu->index >= MAX_IOMMUS)) {
 		WARN(1, "AMD-Vi: System has more IOMMUs than supported by this driver\n");
@@ -2244,6 +2244,11 @@ bool amd_iommu_v2_supported(void)
 }
 EXPORT_SYMBOL(amd_iommu_v2_supported);
 
+int amd_iommu_get_num_iommus(void)
+{
+	return amd_iommus_present;
+}
+
 /****************************************************************************
  *
  * IOMMU EFR Performance Counter support functionality. This code allows
-- 
2.5.0

[PATCH V4 4/6] perf/amd/iommu: Introduce get_iommu_bnk_cnt_evt_idx

[Suravee Suthikulpanit]

Introduce a helper function to calculate bit-index for assigning
performance counter assignment.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index debf22d..812eff2 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -145,18 +145,27 @@ static struct attribute_group amd_iommu_cpumask_group = {
 
 /*---------------------------------------------*/
 
+static inline
+int get_iommu_bnk_cnt_evt_idx(struct perf_amd_iommu *perf_iommu,
+				  int iommu_index, int bank_index,
+				  int cntr_index)
+{
+	int cntrs_per_iommu = perf_iommu->max_banks * perf_iommu->max_counters;
+	int index = (perf_iommu->max_counters * bank_index) + cntr_index;
+
+	return (cntrs_per_iommu * iommu_index) + index;
+}
+
 static int get_next_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu)
 {
 	unsigned long flags;
 	int shift, bank, cntr, retval;
-	int max_banks = perf_iommu->max_banks;
-	int max_cntrs = perf_iommu->max_counters;
 
 	raw_spin_lock_irqsave(&perf_iommu->lock, flags);
 
-	for (bank = 0, shift = 0; bank < max_banks; bank++) {
-		for (cntr = 0; cntr < max_cntrs; cntr++) {
-			shift = bank + (bank*3) + cntr;
+	for (bank = 0, shift = 0; bank < perf_iommu->max_banks; bank++) {
+		for (cntr = 0; cntr < perf_iommu->max_counters; cntr++) {
+			shift = get_iommu_bnk_cnt_evt_idx(perf_iommu, 0, bank, cntr);
 			if (perf_iommu->cntr_assign_mask & (1ULL<<shift)) {
 				continue;
 			} else {
-- 
2.5.0

Re: [PATCH V4 4/6] perf/amd/iommu: Introduce get_iommu_bnk_cnt_evt_idx

[Borislav Petkov]

On Thu, Feb 11, 2016 at 04:15:25PM +0700, Suravee Suthikulpanit wrote:
> Introduce a helper function to calculate bit-index for assigning
> performance counter assignment.
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index debf22d..812eff2 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -145,18 +145,27 @@ static struct attribute_group amd_iommu_cpumask_group = {
>  
>  /*---------------------------------------------*/
>  
> +static inline
> +int get_iommu_bnk_cnt_evt_idx(struct perf_amd_iommu *perf_iommu,
> +				  int iommu_index, int bank_index,
> +				  int cntr_index)

Align args at the opening brace.

> +{
> +	int cntrs_per_iommu = perf_iommu->max_banks * perf_iommu->max_counters;
> +	int index = (perf_iommu->max_counters * bank_index) + cntr_index;
> +
> +	return (cntrs_per_iommu * iommu_index) + index;
> +}
> +
>  static int get_next_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu)
>  {
>  	unsigned long flags;
>  	int shift, bank, cntr, retval;
> -	int max_banks = perf_iommu->max_banks;
> -	int max_cntrs = perf_iommu->max_counters;
>  
>  	raw_spin_lock_irqsave(&perf_iommu->lock, flags);
>  
> -	for (bank = 0, shift = 0; bank < max_banks; bank++) {
> -		for (cntr = 0; cntr < max_cntrs; cntr++) {
> -			shift = bank + (bank*3) + cntr;
> +	for (bank = 0, shift = 0; bank < perf_iommu->max_banks; bank++) {

Please init that shift variable above in the function entry - it
confuses unnecessarily here as if it had anything to do with the loop
stride.

> +		for (cntr = 0; cntr < perf_iommu->max_counters; cntr++) {
> +			shift = get_iommu_bnk_cnt_evt_idx(perf_iommu, 0, bank, cntr);

\n here

>  			if (perf_iommu->cntr_assign_mask & (1ULL<<shift)) {
>  				continue;
>  			} else {
> -- 
> 2.5.0
> 
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCH V4 5/6] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

The current amd_iommu_pc_get_set_reg_val() does not support muli-IOMMU
system. This patch replace amd_iommu_pc_get_set_reg_val() with
amd_iommu_pc_set_reg_val() and amd_iommu_pc_[set|get]_cnt_vals().

Also, the current struct hw_perf_event.prev_count can only store the
previous counter value only from one IOMMU. So, this patch introduces
a new data structure, perf_amd_iommu.prev_cnts, to track previous value
of each performance counter of each bank of each IOMMU.

Last, this patch introduce perf_iommu_cnts to temporary hold counter
values from each IOMMU for internal use when manages counters among
multiple IOMMUs.

Note that this implementation makes an assumption that the counters
on all IOMMUs will be programmed the same way (i.e with the same events).

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c           | 146 +++++++++++++++++++++++-----------
 arch/x86/include/asm/perf/amd/iommu.h |   7 +-
 drivers/iommu/amd_iommu_init.c        | 107 ++++++++++++++++++++++---
 3 files changed, 201 insertions(+), 59 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 812eff2..947ae8a 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -42,6 +42,13 @@ struct perf_amd_iommu {
 	u64 cntr_assign_mask;
 	raw_spinlock_t lock;
 	const struct attribute_group *attr_groups[4];
+
+	/*
+	 * This is a 3D array used to store the previous count values
+	 * from each performance counter of each bank of each IOMMU.
+	 * I.E. size of array = (num iommus * num banks * num counters)
+	 */
+	local64_t *prev_cnts;
 };
 
 #define format_group	attr_groups[0]
@@ -121,6 +128,12 @@ static struct amd_iommu_event_desc amd_iommu_v2_event_descs[] = {
 	{ /* end: all zeroes */ },
 };
 
+/*
+ * This is an array used to temporary hold the current values
+ * read from a particular perf counter from each IOMMU.
+ */
+static u64 *perf_iommu_cnts;
+
 /*---------------------------------------------
  * sysfs cpumask attributes
  *---------------------------------------------*/
@@ -255,44 +268,42 @@ static void perf_iommu_enable_event(struct perf_event *ev)
 	u64 reg = 0ULL;
 
 	reg = csource;
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_COUNTER_SRC_REG, &reg, true);
+	amd_iommu_pc_set_reg(devid, _GET_BANK(ev), _GET_CNTR(ev),
+			     IOMMU_PC_COUNTER_SRC_REG, &reg);
 
-	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);
+	reg = devid | (_GET_DEVID_MASK(ev) << 32);
 	if (reg)
-		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DEVID_MATCH_REG, &reg, true);
+		reg |= BIT(31);
+	amd_iommu_pc_set_reg(devid, _GET_BANK(ev), _GET_CNTR(ev),
+			     IOMMU_PC_DEVID_MATCH_REG, &reg);
 
-	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
+	reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
 	if (reg)
-		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_PASID_MATCH_REG, &reg, true);
+		reg |= BIT(31);
+	amd_iommu_pc_set_reg(devid, _GET_BANK(ev), _GET_CNTR(ev),
+			     IOMMU_PC_PASID_MATCH_REG, &reg);
 
-	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
+	reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
 	if (reg)
-		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DOMID_MATCH_REG, &reg, true);
+		reg |= BIT(31);
+	amd_iommu_pc_set_reg(devid, _GET_BANK(ev), _GET_CNTR(ev),
+			     IOMMU_PC_DOMID_MATCH_REG, &reg);
 }
 
 static void perf_iommu_disable_event(struct perf_event *event)
 {
 	u64 reg = 0ULL;
 
-	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
-			_GET_BANK(event), _GET_CNTR(event),
-			IOMMU_PC_COUNTER_SRC_REG, &reg, true);
+	amd_iommu_pc_set_reg(_GET_DEVID(event), _GET_BANK(event),
+			     _GET_CNTR(event), IOMMU_PC_COUNTER_SRC_REG, &reg);
 }
 
 static void perf_iommu_start(struct perf_event *event, int flags)
 {
 	struct hw_perf_event *hwc = &event->hw;
+	struct perf_amd_iommu *perf_iommu = container_of(event->pmu,
+							 struct perf_amd_iommu,
+							 pmu);
 
 	pr_debug("perf: amd_iommu:perf_iommu_start\n");
 	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
@@ -302,10 +313,19 @@ static void perf_iommu_start(struct perf_event *event, int flags)
 	hwc->state = 0;
 
 	if (flags & PERF_EF_RELOAD) {
-		u64 prev_raw_count =  local64_read(&hwc->prev_count);
-		amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
-				_GET_BANK(event), _GET_CNTR(event),
-				IOMMU_PC_COUNTER_REG, &prev_raw_count, true);
+		int i;
+
+		for (i = 0; i < amd_iommu_get_num_iommus(); i++) {
+			int index = get_iommu_bnk_cnt_evt_idx(perf_iommu, i,
+							      _GET_BANK(event),
+							      _GET_CNTR(event));
+
+			perf_iommu_cnts[i] = local64_read(&perf_iommu->prev_cnts[index]);
+		}
+
+		amd_iommu_pc_set_counters(_GET_BANK(event), _GET_CNTR(event),
+					  amd_iommu_get_num_iommus(),
+					  perf_iommu_cnts);
 	}
 
 	perf_iommu_enable_event(event);
@@ -315,29 +335,45 @@ static void perf_iommu_start(struct perf_event *event, int flags)
 
 static void perf_iommu_read(struct perf_event *event)
 {
-	u64 count = 0ULL;
-	u64 prev_raw_count = 0ULL;
+	int i;
 	u64 delta = 0ULL;
 	struct hw_perf_event *hwc = &event->hw;
-	pr_debug("perf: amd_iommu:perf_iommu_read\n");
-
-	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
-				_GET_BANK(event), _GET_CNTR(event),
-				IOMMU_PC_COUNTER_REG, &count, false);
-
-	/* IOMMU pc counter register is only 48 bits */
-	count &= 0xFFFFFFFFFFFFULL;
+	struct perf_amd_iommu *perf_iommu = container_of(event->pmu,
+							 struct perf_amd_iommu,
+							 pmu);
 
-	prev_raw_count =  local64_read(&hwc->prev_count);
-	if (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
-					count) != prev_raw_count)
+	if (amd_iommu_pc_get_counters(_GET_BANK(event), _GET_CNTR(event),
+				      amd_iommu_get_num_iommus(),
+				      perf_iommu_cnts))
 		return;
 
-	/* Handling 48-bit counter overflowing */
-	delta = (count << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT);
-	delta >>= COUNTER_SHIFT;
-	local64_add(delta, &event->count);
-
+	/*
+	 * Now we re-aggregating event counts and prev-counts
+	 * from all IOMMUs
+	 */
+	local64_set(&hwc->prev_count, 0);
+
+	for (i = 0; i < amd_iommu_get_num_iommus(); i++) {
+		int indx = get_iommu_bnk_cnt_evt_idx(perf_iommu, i,
+						     _GET_BANK(event),
+						     _GET_CNTR(event));
+		u64 prev_raw_count = local64_read(&perf_iommu->prev_cnts[indx]);
+
+		/* IOMMU pc counter register is only 48 bits */
+		perf_iommu_cnts[i] &= GENMASK_ULL(48, 0);
+
+		/*
+		 * Since we do not enable counter overflow interrupts,
+		 * we do not have to worry about prev_count changing on us
+		 */
+		local64_set(&perf_iommu->prev_cnts[indx], perf_iommu_cnts[i]);
+		local64_add(prev_raw_count, &hwc->prev_count);
+
+		/* Handle 48-bit counter overflow */
+		delta = (perf_iommu_cnts[i] << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT);
+		delta >>= COUNTER_SHIFT;
+		local64_add(delta, &event->count);
+	}
 }
 
 static void perf_iommu_stop(struct perf_event *event, int flags)
@@ -427,10 +463,14 @@ 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;
-	}
+	kfree(__perf_iommu.events_group);
+	__perf_iommu.events_group = NULL;
+
+	kfree(__perf_iommu.prev_cnts);
+	__perf_iommu.prev_cnts = NULL;
+
+	kfree(perf_iommu_cnts);
+	perf_iommu_cnts = NULL;
 }
 
 static __init int _init_perf_amd_iommu(
@@ -460,6 +500,18 @@ static __init int _init_perf_amd_iommu(
 	perf_iommu->null_group = NULL;
 	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
 
+	perf_iommu->prev_cnts = kzalloc(sizeof(*perf_iommu->prev_cnts) *
+					amd_iommu_get_num_iommus() *
+					perf_iommu->max_banks *
+					perf_iommu->max_counters, GFP_KERNEL);
+	if (!perf_iommu->prev_cnts)
+		return -ENOMEM;
+
+	perf_iommu_cnts = kzalloc(sizeof(*perf_iommu_cnts) * amd_iommu_get_num_iommus(),
+				  GFP_KERNEL);
+	if (!perf_iommu_cnts)
+		return -ENOMEM;
+
 	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
 	if (ret) {
 		pr_err("perf: amd_iommu: Failed to initialized.\n");
diff --git a/arch/x86/include/asm/perf/amd/iommu.h b/arch/x86/include/asm/perf/amd/iommu.h
index 40919eb..e0aa9be 100644
--- a/arch/x86/include/asm/perf/amd/iommu.h
+++ b/arch/x86/include/asm/perf/amd/iommu.h
@@ -33,9 +33,10 @@ u8 amd_iommu_pc_get_max_banks(void);
 
 u8 amd_iommu_pc_get_max_counters(void);
 
-int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value);
+int amd_iommu_pc_set_reg(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value);
 
-int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
-				 u64 *value, bool is_write);
+int amd_iommu_pc_set_counters(u8 bank, u8 cntr, int num, u64 *value);
+
+int amd_iommu_pc_get_counters(u8 bank, u8 cntr, int num, u64 *value);
 
 #endif /*_PERF_EVENT_AMD_IOMMU_H_*/
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index ffa057e..fd4f2b1 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1133,6 +1133,8 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	return 0;
 }
 
+static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
+				u8 fxn, u64 *value, bool is_write);
 
 static void init_iommu_perf_ctr(struct amd_iommu *iommu)
 {
@@ -1144,8 +1146,8 @@ static void init_iommu_perf_ctr(struct amd_iommu *iommu)
 	amd_iommu_pc_present = true;
 
 	/* Check if the performance counters can be written to */
-	if ((0 != amd_iommu_pc_get_set_reg_val(0, 0, 0, 0, &val, true)) ||
-	    (0 != amd_iommu_pc_get_set_reg_val(0, 0, 0, 0, &val2, false)) ||
+	if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) ||
+	    (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) ||
 	    (val != val2)) {
 		pr_err("AMD-Vi: Unable to write to IOMMU perf counter.\n");
 		amd_iommu_pc_present = false;
@@ -2294,10 +2296,9 @@ u8 amd_iommu_pc_get_max_counters(void)
 }
 EXPORT_SYMBOL(amd_iommu_pc_get_max_counters);
 
-int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
-				    u64 *value, bool is_write)
+static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
+				u8 fxn, u64 *value, bool is_write)
 {
-	struct amd_iommu *iommu;
 	u32 offset;
 	u32 max_offset_lim;
 
@@ -2305,9 +2306,6 @@ int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
 	if (!amd_iommu_pc_present)
 		return -ENODEV;
 
-	/* Locate the iommu associated with the device ID */
-	iommu = amd_iommu_rlookup_table[devid];
-
 	/* Check for valid iommu and pc register indexing */
 	if (WARN_ON((iommu == NULL) || (fxn > 0x28) || (fxn & 7)))
 		return -ENODEV;
@@ -2332,4 +2330,95 @@ int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
 
 	return 0;
 }
-EXPORT_SYMBOL(amd_iommu_pc_get_set_reg_val);
+
+int amd_iommu_pc_set_reg(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value)
+{
+	struct amd_iommu *iommu;
+	int ret, i = 0, j = 0;
+
+	for_each_iommu(iommu) {
+		ret = iommu_pc_get_set_reg(iommu, bank, cntr, fxn, value, true);
+		if (ret)
+			goto err_out;
+		i++;
+	}
+
+	return 0;
+err_out:
+	/* Revert all the PC registers previously set */
+	for_each_iommu(iommu) {
+		if (i == j)
+			break;
+		iommu_pc_get_set_reg(iommu, bank, cntr, fxn, 0, true);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(amd_iommu_pc_set_reg);
+
+int amd_iommu_pc_set_counters(u8 bank, u8 cntr, int num, u64 *value)
+{
+	struct amd_iommu *iommu;
+	int ret, i = 0, j = 0;
+
+	if (num > amd_iommus_present)
+		return -EINVAL;
+
+	for_each_iommu(iommu) {
+		ret = iommu_pc_get_set_reg(iommu, bank, cntr,
+					   IOMMU_PC_COUNTER_REG,
+					   &value[i], true);
+		if (ret)
+			goto err_out;
+		if (i++ == amd_iommus_present)
+			break;
+	}
+
+	return 0;
+err_out:
+	/* Revert all the PC counters previously set */
+	for_each_iommu(iommu) {
+		if (i == j)
+			break;
+		iommu_pc_get_set_reg(iommu, bank, cntr, IOMMU_PC_COUNTER_REG,
+				     0, true);
+		j++;
+	}
+	return ret;
+}
+EXPORT_SYMBOL(amd_iommu_pc_set_counters);
+
+int amd_iommu_pc_get_counters(u8 bank, u8 cntr, int num, u64 *value)
+{
+	struct amd_iommu *iommu;
+	int i = 0, ret;
+
+	if (!num)
+		return -EINVAL;
+
+	/*
+	 * Here, we read the specified counters on all IOMMUs,
+	 * which should have been programmed the same way and
+	 * aggregate the counter values.
+	 */
+	for_each_iommu(iommu) {
+		u64 tmp;
+
+		if (i >= num)
+			return -EINVAL;
+
+		ret = iommu_pc_get_set_reg(iommu, bank, cntr,
+					   IOMMU_PC_COUNTER_REG,
+					   &tmp, false);
+		if (ret)
+			return ret;
+
+		/* IOMMU pc counter register is only 48 bits */
+		value[i] = tmp & GENMASK_ULL(48, 0);
+
+		i++;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(amd_iommu_pc_get_counters);
-- 
2.5.0

Re: [PATCH V4 5/6] perf/amd/iommu: Enable support for multiple IOMMUs

[Peter Zijlstra]

On Thu, Feb 11, 2016 at 04:15:26PM +0700, Suravee Suthikulpanit wrote:
>  static void perf_iommu_read(struct perf_event *event)
>  {
> +	int i;
>  	u64 delta = 0ULL;
>  	struct hw_perf_event *hwc = &event->hw;
> +	struct perf_amd_iommu *perf_iommu = container_of(event->pmu,
> +							 struct perf_amd_iommu,
> +							 pmu);
>  
> +	if (amd_iommu_pc_get_counters(_GET_BANK(event), _GET_CNTR(event),
> +				      amd_iommu_get_num_iommus(),
> +				      perf_iommu_cnts))
>  		return;
>  
> +	/*
> +	 * Now we re-aggregating event counts and prev-counts
> +	 * from all IOMMUs
> +	 */
> +	local64_set(&hwc->prev_count, 0);
> +
> +	for (i = 0; i < amd_iommu_get_num_iommus(); i++) {
> +		int indx = get_iommu_bnk_cnt_evt_idx(perf_iommu, i,
> +						     _GET_BANK(event),
> +						     _GET_CNTR(event));
> +		u64 prev_raw_count = local64_read(&perf_iommu->prev_cnts[indx]);
> +
> +		/* IOMMU pc counter register is only 48 bits */
> +		perf_iommu_cnts[i] &= GENMASK_ULL(48, 0);
> +
> +		/*
> +		 * Since we do not enable counter overflow interrupts,
> +		 * we do not have to worry about prev_count changing on us
> +		 */
> +		local64_set(&perf_iommu->prev_cnts[indx], perf_iommu_cnts[i]);
> +		local64_add(prev_raw_count, &hwc->prev_count);
> +
> +		/* Handle 48-bit counter overflow */
> +		delta = (perf_iommu_cnts[i] << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT);
> +		delta >>= COUNTER_SHIFT;
> +		local64_add(delta, &event->count);
> +	}
>  }

So I really don't have time to review new muck while I'm hunting perf
core fail, but Boris made me look at this.

This is crazy, if you have multiple IOMMUs then create an event per
IOMMU, do _NOT_ fold them all into a single event.

In any case, the reason Boris asked me to look at this is that your
overflow handling is broken, you want delta to be s64. Otherwise:

	delta >>= COUNTER_SHIFT;

ends up as a SHR and you loose the MSB sign bits.

Re: [PATCH V4 5/6] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

Hi Peter,

On 02/18/2016 08:18 PM, Peter Zijlstra wrote:
> On Thu, Feb 11, 2016 at 04:15:26PM +0700, Suravee Suthikulpanit wrote:
>>   static void perf_iommu_read(struct perf_event *event)
>>   {
>> +	int i;
>>   	u64 delta = 0ULL;
>>   	struct hw_perf_event *hwc = &event->hw;
>> +	struct perf_amd_iommu *perf_iommu = container_of(event->pmu,
>> +							 struct perf_amd_iommu,
>> +							 pmu);
>>
>> +	if (amd_iommu_pc_get_counters(_GET_BANK(event), _GET_CNTR(event),
>> +				      amd_iommu_get_num_iommus(),
>> +				      perf_iommu_cnts))
>>   		return;
>>
>> +	/*
>> +	 * Now we re-aggregating event counts and prev-counts
>> +	 * from all IOMMUs
>> +	 */
>> +	local64_set(&hwc->prev_count, 0);
>> +
>> +	for (i = 0; i < amd_iommu_get_num_iommus(); i++) {
>> +		int indx = get_iommu_bnk_cnt_evt_idx(perf_iommu, i,
>> +						     _GET_BANK(event),
>> +						     _GET_CNTR(event));
>> +		u64 prev_raw_count = local64_read(&perf_iommu->prev_cnts[indx]);
>> +
>> +		/* IOMMU pc counter register is only 48 bits */
>> +		perf_iommu_cnts[i] &= GENMASK_ULL(48, 0);
>> +
>> +		/*
>> +		 * Since we do not enable counter overflow interrupts,
>> +		 * we do not have to worry about prev_count changing on us
>> +		 */
>> +		local64_set(&perf_iommu->prev_cnts[indx], perf_iommu_cnts[i]);
>> +		local64_add(prev_raw_count, &hwc->prev_count);
>> +
>> +		/* Handle 48-bit counter overflow */
>> +		delta = (perf_iommu_cnts[i] << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT);
>> +		delta >>= COUNTER_SHIFT;
>> +		local64_add(delta, &event->count);
>> +	}
>>   }
>
> So I really don't have time to review new muck while I'm hunting perf
> core fail, but Boris made me look at this.
>
> This is crazy, if you have multiple IOMMUs then create an event per
> IOMMU, do _NOT_ fold them all into a single event.

These are system-wide events, which are programmed on every IOMMU the 
same way. I am not sure what you meant by creating an event per IOMMU. 
Do you mean I should create internal per-IOMMU struct perf_event for 
each event? Could you please give me some pointers?

> In any case, the reason Boris asked me to look at this is that your
> overflow handling is broken, you want delta to be s64. Otherwise:
>
> 	delta >>= COUNTER_SHIFT;
>
> ends up as a SHR and you loose the MSB sign bits.

Ah. Sorry, I missed that.

Thanks,
Suravee

Re: [PATCH V4 5/6] perf/amd/iommu: Enable support for multiple IOMMUs

[Peter Zijlstra]

On Mon, Feb 22, 2016 at 03:00:31PM +0700, Suravee Suthikulpanit wrote:
> >So I really don't have time to review new muck while I'm hunting perf
> >core fail, but Boris made me look at this.
> >
> >This is crazy, if you have multiple IOMMUs then create an event per
> >IOMMU, do _NOT_ fold them all into a single event.
> 
> These are system-wide events, which are programmed on every IOMMU the same
> way. I am not sure what you meant by creating an event per IOMMU. Do you
> mean I should create internal per-IOMMU struct perf_event for each event?

No, I meant to expose each IOMMU individually to userspace, as a
separate device.

Is there never a case to profile just one of the IOMMUs ?

Re: [PATCH V4 5/6] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

Hi

On 02/22/2016 09:07 PM, Peter Zijlstra wrote:
> On Mon, Feb 22, 2016 at 03:00:31PM +0700, Suravee Suthikulpanit wrote:
>>> So I really don't have time to review new muck while I'm hunting perf
>>> core fail, but Boris made me look at this.
>>>
>>> This is crazy, if you have multiple IOMMUs then create an event per
>>> IOMMU, do _NOT_ fold them all into a single event.
>>
>> These are system-wide events, which are programmed on every IOMMU the same
>> way. I am not sure what you meant by creating an event per IOMMU. Do you
>> mean I should create internal per-IOMMU struct perf_event for each event?
>
> No, I meant to expose each IOMMU individually to userspace, as a
> separate device.
>
> Is there never a case to profile just one of the IOMMUs ?
>

I see. That's definitely doable and simpler to implement.

I was not sure if making users specify the IOMMU instance (e.g. 
amd_iommu_0/<ev name> , amd_iommu_1/<ev_name>, ....) would be too 
tedious. However, this would actually give users better control of the 
performance events, which is a good trade-off. I think it is acceptable.

I'll make the change and send this out in V5.

Thanks,
Suravee

Re: [PATCH V4 5/6] perf/amd/iommu: Enable support for multiple IOMMUs

[Alex Williamson]

On Tue, 23 Feb 2016 12:12:42 +0700
Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> wrote:

> Hi
> 
> On 02/22/2016 09:07 PM, Peter Zijlstra wrote:
> > On Mon, Feb 22, 2016 at 03:00:31PM +0700, Suravee Suthikulpanit wrote:  
> >>> So I really don't have time to review new muck while I'm hunting perf
> >>> core fail, but Boris made me look at this.
> >>>
> >>> This is crazy, if you have multiple IOMMUs then create an event per
> >>> IOMMU, do _NOT_ fold them all into a single event.  
> >>
> >> These are system-wide events, which are programmed on every IOMMU the same
> >> way. I am not sure what you meant by creating an event per IOMMU. Do you
> >> mean I should create internal per-IOMMU struct perf_event for each event?  
> >
> > No, I meant to expose each IOMMU individually to userspace, as a
> > separate device.
> >
> > Is there never a case to profile just one of the IOMMUs ?
> >  
> 
> I see. That's definitely doable and simpler to implement.
> 
> I was not sure if making users specify the IOMMU instance (e.g. 
> amd_iommu_0/<ev name> , amd_iommu_1/<ev_name>, ....) would be too 
> tedious. However, this would actually give users better control of the 
> performance events, which is a good trade-off. I think it is acceptable.
> 
> I'll make the change and send this out in V5.

We already expose individual IOMMU hardware units in /sys/class/iommu/,
you might consider trying to match the names there for the convenience
of the user.  Looks like we use ivhd%d for AMD.  Thanks,

Alex

Re: [PATCH V4 5/6] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

On 02/23/2016 12:24 PM, Alex Williamson wrote:
> On Tue, 23 Feb 2016 12:12:42 +0700
> Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> wrote:
>
>> Hi
>>
>> On 02/22/2016 09:07 PM, Peter Zijlstra wrote:
>>> On Mon, Feb 22, 2016 at 03:00:31PM +0700, Suravee Suthikulpanit wrote:
>>>>> So I really don't have time to review new muck while I'm hunting perf
>>>>> core fail, but Boris made me look at this.
>>>>>
>>>>> This is crazy, if you have multiple IOMMUs then create an event per
>>>>> IOMMU, do _NOT_ fold them all into a single event.
>>>>
>>>> These are system-wide events, which are programmed on every IOMMU the same
>>>> way. I am not sure what you meant by creating an event per IOMMU. Do you
>>>> mean I should create internal per-IOMMU struct perf_event for each event?
>>>
>>> No, I meant to expose each IOMMU individually to userspace, as a
>>> separate device.
>>>
>>> Is there never a case to profile just one of the IOMMUs ?
>>>
>>
>> I see. That's definitely doable and simpler to implement.
>>
>> I was not sure if making users specify the IOMMU instance (e.g.
>> amd_iommu_0/<ev name> , amd_iommu_1/<ev_name>, ....) would be too
>> tedious. However, this would actually give users better control of the
>> performance events, which is a good trade-off. I think it is acceptable.
>>
>> I'll make the change and send this out in V5.
>
> We already expose individual IOMMU hardware units in /sys/class/iommu/,
> you might consider trying to match the names there for the convenience
> of the user.  Looks like we use ivhd%d for AMD.  Thanks,
>
> Alex
>

Hm, the PMU for AMD IOMMU is currently shown as /sys/device/amd_iommu. I 
am not sure if /sys/device/ivhd[0|1|...] would be obvious to users that 
IVHD is really the AMD IOMMU.

Suravee

Re: [PATCH V4 5/6] perf/amd/iommu: Enable support for multiple IOMMUs

[Borislav Petkov]

On Thu, Feb 11, 2016 at 04:15:26PM +0700, Suravee Suthikulpanit wrote:
> The current amd_iommu_pc_get_set_reg_val() does not support muli-IOMMU

multi

> system. This patch replace amd_iommu_pc_get_set_reg_val() with

You don't need to say in the commit message what this patch does - I
think most of us can see it - but concentrate on the why. So if you catch
yourself starting a commit message with "This patch does... " then that
description is most of the time redundant and needless.

> amd_iommu_pc_set_reg_val() and amd_iommu_pc_[set|get]_cnt_vals().
> 
> Also, the current struct hw_perf_event.prev_count can only store the
> previous counter value only from one IOMMU. So, this patch introduces
> a new data structure, perf_amd_iommu.prev_cnts, to track previous value
> of each performance counter of each bank of each IOMMU.
> 
> Last, this patch introduce perf_iommu_cnts to temporary hold counter
> values from each IOMMU for internal use when manages counters among
> multiple IOMMUs.
> 
> Note that this implementation makes an assumption that the counters
> on all IOMMUs will be programmed the same way (i.e with the same events).

Why?

Is that some hw restriction or what's going on?

This needs to at least be documented why or even completely lifted. I'd
prefer the latter.

> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c           | 146 +++++++++++++++++++++++-----------
>  arch/x86/include/asm/perf/amd/iommu.h |   7 +-
>  drivers/iommu/amd_iommu_init.c        | 107 ++++++++++++++++++++++---
>  3 files changed, 201 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 812eff2..947ae8a 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -42,6 +42,13 @@ struct perf_amd_iommu {
>  	u64 cntr_assign_mask;
>  	raw_spinlock_t lock;
>  	const struct attribute_group *attr_groups[4];
> +
> +	/*
> +	 * This is a 3D array used to store the previous count values
> +	 * from each performance counter of each bank of each IOMMU.
> +	 * I.E. size of array = (num iommus * num banks * num counters)
> +	 */
> +	local64_t *prev_cnts;
>  };
>  
>  #define format_group	attr_groups[0]
> @@ -121,6 +128,12 @@ static struct amd_iommu_event_desc amd_iommu_v2_event_descs[] = {
>  	{ /* end: all zeroes */ },
>  };
>  
> +/*
> + * This is an array used to temporary hold the current values
> + * read from a particular perf counter from each IOMMU.
> + */
> +static u64 *perf_iommu_cnts;

Is this going to be accessed in parallel on different CPUs? If so, you
need synchronization for it.

> +
>  /*---------------------------------------------
>   * sysfs cpumask attributes
>   *---------------------------------------------*/
> @@ -255,44 +268,42 @@ static void perf_iommu_enable_event(struct perf_event *ev)
>  	u64 reg = 0ULL;
>  
>  	reg = csource;
> -	amd_iommu_pc_get_set_reg_val(devid,
> -			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_COUNTER_SRC_REG, &reg, true);
> +	amd_iommu_pc_set_reg(devid, _GET_BANK(ev), _GET_CNTR(ev),
> +			     IOMMU_PC_COUNTER_SRC_REG, &reg);
>  
> -	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);
> +	reg = devid | (_GET_DEVID_MASK(ev) << 32);
>  	if (reg)
> -		reg |= (1UL << 31);
> -	amd_iommu_pc_get_set_reg_val(devid,
> -			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_DEVID_MATCH_REG, &reg, true);
> +		reg |= BIT(31);
> +	amd_iommu_pc_set_reg(devid, _GET_BANK(ev), _GET_CNTR(ev),
> +			     IOMMU_PC_DEVID_MATCH_REG, &reg);
>  
> -	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
> +	reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
>  	if (reg)
> -		reg |= (1UL << 31);
> -	amd_iommu_pc_get_set_reg_val(devid,
> -			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_PASID_MATCH_REG, &reg, true);
> +		reg |= BIT(31);
> +	amd_iommu_pc_set_reg(devid, _GET_BANK(ev), _GET_CNTR(ev),
> +			     IOMMU_PC_PASID_MATCH_REG, &reg);
>  
> -	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
> +	reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
>  	if (reg)
> -		reg |= (1UL << 31);
> -	amd_iommu_pc_get_set_reg_val(devid,
> -			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_DOMID_MATCH_REG, &reg, true);
> +		reg |= BIT(31);
> +	amd_iommu_pc_set_reg(devid, _GET_BANK(ev), _GET_CNTR(ev),
> +			     IOMMU_PC_DOMID_MATCH_REG, &reg);
>  }
>  

This is a cleanup and belongs in a separate patch. Each patch should
contain one logical change: patch 1 cleans up things, patch2 cleans up
things even more, ... patch N adds functionality, patch N + 1 adds more
functionality ... and so on.

>  static void perf_iommu_disable_event(struct perf_event *event)
>  {
>  	u64 reg = 0ULL;
>  
> -	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
> -			_GET_BANK(event), _GET_CNTR(event),
> -			IOMMU_PC_COUNTER_SRC_REG, &reg, true);
> +	amd_iommu_pc_set_reg(_GET_DEVID(event), _GET_BANK(event),
> +			     _GET_CNTR(event), IOMMU_PC_COUNTER_SRC_REG, &reg);
>  }

Ditto.

>  static void perf_iommu_start(struct perf_event *event, int flags)
>  {
>  	struct hw_perf_event *hwc = &event->hw;
> +	struct perf_amd_iommu *perf_iommu = container_of(event->pmu,
> +							 struct perf_amd_iommu,
> +							 pmu);

Or simply:

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

I'd make that struct perf_amd_iommu name shorter too.

>  	pr_debug("perf: amd_iommu:perf_iommu_start\n");
>  	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> @@ -302,10 +313,19 @@ static void perf_iommu_start(struct perf_event *event, int flags)
>  	hwc->state = 0;
>  
>  	if (flags & PERF_EF_RELOAD) {

Flip check and save an indentation level:

	if (!(flags & PERF_EF_RELOAD))
		goto enable;

> -		u64 prev_raw_count =  local64_read(&hwc->prev_count);
> -		amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
> -				_GET_BANK(event), _GET_CNTR(event),
> -				IOMMU_PC_COUNTER_REG, &prev_raw_count, true);
> +		int i;
> +
> +		for (i = 0; i < amd_iommu_get_num_iommus(); i++) {
> +			int index = get_iommu_bnk_cnt_evt_idx(perf_iommu, i,
> +							      _GET_BANK(event),
> +							      _GET_CNTR(event));
> +
> +			perf_iommu_cnts[i] = local64_read(&perf_iommu->prev_cnts[index]);
> +		}
> +
> +		amd_iommu_pc_set_counters(_GET_BANK(event), _GET_CNTR(event),
> +					  amd_iommu_get_num_iommus(),
> +					  perf_iommu_cnts);
>  	}
>  
>  	perf_iommu_enable_event(event);
> @@ -315,29 +335,45 @@ static void perf_iommu_start(struct perf_event *event, int flags)
>  
>  static void perf_iommu_read(struct perf_event *event)
>  {
> -	u64 count = 0ULL;
> -	u64 prev_raw_count = 0ULL;
> +	int i;
>  	u64 delta = 0ULL;
>  	struct hw_perf_event *hwc = &event->hw;
> -	pr_debug("perf: amd_iommu:perf_iommu_read\n");
> -
> -	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
> -				_GET_BANK(event), _GET_CNTR(event),
> -				IOMMU_PC_COUNTER_REG, &count, false);
> -
> -	/* IOMMU pc counter register is only 48 bits */
> -	count &= 0xFFFFFFFFFFFFULL;
> +	struct perf_amd_iommu *perf_iommu = container_of(event->pmu,
> +							 struct perf_amd_iommu,
> +							 pmu);

See above.

>  
> -	prev_raw_count =  local64_read(&hwc->prev_count);
> -	if (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
> -					count) != prev_raw_count)
> +	if (amd_iommu_pc_get_counters(_GET_BANK(event), _GET_CNTR(event),
> +				      amd_iommu_get_num_iommus(),
> +				      perf_iommu_cnts))
>  		return;
>  
> -	/* Handling 48-bit counter overflowing */
> -	delta = (count << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT);
> -	delta >>= COUNTER_SHIFT;
> -	local64_add(delta, &event->count);
> -
> +	/*
> +	 * Now we re-aggregating event counts and prev-counts
> +	 * from all IOMMUs

Just say "Aggregate ..." and finish that sentence with a "."

> +	 */
> +	local64_set(&hwc->prev_count, 0);
> +
> +	for (i = 0; i < amd_iommu_get_num_iommus(); i++) {
> +		int indx = get_iommu_bnk_cnt_evt_idx(perf_iommu, i,

		"idx" is what most code calls it.

> +						     _GET_BANK(event),
> +						     _GET_CNTR(event));

I'd let it stick out and shorten that function's name:

		int idx = get_bank_evt_idx(perf_iommu, i, _GET_BANK(event), _GET_CNTR(event));


> +		u64 prev_raw_count = local64_read(&perf_iommu->prev_cnts[indx]);
> +
> +		/* IOMMU pc counter register is only 48 bits */
> +		perf_iommu_cnts[i] &= GENMASK_ULL(48, 0);
> +
> +		/*
> +		 * Since we do not enable counter overflow interrupts,
> +		 * we do not have to worry about prev_count changing on us

Comments should be normal english sentences ending with a "."

> +		 */
> +		local64_set(&perf_iommu->prev_cnts[indx], perf_iommu_cnts[i]);
> +		local64_add(prev_raw_count, &hwc->prev_count);
> +
> +		/* Handle 48-bit counter overflow */
> +		delta = (perf_iommu_cnts[i] << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT);
> +		delta >>= COUNTER_SHIFT;
> +		local64_add(delta, &event->count);
> +	}
>  }
>  
>  static void perf_iommu_stop(struct perf_event *event, int flags)
> @@ -427,10 +463,14 @@ 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;
> -	}
> +	kfree(__perf_iommu.events_group);
> +	__perf_iommu.events_group = NULL;
> +
> +	kfree(__perf_iommu.prev_cnts);
> +	__perf_iommu.prev_cnts = NULL;
> +
> +	kfree(perf_iommu_cnts);
> +	perf_iommu_cnts = NULL;
>  }
>  
>  static __init int _init_perf_amd_iommu(
> @@ -460,6 +500,18 @@ static __init int _init_perf_amd_iommu(
>  	perf_iommu->null_group = NULL;
>  	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
>  
> +	perf_iommu->prev_cnts = kzalloc(sizeof(*perf_iommu->prev_cnts) *
> +					amd_iommu_get_num_iommus() *
> +					perf_iommu->max_banks *
> +					perf_iommu->max_counters, GFP_KERNEL);
> +	if (!perf_iommu->prev_cnts)
> +		return -ENOMEM;
> +
> +	perf_iommu_cnts = kzalloc(sizeof(*perf_iommu_cnts) * amd_iommu_get_num_iommus(),
> +				  GFP_KERNEL);


Sometimes checkpatch is right:

WARNING: Prefer kcalloc over kzalloc with multiply
#253: FILE: arch/x86/events/amd/iommu.c:510:
+       perf_iommu_cnts = kzalloc(sizeof(*perf_iommu_cnts) * amd_iommu_get_num_iommus(),

You could integrate it into your patch creation workflow but not take it
too seriously.


> +	if (!perf_iommu_cnts)
> +		return -ENOMEM;
> +
>  	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
>  	if (ret) {
>  		pr_err("perf: amd_iommu: Failed to initialized.\n");
> diff --git a/arch/x86/include/asm/perf/amd/iommu.h b/arch/x86/include/asm/perf/amd/iommu.h
> index 40919eb..e0aa9be 100644
> --- a/arch/x86/include/asm/perf/amd/iommu.h
> +++ b/arch/x86/include/asm/perf/amd/iommu.h
> @@ -33,9 +33,10 @@ u8 amd_iommu_pc_get_max_banks(void);
>  
>  u8 amd_iommu_pc_get_max_counters(void);
>  
> -int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value);
> +int amd_iommu_pc_set_reg(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value);
>  
> -int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
> -				 u64 *value, bool is_write);
> +int amd_iommu_pc_set_counters(u8 bank, u8 cntr, int num, u64 *value);
> +
> +int amd_iommu_pc_get_counters(u8 bank, u8 cntr, int num, u64 *value);
>  
>  #endif /*_PERF_EVENT_AMD_IOMMU_H_*/
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index ffa057e..fd4f2b1 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1133,6 +1133,8 @@ static int __init init_iommu_all(struct acpi_table_header *table)
>  	return 0;
>  }
>  
> +static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
> +				u8 fxn, u64 *value, bool is_write);

Why the forward declaration? Just move the whole function.

>  static void init_iommu_perf_ctr(struct amd_iommu *iommu)
>  {
> @@ -1144,8 +1146,8 @@ static void init_iommu_perf_ctr(struct amd_iommu *iommu)
>  	amd_iommu_pc_present = true;
>  
>  	/* Check if the performance counters can be written to */
> -	if ((0 != amd_iommu_pc_get_set_reg_val(0, 0, 0, 0, &val, true)) ||
> -	    (0 != amd_iommu_pc_get_set_reg_val(0, 0, 0, 0, &val2, false)) ||
> +	if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) ||
> +	    (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) ||
>  	    (val != val2)) {
>  		pr_err("AMD-Vi: Unable to write to IOMMU perf counter.\n");
>  		amd_iommu_pc_present = false;

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCH V4 6/6] perf/amd/iommu: Clean up print messages pr_debug

[Suravee Suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

This patch declares pr_fmt for perf/amd_iommu, removes unnecessary
pr_debug, and clean up error messages.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 947ae8a..3e00925 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -11,6 +11,8 @@
  * published by the Free Software Foundation.
  */
 
+#define pr_fmt(fmt)	"perf/amd_iommu: " fmt
+
 #include <linux/perf_event.h>
 #include <linux/module.h>
 #include <linux/cpumask.h>
@@ -305,7 +307,6 @@ static void perf_iommu_start(struct perf_event *event, int flags)
 							 struct perf_amd_iommu,
 							 pmu);
 
-	pr_debug("perf: amd_iommu:perf_iommu_start\n");
 	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
 		return;
 
@@ -381,8 +382,6 @@ static void perf_iommu_stop(struct perf_event *event, int flags)
 	struct hw_perf_event *hwc = &event->hw;
 	u64 config;
 
-	pr_debug("perf: amd_iommu:perf_iommu_stop\n");
-
 	if (hwc->state & PERF_HES_UPTODATE)
 		return;
 
@@ -404,7 +403,6 @@ static int perf_iommu_add(struct perf_event *event, int flags)
 	struct perf_amd_iommu *perf_iommu =
 			container_of(event->pmu, struct perf_amd_iommu, pmu);
 
-	pr_debug("perf: amd_iommu:perf_iommu_add\n");
 	event->hw.state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
 
 	/* request an iommu bank/counter */
@@ -425,7 +423,6 @@ static void perf_iommu_del(struct perf_event *event, int flags)
 	struct perf_amd_iommu *perf_iommu =
 			container_of(event->pmu, struct perf_amd_iommu, pmu);
 
-	pr_debug("perf: amd_iommu:perf_iommu_del\n");
 	perf_iommu_stop(event, PERF_EF_UPDATE);
 
 	/* clear the assigned iommu bank/counter */
@@ -489,7 +486,7 @@ static __init int _init_perf_amd_iommu(
 
 	/* Init events attributes */
 	if (_init_events_attrs(perf_iommu) != 0)
-		pr_err("perf: amd_iommu: Only support raw events.\n");
+		pr_err("Only support raw events.\n");
 
 	perf_iommu->max_banks = amd_iommu_pc_get_max_banks();
 	perf_iommu->max_counters = amd_iommu_pc_get_max_counters();
@@ -514,10 +511,10 @@ static __init int _init_perf_amd_iommu(
 
 	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
 	if (ret) {
-		pr_err("perf: amd_iommu: Failed to initialized.\n");
+		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",
+		pr_info("Detected. (%d banks, %d counters/bank)\n",
 			amd_iommu_pc_get_max_banks(),
 			amd_iommu_pc_get_max_counters());
 	}
-- 
2.5.0

Re: [PATCH V4 6/6] perf/amd/iommu: Clean up print messages pr_debug

[Borislav Petkov]

On Thu, Feb 11, 2016 at 04:15:27PM +0700, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> This patch declares pr_fmt for perf/amd_iommu, removes unnecessary
> pr_debug, and clean up error messages.

For the next version, move all the cleanups first so that the later
patches are simpler and easier to review.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH V4 0/6] perf/amd/iommu: Enable multi-IOMMU support

[Suravee Suthikulpanit]

Hi All,

I am wondering if there are any other concerns for this patch series.

Thanks,
Suravee

On 2/11/16 16:15, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> This patch series modifies the existing perf_event_amd_iommu driver
> to support systems with multiple IOMMUs. It introduces new AMD IOMMU APIs,
> which are used by the AMD IOMMU Perf driver to access performance
> counters in multiple IOMMUs.
>
> In addition, this series should also fix current AMD IOMMU PMU driver
> initialization issue in some existing KV and CZ platform, where it fails
> to write to IOMMU perf counter as reported by Andreas Hartmann here
> (http://comments.gmane.org/gmane.linux.kernel.pci/49147).
>
> Git branch containing this patch series is available here:
>
>      https://github.com/ssuthiku/linux.git  perf-iommu-v4
>
> Changes from V3 (https://lkml.org/lkml/2016/2/9/845)
>    * Rebase the code to tip/master per Boris suggestion
>    * Most changes are in patch 5/6:
>      * Fix several spelling and styling issues per Boris review comment
>      * Remove unnecessary pr_debug in the perf amd iommu driver (per Boris)
>      * Rename several function to make it less confusing (per Boris)
>      * Properly handle errors when fails to set registers/counters
>        on multiple IOMMUs. (per Boris)
>
> Changes from V2 ( https://lkml.org/lkml/2016/1/1/141)
>    * Ported to 4.5.0-rc2
>    * Add reviewed by Joerg for patch 1 and 2
>    * Remove EXPORT_SYMBOL from patch 3 (per Joerg suggestion)
>    * Merge patch 4/6 and 6/6 from V2 into 5/5 in V3 and add
>      more description in the commit message and in code comment.
>    * Patch 5: modify the logic to update counts to get rid off
>      un-necessary local64_cmpxchg().
>
> Changes from V1 (https://lkml.org/lkml/2015/12/22/535):
>    * Update patch3 and 6 to use amd_iommus_present instead of introducing
>      amd_iommu_cnt static variable since they are the same thing
>
> Suravee Suthikulpanit (6):
>    perf/amd/iommu: Consolidate and move perf_event_amd_iommu header
>    perf/amd/iommu: Modify functions to query max banks and counters
>    iommu/amd: Introduce amd_iommu_get_num_iommus()
>    perf/amd/iommu: Introduce get_iommu_bnk_cnt_evt_idx
>    perf/amd/iommu: Enable support for multiple IOMMUs
>    perf/amd/iommu: Declar pr_fmt and remove unnecessary pr_debug
>
>   arch/x86/events/amd/iommu.c           | 197 ++++++++++++++++++++++------------
>   arch/x86/events/amd/iommu.h           |  40 -------
>   arch/x86/include/asm/perf/amd/iommu.h |  42 ++++++++
>   drivers/iommu/amd_iommu_init.c        | 136 +++++++++++++++++++----
>   drivers/iommu/amd_iommu_proto.h       |   7 --
>   5 files changed, 286 insertions(+), 136 deletions(-)
>   delete mode 100644 arch/x86/events/amd/iommu.h
>   create mode 100644 arch/x86/include/asm/perf/amd/iommu.h
>

Re: [PATCH V4 0/6] perf/amd/iommu: Enable multi-IOMMU support

[Joerg Roedel]

Hi Suravee,

On Thu, Feb 11, 2016 at 04:15:21PM +0700, Suthikulpanit, Suravee wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> This patch series modifies the existing perf_event_amd_iommu driver
> to support systems with multiple IOMMUs. It introduces new AMD IOMMU APIs,
> which are used by the AMD IOMMU Perf driver to access performance
> counters in multiple IOMMUs.
> 
> In addition, this series should also fix current AMD IOMMU PMU driver
> initialization issue in some existing KV and CZ platform, where it fails
> to write to IOMMU perf counter as reported by Andreas Hartmann here
> (http://comments.gmane.org/gmane.linux.kernel.pci/49147).

Since that problem also affects older kernels, we need a stand-alone
fix for this initialization issue. We can't backport new features to fix
bugs in old kernels.


	Joerg

Re: [PATCH V4 0/6] perf/amd/iommu: Enable multi-IOMMU support

[Suravee Suthikulpanit]

Hi,

On 02/23/2016 06:04 PM, Joerg Roedel wrote:
> Hi Suravee,
>
> On Thu, Feb 11, 2016 at 04:15:21PM +0700, Suthikulpanit, Suravee wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> This patch series modifies the existing perf_event_amd_iommu driver
>> to support systems with multiple IOMMUs. It introduces new AMD IOMMU APIs,
>> which are used by the AMD IOMMU Perf driver to access performance
>> counters in multiple IOMMUs.
>>
>> In addition, this series should also fix current AMD IOMMU PMU driver
>> initialization issue in some existing KV and CZ platform, where it fails
>> to write to IOMMU perf counter as reported by Andreas Hartmann here
>> (http://comments.gmane.org/gmane.linux.kernel.pci/49147).
>
> Since that problem also affects older kernels, we need a stand-alone
> fix for this initialization issue. We can't backport new features to fix
> bugs in old kernels.
>
>
> 	Joerg
>

OK, I am spliting V5 into two part. First would be just to fix the 
issue. Then the second part would be to add the multiple IOMMU support.

Suravee

Re: [PATCH V4 0/6] perf/amd/iommu: Enable multi-IOMMU support

[Suravee Suthikulpanit]

On 02/23/2016 06:27 PM, Suravee Suthikulpanit wrote:
> Hi,
>
> On 02/23/2016 06:04 PM, Joerg Roedel wrote:
>> Hi Suravee,
>>
>> On Thu, Feb 11, 2016 at 04:15:21PM +0700, Suthikulpanit, Suravee wrote:
>>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>
>>> This patch series modifies the existing perf_event_amd_iommu driver
>>> to support systems with multiple IOMMUs. It introduces new AMD IOMMU
>>> APIs,
>>> which are used by the AMD IOMMU Perf driver to access performance
>>> counters in multiple IOMMUs.
>>>
>>> In addition, this series should also fix current AMD IOMMU PMU driver
>>> initialization issue in some existing KV and CZ platform, where it fails
>>> to write to IOMMU perf counter as reported by Andreas Hartmann here
>>> (http://comments.gmane.org/gmane.linux.kernel.pci/49147).
>>
>> Since that problem also affects older kernels, we need a stand-alone
>> fix for this initialization issue. We can't backport new features to fix
>> bugs in old kernels.
>>
>>
>>     Joerg
>>
>
> OK, I am spliting V5 into two part. First would be just to fix the
> issue. Then the second part would be to add the multiple IOMMU support.
>
> Suravee

Actually, my V5 is rebased from tips which has moved several files. I 
think we might need to create a separate patch series to fix this issue 
in older kernels. Which branch should I rebase my changes for the older 
kernel?

Suravee

[PATCH] iommu/amd: Fix boot warning when device 00:00.0 is not iommu

[Joerg Roedel]

On Tue, Feb 23, 2016 at 06:39:04PM +0700, Suravee Suthikulpanit wrote:
> Actually, my V5 is rebased from tips which has moved several files.
> I think we might need to create a separate patch series to fix this
> issue in older kernels. Which branch should I rebase my changes for
> the older kernel?

The fix is iommu-code only. Based on your patches, I extracted this
smaller patch, which only fixes the issue. Does it look good to you? If
you are okay with it I am going to queue it asap into my iommu/fixes
branch and send it upstream.


	Joerg

>From b91309eedd77374fdecc379942c44f903e2dedff Mon Sep 17 00:00:00 2001
From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Date: Tue, 23 Feb 2016 13:03:30 +0100
Subject: [PATCH] iommu/amd: Fix boot warning when device 00:00.0 is not iommu
 covered

The setup code for the performance counters in the AMD IOMMU driver
tests whether the counters can be written. It tests to setup a counter
for device 00:00.0, which fails on systems where this particular device
is not covered by the IOMMU.

Fix this by not relying on device 00:00.0 but only on the IOMMU being
present.

Cc: stable@vger.kernel.org
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu_init.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 013bdff..d06a6d9 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -228,6 +228,10 @@ static int amd_iommu_enable_interrupts(void);
 static int __init iommu_go_to_state(enum iommu_init_state state);
 static void init_device_table_dma(void);
 
+static int iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
+				    u8 bank, u8 cntr, u8 fxn,
+				    u64 *value, bool is_write);
+
 static inline void update_last_devid(u16 devid)
 {
 	if (devid > amd_iommu_last_bdf)
@@ -1142,8 +1146,8 @@ static void init_iommu_perf_ctr(struct amd_iommu *iommu)
 	amd_iommu_pc_present = true;
 
 	/* Check if the performance counters can be written to */
-	if ((0 != amd_iommu_pc_get_set_reg_val(0, 0, 0, 0, &val, true)) ||
-	    (0 != amd_iommu_pc_get_set_reg_val(0, 0, 0, 0, &val2, false)) ||
+	if ((0 != iommu_pc_get_set_reg_val(iommu, 0, 0, 0, &val, true)) ||
+	    (0 != iommu_pc_get_set_reg_val(iommu, 0, 0, 0, &val2, false)) ||
 	    (val != val2)) {
 		pr_err("AMD-Vi: Unable to write to IOMMU perf counter.\n");
 		amd_iommu_pc_present = false;
@@ -2283,22 +2287,15 @@ u8 amd_iommu_pc_get_max_counters(u16 devid)
 }
 EXPORT_SYMBOL(amd_iommu_pc_get_max_counters);
 
-int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
+static int iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
+				    u8 bank, u8 cntr, u8 fxn,
 				    u64 *value, bool is_write)
 {
-	struct amd_iommu *iommu;
 	u32 offset;
 	u32 max_offset_lim;
 
-	/* Make sure the IOMMU PC resource is available */
-	if (!amd_iommu_pc_present)
-		return -ENODEV;
-
-	/* Locate the iommu associated with the device ID */
-	iommu = amd_iommu_rlookup_table[devid];
-
 	/* Check for valid iommu and pc register indexing */
-	if (WARN_ON((iommu == NULL) || (fxn > 0x28) || (fxn & 7)))
+	if (WARN_ON((fxn > 0x28) || (fxn & 7)))
 		return -ENODEV;
 
 	offset = (u32)(((0x40|bank) << 12) | (cntr << 8) | fxn);
@@ -2322,3 +2319,16 @@ int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
 	return 0;
 }
 EXPORT_SYMBOL(amd_iommu_pc_get_set_reg_val);
+
+int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
+				    u64 *value, bool is_write)
+{
+	struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+
+	/* Make sure the IOMMU PC resource is available */
+	if (!amd_iommu_pc_present || iommu == NULL)
+		return -ENODEV;
+
+	return iommu_pc_get_set_reg_val(iommu, bank, cntr, fxn,
+					value, is_write);
+}
-- 
1.8.4.5