[PATCH V3 0/5] perf/amd/iommu: Enable multi-IOMMU support

[PATCH V3 0/5] 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 will 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.

Note that this patch also fixes the issue where IOMMU driver 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-v3

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 (5):
  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

 arch/x86/kernel/cpu/perf_event_amd_iommu.c | 164 ++++++++++++++++++++---------
 arch/x86/kernel/cpu/perf_event_amd_iommu.h |  40 -------
 drivers/iommu/amd_iommu_init.c             | 116 ++++++++++++++++----
 drivers/iommu/amd_iommu_proto.h            |   7 --
 include/linux/perf/perf_event_amd_iommu.h  |  43 ++++++++
 5 files changed, 254 insertions(+), 116 deletions(-)
 delete mode 100644 arch/x86/kernel/cpu/perf_event_amd_iommu.h
 create mode 100644 include/linux/perf/perf_event_amd_iommu.h

-- 
2.5.0

[PATCH V3 0/5] perf/amd/iommu: Enable multi-IOMMU support

[Suravee Suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit-5C7GfCeVMHo@public.gmane.org>

This patch series modifies the existing perf_event_amd_iommu driver
to support systems with multiple IOMMUs. It introduces new AMD IOMMU APIs,
which will 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.

Note that this patch also fixes the issue where IOMMU driver 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-v3

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 (5):
  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

 arch/x86/kernel/cpu/perf_event_amd_iommu.c | 164 ++++++++++++++++++++---------
 arch/x86/kernel/cpu/perf_event_amd_iommu.h |  40 -------
 drivers/iommu/amd_iommu_init.c             | 116 ++++++++++++++++----
 drivers/iommu/amd_iommu_proto.h            |   7 --
 include/linux/perf/perf_event_amd_iommu.h  |  43 ++++++++
 5 files changed, 254 insertions(+), 116 deletions(-)
 delete mode 100644 arch/x86/kernel/cpu/perf_event_amd_iommu.h
 create mode 100644 include/linux/perf/perf_event_amd_iommu.h

-- 
2.5.0

[PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Suravee Suthikulpanit]

This patch consolidates "arch/x86/kernel/cpu/perf_event_amd_iommu.h" and
"drivers/iommu/amd_iommu_proto.h", which contain duplicate function
declarations, into "include/linux/perf/perf_event_amd_iommu.h"

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

diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
index 97242a9..d44525e 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
+++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
@@ -12,12 +12,12 @@
  */
 
 #include <linux/perf_event.h>
+#include <linux/perf/perf_event_amd_iommu.h>
 #include <linux/module.h>
 #include <linux/cpumask.h>
 #include <linux/slab.h>
 
 #include "perf_event.h"
-#include "perf_event_amd_iommu.h"
 
 #define COUNTER_SHIFT		16
 
diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.h b/arch/x86/kernel/cpu/perf_event_amd_iommu.h
deleted file mode 100644
index 845d173..0000000
--- a/arch/x86/kernel/cpu/perf_event_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/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 013bdff..b6d684c 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 <linux/perf/perf_event_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
diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
new file mode 100644
index 0000000..845d173
--- /dev/null
+++ b/include/linux/perf/perf_event_amd_iommu.h
@@ -0,0 +1,40 @@
+/*
+ * 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_*/
-- 
2.5.0

[PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Suravee Suthikulpanit]

This patch consolidates "arch/x86/kernel/cpu/perf_event_amd_iommu.h" and
"drivers/iommu/amd_iommu_proto.h", which contain duplicate function
declarations, into "include/linux/perf/perf_event_amd_iommu.h"

Reviewed-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
---
 arch/x86/kernel/cpu/perf_event_amd_iommu.c |  2 +-
 arch/x86/kernel/cpu/perf_event_amd_iommu.h | 40 ------------------------------
 drivers/iommu/amd_iommu_init.c             |  2 ++
 drivers/iommu/amd_iommu_proto.h            |  7 ------
 include/linux/perf/perf_event_amd_iommu.h  | 40 ++++++++++++++++++++++++++++++
 5 files changed, 43 insertions(+), 48 deletions(-)
 delete mode 100644 arch/x86/kernel/cpu/perf_event_amd_iommu.h
 create mode 100644 include/linux/perf/perf_event_amd_iommu.h

diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
index 97242a9..d44525e 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
+++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
@@ -12,12 +12,12 @@
  */
 
 #include <linux/perf_event.h>
+#include <linux/perf/perf_event_amd_iommu.h>
 #include <linux/module.h>
 #include <linux/cpumask.h>
 #include <linux/slab.h>
 
 #include "perf_event.h"
-#include "perf_event_amd_iommu.h"
 
 #define COUNTER_SHIFT		16
 
diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.h b/arch/x86/kernel/cpu/perf_event_amd_iommu.h
deleted file mode 100644
index 845d173..0000000
--- a/arch/x86/kernel/cpu/perf_event_amd_iommu.h
+++ /dev/null
@@ -1,40 +0,0 @@
-/*
- * Copyright (C) 2013 Advanced Micro Devices, Inc.
- *
- * Author: Steven Kinney <Steven.Kinney-5C7GfCeVMHo@public.gmane.org>
- * Author: Suravee Suthikulpanit <Suraveee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
- *
- * 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/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 013bdff..b6d684c 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 <linux/perf/perf_event_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
diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
new file mode 100644
index 0000000..845d173
--- /dev/null
+++ b/include/linux/perf/perf_event_amd_iommu.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2013 Advanced Micro Devices, Inc.
+ *
+ * Author: Steven Kinney <Steven.Kinney-5C7GfCeVMHo@public.gmane.org>
+ * Author: Suravee Suthikulpanit <Suraveee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
+ *
+ * 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_*/
-- 
2.5.0

[PATCH V3 2/5] 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/kernel/cpu/perf_event_amd_iommu.c | 17 +++++++----------
 drivers/iommu/amd_iommu_init.c             | 20 ++++++++++++--------
 include/linux/perf/perf_event_amd_iommu.h  |  7 ++-----
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
index d44525e..2d59e20 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
+++ b/arch/x86/kernel/cpu/perf_event_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/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index b6d684c..275c0f5 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;
 }
diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
index 845d173..815eabb 100644
--- a/include/linux/perf/perf_event_amd_iommu.h
+++ b/include/linux/perf/perf_event_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 */
 extern bool amd_iommu_pc_supported(void);
 
-extern u8 amd_iommu_pc_get_max_banks(u16 devid);
+extern u8 amd_iommu_pc_get_max_banks(void);
 
-extern u8 amd_iommu_pc_get_max_counters(u16 devid);
+extern u8 amd_iommu_pc_get_max_counters(void);
 
 extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr,
 			u8 fxn, u64 *value, bool is_write);
-- 
2.5.0

[PATCH V3 2/5] 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-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
---
 arch/x86/kernel/cpu/perf_event_amd_iommu.c | 17 +++++++----------
 drivers/iommu/amd_iommu_init.c             | 20 ++++++++++++--------
 include/linux/perf/perf_event_amd_iommu.h  |  7 ++-----
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
index d44525e..2d59e20 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
+++ b/arch/x86/kernel/cpu/perf_event_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/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index b6d684c..275c0f5 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;
 }
diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
index 845d173..815eabb 100644
--- a/include/linux/perf/perf_event_amd_iommu.h
+++ b/include/linux/perf/perf_event_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 */
 extern bool amd_iommu_pc_supported(void);
 
-extern u8 amd_iommu_pc_get_max_banks(u16 devid);
+extern u8 amd_iommu_pc_get_max_banks(void);
 
-extern u8 amd_iommu_pc_get_max_counters(u16 devid);
+extern u8 amd_iommu_pc_get_max_counters(void);
 
 extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr,
 			u8 fxn, u64 *value, bool is_write);
-- 
2.5.0

[PATCH V3 3/5] 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>
---
 drivers/iommu/amd_iommu_init.c            | 7 ++++++-
 include/linux/perf/perf_event_amd_iommu.h | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 275c0f5..531b2e1 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
diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
index 815eabb..cb820c2 100644
--- a/include/linux/perf/perf_event_amd_iommu.h
+++ b/include/linux/perf/perf_event_amd_iommu.h
@@ -25,6 +25,8 @@
 #define PC_MAX_SPEC_CNTRS			16
 
 /* amd_iommu_init.c external support functions */
+extern int amd_iommu_get_num_iommus(void);
+
 extern bool amd_iommu_pc_supported(void);
 
 extern u8 amd_iommu_pc_get_max_banks(void);
-- 
2.5.0

[PATCH V3 3/5] 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-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/amd_iommu_init.c            | 7 ++++++-
 include/linux/perf/perf_event_amd_iommu.h | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 275c0f5..531b2e1 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
diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
index 815eabb..cb820c2 100644
--- a/include/linux/perf/perf_event_amd_iommu.h
+++ b/include/linux/perf/perf_event_amd_iommu.h
@@ -25,6 +25,8 @@
 #define PC_MAX_SPEC_CNTRS			16
 
 /* amd_iommu_init.c external support functions */
+extern int amd_iommu_get_num_iommus(void);
+
 extern bool amd_iommu_pc_supported(void);
 
 extern u8 amd_iommu_pc_get_max_banks(void);
-- 
2.5.0

[PATCH V3 4/5] 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/kernel/cpu/perf_event_amd_iommu.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
index 2d59e20..791bbcf 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
+++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
@@ -145,18 +145,28 @@ 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

[PATCH V3 4/5] 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-5C7GfCeVMHo@public.gmane.org>
---
 arch/x86/kernel/cpu/perf_event_amd_iommu.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
index 2d59e20..791bbcf 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
+++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
@@ -145,18 +145,28 @@ 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

[PATCH V3 5/5] 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/kernel/cpu/perf_event_amd_iommu.c | 125 +++++++++++++++++++++--------
 drivers/iommu/amd_iommu_init.c             |  87 +++++++++++++++++---
 include/linux/perf/perf_event_amd_iommu.h  |   8 +-
 3 files changed, 174 insertions(+), 46 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
index 791bbcf..ce6ba3f 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
+++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
@@ -42,6 +42,12 @@ 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 +127,11 @@ 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
  *---------------------------------------------*/
@@ -256,44 +267,46 @@ static void perf_iommu_enable_event(struct perf_event *ev)
 	u64 reg = 0ULL;
 
 	reg = csource;
-	amd_iommu_pc_get_set_reg_val(devid,
+	amd_iommu_pc_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_COUNTER_SRC_REG, &reg, true);
+			 IOMMU_PC_COUNTER_SRC_REG, &reg);
 
 	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);
 	if (reg)
 		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
+	amd_iommu_pc_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DEVID_MATCH_REG, &reg, true);
+			 IOMMU_PC_DEVID_MATCH_REG, &reg);
 
 	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
 	if (reg)
 		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
+	amd_iommu_pc_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_PASID_MATCH_REG, &reg, true);
+			 IOMMU_PC_PASID_MATCH_REG, &reg);
 
 	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
 	if (reg)
 		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
+	amd_iommu_pc_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DOMID_MATCH_REG, &reg, true);
+			 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),
+	amd_iommu_pc_set_reg_val(_GET_DEVID(event),
 			_GET_BANK(event), _GET_CNTR(event),
-			IOMMU_PC_COUNTER_SRC_REG, &reg, true);
+			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)))
@@ -303,10 +316,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_cnt_vals(_GET_BANK(event), _GET_CNTR(event),
+					  amd_iommu_get_num_iommus(),
+					  perf_iommu_cnts);
 	}
 
 	perf_iommu_enable_event(event);
@@ -316,29 +338,47 @@ 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);
+	struct perf_amd_iommu *perf_iommu =
+			container_of(event->pmu, struct perf_amd_iommu, pmu);
 
-	/* IOMMU pc counter register is only 48 bits */
-	count &= 0xFFFFFFFFFFFFULL;
+	pr_debug("perf: amd_iommu:perf_iommu_read\n");
 
-	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_cnt_vals(_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] &= 0xFFFFFFFFFFFFULL;
+
+		/*
+		 * 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);
+
+		/* Handling 48-bit counter overflowing */
+		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)
@@ -428,10 +468,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(
@@ -461,6 +505,17 @@ 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/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 531b2e1..7b1b561 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1133,6 +1133,9 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	return 0;
 }
 
+static int _amd_iommu_pc_get_set_reg_val(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 +1147,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 ((_amd_iommu_pc_get_set_reg_val(iommu, 0, 0, 0, &val, true)) ||
+	    (_amd_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;
@@ -2294,10 +2297,10 @@ 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 _amd_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;
 
@@ -2305,9 +2308,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 +2332,73 @@ 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_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value)
+{
+	struct amd_iommu *iommu;
+
+	for_each_iommu(iommu) {
+		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
+							fxn, value, true);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(amd_iommu_pc_set_reg_val);
+
+int amd_iommu_pc_set_cnt_vals(u8 bank, u8 cntr, int num, u64 *value)
+{
+	struct amd_iommu *iommu;
+	int i = 0;
+
+	if (num > amd_iommus_present)
+		return -EINVAL;
+
+	for_each_iommu(iommu) {
+		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
+							IOMMU_PC_COUNTER_REG,
+							&value[i], true);
+		if (ret)
+			return ret;
+		if (i++ == amd_iommus_present)
+			break;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(amd_iommu_pc_set_cnt_vals);
+
+int amd_iommu_pc_get_cnt_vals(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 IOMMU,
+	 * 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 = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
+							IOMMU_PC_COUNTER_REG,
+							&tmp, false);
+		if (ret)
+			return ret;
+
+		/* IOMMU pc counter register is only 48 bits */
+		value[i] = tmp & 0xFFFFFFFFFFFFULL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(amd_iommu_pc_get_cnt_vals);
diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
index cb820c2..be1a17d 100644
--- a/include/linux/perf/perf_event_amd_iommu.h
+++ b/include/linux/perf/perf_event_amd_iommu.h
@@ -33,7 +33,11 @@ extern u8 amd_iommu_pc_get_max_banks(void);
 
 extern u8 amd_iommu_pc_get_max_counters(void);
 
-extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr,
-			u8 fxn, u64 *value, bool is_write);
+extern int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
+				    u64 *value);
+
+extern int amd_iommu_pc_set_cnt_vals(u8 bank, u8 cntr, int num, u64 *value);
+
+extern int amd_iommu_pc_get_cnt_vals(u8 bank, u8 cntr, int num, u64 *value);
 
 #endif /*_PERF_EVENT_AMD_IOMMU_H_*/
-- 
2.5.0

[PATCH V3 5/5] 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-5C7GfCeVMHo@public.gmane.org>
---
 arch/x86/kernel/cpu/perf_event_amd_iommu.c | 125 +++++++++++++++++++++--------
 drivers/iommu/amd_iommu_init.c             |  87 +++++++++++++++++---
 include/linux/perf/perf_event_amd_iommu.h  |   8 +-
 3 files changed, 174 insertions(+), 46 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
index 791bbcf..ce6ba3f 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
+++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
@@ -42,6 +42,12 @@ 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 +127,11 @@ 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
  *---------------------------------------------*/
@@ -256,44 +267,46 @@ static void perf_iommu_enable_event(struct perf_event *ev)
 	u64 reg = 0ULL;
 
 	reg = csource;
-	amd_iommu_pc_get_set_reg_val(devid,
+	amd_iommu_pc_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_COUNTER_SRC_REG, &reg, true);
+			 IOMMU_PC_COUNTER_SRC_REG, &reg);
 
 	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);
 	if (reg)
 		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
+	amd_iommu_pc_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DEVID_MATCH_REG, &reg, true);
+			 IOMMU_PC_DEVID_MATCH_REG, &reg);
 
 	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
 	if (reg)
 		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
+	amd_iommu_pc_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_PASID_MATCH_REG, &reg, true);
+			 IOMMU_PC_PASID_MATCH_REG, &reg);
 
 	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
 	if (reg)
 		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
+	amd_iommu_pc_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DOMID_MATCH_REG, &reg, true);
+			 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),
+	amd_iommu_pc_set_reg_val(_GET_DEVID(event),
 			_GET_BANK(event), _GET_CNTR(event),
-			IOMMU_PC_COUNTER_SRC_REG, &reg, true);
+			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)))
@@ -303,10 +316,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_cnt_vals(_GET_BANK(event), _GET_CNTR(event),
+					  amd_iommu_get_num_iommus(),
+					  perf_iommu_cnts);
 	}
 
 	perf_iommu_enable_event(event);
@@ -316,29 +338,47 @@ 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);
+	struct perf_amd_iommu *perf_iommu =
+			container_of(event->pmu, struct perf_amd_iommu, pmu);
 
-	/* IOMMU pc counter register is only 48 bits */
-	count &= 0xFFFFFFFFFFFFULL;
+	pr_debug("perf: amd_iommu:perf_iommu_read\n");
 
-	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_cnt_vals(_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] &= 0xFFFFFFFFFFFFULL;
+
+		/*
+		 * 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);
+
+		/* Handling 48-bit counter overflowing */
+		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)
@@ -428,10 +468,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(
@@ -461,6 +505,17 @@ 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/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 531b2e1..7b1b561 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1133,6 +1133,9 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	return 0;
 }
 
+static int _amd_iommu_pc_get_set_reg_val(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 +1147,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 ((_amd_iommu_pc_get_set_reg_val(iommu, 0, 0, 0, &val, true)) ||
+	    (_amd_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;
@@ -2294,10 +2297,10 @@ 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 _amd_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;
 
@@ -2305,9 +2308,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 +2332,73 @@ 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_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value)
+{
+	struct amd_iommu *iommu;
+
+	for_each_iommu(iommu) {
+		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
+							fxn, value, true);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(amd_iommu_pc_set_reg_val);
+
+int amd_iommu_pc_set_cnt_vals(u8 bank, u8 cntr, int num, u64 *value)
+{
+	struct amd_iommu *iommu;
+	int i = 0;
+
+	if (num > amd_iommus_present)
+		return -EINVAL;
+
+	for_each_iommu(iommu) {
+		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
+							IOMMU_PC_COUNTER_REG,
+							&value[i], true);
+		if (ret)
+			return ret;
+		if (i++ == amd_iommus_present)
+			break;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(amd_iommu_pc_set_cnt_vals);
+
+int amd_iommu_pc_get_cnt_vals(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 IOMMU,
+	 * 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 = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
+							IOMMU_PC_COUNTER_REG,
+							&tmp, false);
+		if (ret)
+			return ret;
+
+		/* IOMMU pc counter register is only 48 bits */
+		value[i] = tmp & 0xFFFFFFFFFFFFULL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(amd_iommu_pc_get_cnt_vals);
diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
index cb820c2..be1a17d 100644
--- a/include/linux/perf/perf_event_amd_iommu.h
+++ b/include/linux/perf/perf_event_amd_iommu.h
@@ -33,7 +33,11 @@ extern u8 amd_iommu_pc_get_max_banks(void);
 
 extern u8 amd_iommu_pc_get_max_counters(void);
 
-extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr,
-			u8 fxn, u64 *value, bool is_write);
+extern int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
+				    u64 *value);
+
+extern int amd_iommu_pc_set_cnt_vals(u8 bank, u8 cntr, int num, u64 *value);
+
+extern int amd_iommu_pc_get_cnt_vals(u8 bank, u8 cntr, int num, u64 *value);
 
 #endif /*_PERF_EVENT_AMD_IOMMU_H_*/
-- 
2.5.0

Re: [PATCH V3 0/5] perf/amd/iommu: Enable multi-IOMMU support

[Borislav Petkov]

On Tue, Feb 09, 2016 at 04:53:50PM -0600, 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 will 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.
> 
> Note that this patch also fixes the issue where IOMMU driver 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-v3
> 
> 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 (5):
>   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
> 
>  arch/x86/kernel/cpu/perf_event_amd_iommu.c | 164 ++++++++++++++++++++---------
>  arch/x86/kernel/cpu/perf_event_amd_iommu.h |  40 -------

You probably want to redo those against latest tip/master:

http://git.kernel.org/tip/5b26547dd7faa84e1293baa144a0f3e74ed7d4c7

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V3 0/5] perf/amd/iommu: Enable multi-IOMMU support

[Borislav Petkov]

On Tue, Feb 09, 2016 at 04:53:50PM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> 
> This patch series modifies the existing perf_event_amd_iommu driver
> to support systems with multiple IOMMUs. It introduces new AMD IOMMU APIs,
> which will 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.
> 
> Note that this patch also fixes the issue where IOMMU driver 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-v3
> 
> 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 (5):
>   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
> 
>  arch/x86/kernel/cpu/perf_event_amd_iommu.c | 164 ++++++++++++++++++++---------
>  arch/x86/kernel/cpu/perf_event_amd_iommu.h |  40 -------

You probably want to redo those against latest tip/master:

http://git.kernel.org/tip/5b26547dd7faa84e1293baa144a0f3e74ed7d4c7

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V3 0/5] perf/amd/iommu: Enable multi-IOMMU support

[Suravee Suthikulpanit]

Hi Boris,

On 02/09/2016 06:08 PM, Borislav Petkov wrote:
> You probably want to redo those against latest tip/master:
>
> http://git.kernel.org/tip/5b26547dd7faa84e1293baa144a0f3e74ed7d4c7
>
> -- Regards/Gruss, Boris.

Sure, I'll re-base this. By the way, do you have any other comments 
regarding the changes?

Thanks,
Suravee

Re: [PATCH V3 0/5] perf/amd/iommu: Enable multi-IOMMU support

[Suravee Suthikulpanit]

Hi Boris,

On 02/09/2016 06:08 PM, Borislav Petkov wrote:
> You probably want to redo those against latest tip/master:
>
> http://git.kernel.org/tip/5b26547dd7faa84e1293baa144a0f3e74ed7d4c7
>
> -- Regards/Gruss, Boris.

Sure, I'll re-base this. By the way, do you have any other comments 
regarding the changes?

Thanks,
Suravee

Re: [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Borislav Petkov]

On Tue, Feb 09, 2016 at 04:53:51PM -0600, Suravee Suthikulpanit wrote:
> This patch consolidates "arch/x86/kernel/cpu/perf_event_amd_iommu.h" and
> "drivers/iommu/amd_iommu_proto.h", which contain duplicate function
> declarations, into "include/linux/perf/perf_event_amd_iommu.h"
> 
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/kernel/cpu/perf_event_amd_iommu.c |  2 +-
>  arch/x86/kernel/cpu/perf_event_amd_iommu.h | 40 ------------------------------
>  drivers/iommu/amd_iommu_init.c             |  2 ++
>  drivers/iommu/amd_iommu_proto.h            |  7 ------
>  include/linux/perf/perf_event_amd_iommu.h  | 40 ++++++++++++++++++++++++++++++
>  5 files changed, 43 insertions(+), 48 deletions(-)
>  delete mode 100644 arch/x86/kernel/cpu/perf_event_amd_iommu.h
>  create mode 100644 include/linux/perf/perf_event_amd_iommu.h

Is this a header which will be used on something else besides x86 or why
is it being moved to include/linux/ ?

If not, it should go into arch/x86/events/ with the rest of the perf
private headers.

...

> diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
> new file mode 100644
> index 0000000..845d173
> --- /dev/null
> +++ b/include/linux/perf/perf_event_amd_iommu.h
> @@ -0,0 +1,40 @@
> +/*
> + * 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 */

s/maximun/maximum/

> +#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);

You can drop the "extern" qualifiers while at it - they're not needed.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V3 4/5] perf/amd/iommu: Introduce get_iommu_bnk_cnt_evt_idx

[Borislav Petkov]

On Tue, Feb 09, 2016 at 04:53:54PM -0600, 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/kernel/cpu/perf_event_amd_iommu.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> index 2d59e20..791bbcf 100644
> --- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> +++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> @@ -145,18 +145,28 @@ 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);

You don't need to break this line - let it stick out.


-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V3 4/5] perf/amd/iommu: Introduce get_iommu_bnk_cnt_evt_idx

[Borislav Petkov]

On Tue, Feb 09, 2016 at 04:53:54PM -0600, Suravee Suthikulpanit wrote:
> Introduce a helper function to calculate bit-index for assigning
> performance counter assignment.
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> ---
>  arch/x86/kernel/cpu/perf_event_amd_iommu.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> index 2d59e20..791bbcf 100644
> --- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> +++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> @@ -145,18 +145,28 @@ 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);

You don't need to break this line - let it stick out.


-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Tue, Feb 09, 2016 at 04:53:55PM -0600, Suravee Suthikulpanit wrote:
> 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/kernel/cpu/perf_event_amd_iommu.c | 125 +++++++++++++++++++++--------
>  drivers/iommu/amd_iommu_init.c             |  87 +++++++++++++++++---
>  include/linux/perf/perf_event_amd_iommu.h  |   8 +-
>  3 files changed, 174 insertions(+), 46 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> index 791bbcf..ce6ba3f 100644
> --- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> +++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> @@ -42,6 +42,12 @@ 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 +127,11 @@ 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
>   *---------------------------------------------*/
> @@ -256,44 +267,46 @@ static void perf_iommu_enable_event(struct perf_event *ev)
>  	u64 reg = 0ULL;
>  
>  	reg = csource;
> -	amd_iommu_pc_get_set_reg_val(devid,
> +	amd_iommu_pc_set_reg_val(devid,
>  			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_COUNTER_SRC_REG, &reg, true);
> +			 IOMMU_PC_COUNTER_SRC_REG, &reg);

Read-impairing indentation. It should be aligned at the opening brace:

	function_name(param1, param2,
		      param3, ...

Ditto for the calls below.

>  
>  	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);

0ULL?

Is this supposed to clear the old value from the previous read above?

What's wrong with

	reg = devid | (_GET_DEVID_MASK(ev) << 32);

?

Same for the rest.

>  	if (reg)
>  		reg |= (1UL << 31);

All those can be turned into

		reg |= BIT(31);

> -	amd_iommu_pc_get_set_reg_val(devid,
> +	amd_iommu_pc_set_reg_val(devid,
>  			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_DEVID_MATCH_REG, &reg, true);
> +			 IOMMU_PC_DEVID_MATCH_REG, &reg);
>  
>  	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
>  	if (reg)
>  		reg |= (1UL << 31);
> -	amd_iommu_pc_get_set_reg_val(devid,
> +	amd_iommu_pc_set_reg_val(devid,
>  			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_PASID_MATCH_REG, &reg, true);
> +			 IOMMU_PC_PASID_MATCH_REG, &reg);
>  
>  	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
>  	if (reg)
>  		reg |= (1UL << 31);
> -	amd_iommu_pc_get_set_reg_val(devid,
> +	amd_iommu_pc_set_reg_val(devid,
>  			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_DOMID_MATCH_REG, &reg, true);
> +			 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),
> +	amd_iommu_pc_set_reg_val(_GET_DEVID(event),
>  			_GET_BANK(event), _GET_CNTR(event),
> -			IOMMU_PC_COUNTER_SRC_REG, &reg, true);
> +			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);

There's no need to make it less readable and still fit within the 80
cols rule. Feel free to relax it a little.

>  	pr_debug("perf: amd_iommu:perf_iommu_start\n");
>  	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> @@ -303,10 +316,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));

No need for the line break.

> +
> +			perf_iommu_cnts[i] = local64_read(
> +						&perf_iommu->prev_cnts[index]);

Yuck. What's wrong with:

			perf_iommu_cnts[i] = local64_read(&perf_iommu->prev_cnts[index]);

?

It looks much better to me.

> +		}
> +
> +		amd_iommu_pc_set_cnt_vals(_GET_BANK(event), _GET_CNTR(event),
> +					  amd_iommu_get_num_iommus(),
> +					  perf_iommu_cnts);

This one looks good.

>  	}
>  
>  	perf_iommu_enable_event(event);
> @@ -316,29 +338,47 @@ 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);
> +	struct perf_amd_iommu *perf_iommu =
> +			container_of(event->pmu, struct perf_amd_iommu, pmu);

Indentation.

>  
> -	/* IOMMU pc counter register is only 48 bits */
> -	count &= 0xFFFFFFFFFFFFULL;
> +	pr_debug("perf: amd_iommu:perf_iommu_read\n");

What is that debug statement good for? Can it go?

>From looking at that file, it has a bunch more which are completely
useless and can be deleted. In a separate patch.

> -	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_cnt_vals(_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
> +	 */

Kernel comment style:

	/*
	 * Start of first sentence...
	 * Second sentence...
	 * ...
	 */

> +	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));

Indentation.

> +		u64 prev_raw_count = local64_read(&perf_iommu->prev_cnts[indx]);
> +
> +		/* IOMMU pc counter register is only 48 bits */
> +		perf_iommu_cnts[i] &= 0xFFFFFFFFFFFFULL;

				   &= 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]);

No need to break it.

> +
> +		local64_add(prev_raw_count, &hwc->prev_count);
> +
> +		/* Handling 48-bit counter overflowing */

		/* Handle 48-bit counter overflow: */

reads better.

> +		delta = (perf_iommu_cnts[i] << COUNTER_SHIFT) -
> +				(prev_raw_count << COUNTER_SHIFT);

No need for the linebreak.

> +		delta >>= COUNTER_SHIFT;
> +		local64_add(delta, &event->count);
> +	}
>  }
>  
>  static void perf_iommu_stop(struct perf_event *event, int flags)
> @@ -428,10 +468,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(
> @@ -461,6 +505,17 @@ 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);

Indentation.

	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);

looks more readable to me.

> +	if (!perf_iommu->prev_cnts)
> +		return -ENOMEM;
> +
> +	perf_iommu_cnts = kzalloc(sizeof(*perf_iommu_cnts) *
> +				  amd_iommu_get_num_iommus(), GFP_KERNEL);

No need for the linebreak.

> +	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");

You can define pr_fmt to "perf/amd_iommu: " at the beginning of this
file and then you don't need to supply that prefix each time you call
pr_*

And that sentence above it wrong. It should be:

	pr_err("Error initializing AMD IOMMU perf counters.\n");

or something like that.

> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 531b2e1..7b1b561 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1133,6 +1133,9 @@ static int __init init_iommu_all(struct acpi_table_header *table)
>  	return 0;
>  }
>  
> +static int _amd_iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
> +					 u8 bank, u8 cntr, u8 fxn,
> +					 u64 *value, bool is_write);

static int _amd_iommu_this_func_name_is_def_too_long

>  
>  static void init_iommu_perf_ctr(struct amd_iommu *iommu)
>  {
> @@ -1144,8 +1147,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 ((_amd_iommu_pc_get_set_reg_val(iommu, 0, 0, 0, &val, true)) ||
> +	    (_amd_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;
> @@ -2294,10 +2297,10 @@ 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 _amd_iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
> +					 u8 bank, u8 cntr, u8 fxn,
> +					 u64 *value, bool is_write)

Ditto.

>  {
> -	struct amd_iommu *iommu;
>  	u32 offset;
>  	u32 max_offset_lim;
>  
> @@ -2305,9 +2308,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 +2332,73 @@ 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_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value)
> +{
> +	struct amd_iommu *iommu;
> +
> +	for_each_iommu(iommu) {
> +		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
> +							fxn, value, true);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_set_reg_val);

Why isn't this EXPORT_SYMBOL_GPL?

> +
> +int amd_iommu_pc_set_cnt_vals(u8 bank, u8 cntr, int num, u64 *value)

That whole naming is hard to read. What's wrong with

amd_iommu_set_counters()

?

> +{
> +	struct amd_iommu *iommu;
> +	int i = 0;
> +
> +	if (num > amd_iommus_present)
> +		return -EINVAL;
> +
> +	for_each_iommu(iommu) {
> +		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
> +							IOMMU_PC_COUNTER_REG,
> +							&value[i], true);
> +		if (ret)
> +			return ret;
> +		if (i++ == amd_iommus_present)
> +			break;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_set_cnt_vals);

Ditto, why not EXPORT_SYMBOL_GPL ?

> +
> +int amd_iommu_pc_get_cnt_vals(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 IOMMU,

Plural is IOMMUs ?

> +	 * which should have been programmed the same way.
> +	 * and aggregate the counter values.

That comment is a sentence with a fullstop in the middle.

> +	 */
> +	for_each_iommu(iommu) {
> +		u64 tmp;
> +
> +		if (i >= num)
> +			return -EINVAL;
> +
> +		ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
> +							IOMMU_PC_COUNTER_REG,
> +							&tmp, false);
> +		if (ret)
> +			return ret;

So if one of those intermediary calls of _amd_iommu_pc_get_set_reg_val()
fails, we return here not having read all counters. Unwind and handle
that error properly maybe?

Same issue above.

> +
> +		/* IOMMU pc counter register is only 48 bits */
> +		value[i] = tmp & 0xFFFFFFFFFFFFULL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_get_cnt_vals);
> diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
> index cb820c2..be1a17d 100644
> --- a/include/linux/perf/perf_event_amd_iommu.h
> +++ b/include/linux/perf/perf_event_amd_iommu.h
> @@ -33,7 +33,11 @@ extern u8 amd_iommu_pc_get_max_banks(void);
>  
>  extern u8 amd_iommu_pc_get_max_counters(void);
>  
> -extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr,
> -			u8 fxn, u64 *value, bool is_write);
> +extern int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
> +				    u64 *value);
> +
> +extern int amd_iommu_pc_set_cnt_vals(u8 bank, u8 cntr, int num, u64 *value);
> +
> +extern int amd_iommu_pc_get_cnt_vals(u8 bank, u8 cntr, int num, u64 *value);

No need for that "extern"

>  #endif /*_PERF_EVENT_AMD_IOMMU_H_*/
> -- 
> 2.5.0
> 
> 

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Tue, Feb 09, 2016 at 04:53:55PM -0600, Suravee Suthikulpanit wrote:
> 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-5C7GfCeVMHo@public.gmane.org>
> ---
>  arch/x86/kernel/cpu/perf_event_amd_iommu.c | 125 +++++++++++++++++++++--------
>  drivers/iommu/amd_iommu_init.c             |  87 +++++++++++++++++---
>  include/linux/perf/perf_event_amd_iommu.h  |   8 +-
>  3 files changed, 174 insertions(+), 46 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event_amd_iommu.c b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> index 791bbcf..ce6ba3f 100644
> --- a/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> +++ b/arch/x86/kernel/cpu/perf_event_amd_iommu.c
> @@ -42,6 +42,12 @@ 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 +127,11 @@ 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
>   *---------------------------------------------*/
> @@ -256,44 +267,46 @@ static void perf_iommu_enable_event(struct perf_event *ev)
>  	u64 reg = 0ULL;
>  
>  	reg = csource;
> -	amd_iommu_pc_get_set_reg_val(devid,
> +	amd_iommu_pc_set_reg_val(devid,
>  			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_COUNTER_SRC_REG, &reg, true);
> +			 IOMMU_PC_COUNTER_SRC_REG, &reg);

Read-impairing indentation. It should be aligned at the opening brace:

	function_name(param1, param2,
		      param3, ...

Ditto for the calls below.

>  
>  	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);

0ULL?

Is this supposed to clear the old value from the previous read above?

What's wrong with

	reg = devid | (_GET_DEVID_MASK(ev) << 32);

?

Same for the rest.

>  	if (reg)
>  		reg |= (1UL << 31);

All those can be turned into

		reg |= BIT(31);

> -	amd_iommu_pc_get_set_reg_val(devid,
> +	amd_iommu_pc_set_reg_val(devid,
>  			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_DEVID_MATCH_REG, &reg, true);
> +			 IOMMU_PC_DEVID_MATCH_REG, &reg);
>  
>  	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
>  	if (reg)
>  		reg |= (1UL << 31);
> -	amd_iommu_pc_get_set_reg_val(devid,
> +	amd_iommu_pc_set_reg_val(devid,
>  			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_PASID_MATCH_REG, &reg, true);
> +			 IOMMU_PC_PASID_MATCH_REG, &reg);
>  
>  	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
>  	if (reg)
>  		reg |= (1UL << 31);
> -	amd_iommu_pc_get_set_reg_val(devid,
> +	amd_iommu_pc_set_reg_val(devid,
>  			_GET_BANK(ev), _GET_CNTR(ev) ,
> -			 IOMMU_PC_DOMID_MATCH_REG, &reg, true);
> +			 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),
> +	amd_iommu_pc_set_reg_val(_GET_DEVID(event),
>  			_GET_BANK(event), _GET_CNTR(event),
> -			IOMMU_PC_COUNTER_SRC_REG, &reg, true);
> +			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);

There's no need to make it less readable and still fit within the 80
cols rule. Feel free to relax it a little.

>  	pr_debug("perf: amd_iommu:perf_iommu_start\n");
>  	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> @@ -303,10 +316,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));

No need for the line break.

> +
> +			perf_iommu_cnts[i] = local64_read(
> +						&perf_iommu->prev_cnts[index]);

Yuck. What's wrong with:

			perf_iommu_cnts[i] = local64_read(&perf_iommu->prev_cnts[index]);

?

It looks much better to me.

> +		}
> +
> +		amd_iommu_pc_set_cnt_vals(_GET_BANK(event), _GET_CNTR(event),
> +					  amd_iommu_get_num_iommus(),
> +					  perf_iommu_cnts);

This one looks good.

>  	}
>  
>  	perf_iommu_enable_event(event);
> @@ -316,29 +338,47 @@ 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);
> +	struct perf_amd_iommu *perf_iommu =
> +			container_of(event->pmu, struct perf_amd_iommu, pmu);

Indentation.

>  
> -	/* IOMMU pc counter register is only 48 bits */
> -	count &= 0xFFFFFFFFFFFFULL;
> +	pr_debug("perf: amd_iommu:perf_iommu_read\n");

What is that debug statement good for? Can it go?

>From looking at that file, it has a bunch more which are completely
useless and can be deleted. In a separate patch.

> -	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_cnt_vals(_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
> +	 */

Kernel comment style:

	/*
	 * Start of first sentence...
	 * Second sentence...
	 * ...
	 */

> +	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));

Indentation.

> +		u64 prev_raw_count = local64_read(&perf_iommu->prev_cnts[indx]);
> +
> +		/* IOMMU pc counter register is only 48 bits */
> +		perf_iommu_cnts[i] &= 0xFFFFFFFFFFFFULL;

				   &= 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]);

No need to break it.

> +
> +		local64_add(prev_raw_count, &hwc->prev_count);
> +
> +		/* Handling 48-bit counter overflowing */

		/* Handle 48-bit counter overflow: */

reads better.

> +		delta = (perf_iommu_cnts[i] << COUNTER_SHIFT) -
> +				(prev_raw_count << COUNTER_SHIFT);

No need for the linebreak.

> +		delta >>= COUNTER_SHIFT;
> +		local64_add(delta, &event->count);
> +	}
>  }
>  
>  static void perf_iommu_stop(struct perf_event *event, int flags)
> @@ -428,10 +468,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(
> @@ -461,6 +505,17 @@ 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);

Indentation.

	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);

looks more readable to me.

> +	if (!perf_iommu->prev_cnts)
> +		return -ENOMEM;
> +
> +	perf_iommu_cnts = kzalloc(sizeof(*perf_iommu_cnts) *
> +				  amd_iommu_get_num_iommus(), GFP_KERNEL);

No need for the linebreak.

> +	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");

You can define pr_fmt to "perf/amd_iommu: " at the beginning of this
file and then you don't need to supply that prefix each time you call
pr_*

And that sentence above it wrong. It should be:

	pr_err("Error initializing AMD IOMMU perf counters.\n");

or something like that.

> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 531b2e1..7b1b561 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1133,6 +1133,9 @@ static int __init init_iommu_all(struct acpi_table_header *table)
>  	return 0;
>  }
>  
> +static int _amd_iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
> +					 u8 bank, u8 cntr, u8 fxn,
> +					 u64 *value, bool is_write);

static int _amd_iommu_this_func_name_is_def_too_long

>  
>  static void init_iommu_perf_ctr(struct amd_iommu *iommu)
>  {
> @@ -1144,8 +1147,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 ((_amd_iommu_pc_get_set_reg_val(iommu, 0, 0, 0, &val, true)) ||
> +	    (_amd_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;
> @@ -2294,10 +2297,10 @@ 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 _amd_iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
> +					 u8 bank, u8 cntr, u8 fxn,
> +					 u64 *value, bool is_write)

Ditto.

>  {
> -	struct amd_iommu *iommu;
>  	u32 offset;
>  	u32 max_offset_lim;
>  
> @@ -2305,9 +2308,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 +2332,73 @@ 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_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value)
> +{
> +	struct amd_iommu *iommu;
> +
> +	for_each_iommu(iommu) {
> +		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
> +							fxn, value, true);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_set_reg_val);

Why isn't this EXPORT_SYMBOL_GPL?

> +
> +int amd_iommu_pc_set_cnt_vals(u8 bank, u8 cntr, int num, u64 *value)

That whole naming is hard to read. What's wrong with

amd_iommu_set_counters()

?

> +{
> +	struct amd_iommu *iommu;
> +	int i = 0;
> +
> +	if (num > amd_iommus_present)
> +		return -EINVAL;
> +
> +	for_each_iommu(iommu) {
> +		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
> +							IOMMU_PC_COUNTER_REG,
> +							&value[i], true);
> +		if (ret)
> +			return ret;
> +		if (i++ == amd_iommus_present)
> +			break;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_set_cnt_vals);

Ditto, why not EXPORT_SYMBOL_GPL ?

> +
> +int amd_iommu_pc_get_cnt_vals(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 IOMMU,

Plural is IOMMUs ?

> +	 * which should have been programmed the same way.
> +	 * and aggregate the counter values.

That comment is a sentence with a fullstop in the middle.

> +	 */
> +	for_each_iommu(iommu) {
> +		u64 tmp;
> +
> +		if (i >= num)
> +			return -EINVAL;
> +
> +		ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
> +							IOMMU_PC_COUNTER_REG,
> +							&tmp, false);
> +		if (ret)
> +			return ret;

So if one of those intermediary calls of _amd_iommu_pc_get_set_reg_val()
fails, we return here not having read all counters. Unwind and handle
that error properly maybe?

Same issue above.

> +
> +		/* IOMMU pc counter register is only 48 bits */
> +		value[i] = tmp & 0xFFFFFFFFFFFFULL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_get_cnt_vals);
> diff --git a/include/linux/perf/perf_event_amd_iommu.h b/include/linux/perf/perf_event_amd_iommu.h
> index cb820c2..be1a17d 100644
> --- a/include/linux/perf/perf_event_amd_iommu.h
> +++ b/include/linux/perf/perf_event_amd_iommu.h
> @@ -33,7 +33,11 @@ extern u8 amd_iommu_pc_get_max_banks(void);
>  
>  extern u8 amd_iommu_pc_get_max_counters(void);
>  
> -extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr,
> -			u8 fxn, u64 *value, bool is_write);
> +extern int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
> +				    u64 *value);
> +
> +extern int amd_iommu_pc_set_cnt_vals(u8 bank, u8 cntr, int num, u64 *value);
> +
> +extern int amd_iommu_pc_get_cnt_vals(u8 bank, u8 cntr, int num, u64 *value);

No need for that "extern"

>  #endif /*_PERF_EVENT_AMD_IOMMU_H_*/
> -- 
> 2.5.0
> 
> 

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Suravee Suthikulpanit]

Hi Boris,

On 2/10/16 23:41, Borislav Petkov wrote:
> On Tue, Feb 09, 2016 at 04:53:51PM -0600, Suravee Suthikulpanit wrote:
>> >This patch consolidates "arch/x86/kernel/cpu/perf_event_amd_iommu.h" and
>> >"drivers/iommu/amd_iommu_proto.h", which contain duplicate function
>> >declarations, into "include/linux/perf/perf_event_amd_iommu.h"
>> >
>> >Reviewed-by: Joerg Roedel<jroedel@suse.de>
>> >Signed-off-by: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com>
>> >---
>> >  arch/x86/kernel/cpu/perf_event_amd_iommu.c |  2 +-
>> >  arch/x86/kernel/cpu/perf_event_amd_iommu.h | 40 ------------------------------
>> >  drivers/iommu/amd_iommu_init.c             |  2 ++
>> >  drivers/iommu/amd_iommu_proto.h            |  7 ------
>> >  include/linux/perf/perf_event_amd_iommu.h  | 40 ++++++++++++++++++++++++++++++
>> >  5 files changed, 43 insertions(+), 48 deletions(-)
>> >  delete mode 100644 arch/x86/kernel/cpu/perf_event_amd_iommu.h
>> >  create mode 100644 include/linux/perf/perf_event_amd_iommu.h
> Is this a header which will be used on something else besides x86 or why
> is it being moved to include/linux/ ?
>
> If not, it should go into arch/x86/events/ with the rest of the perf
> private headers.
>
> ...
>

My goal here is to find a place that I can declare a set of function 
prototypes and macros used in the IOMMU driver and IOMMU PERF. These are 
exported by the AMD IOMMU and can be used by other drivers (e.g. IOMMU 
perf).

The reason I picked this location to place the header file is because 
there is already an existing include/linux/perf/arm_pmu.h file there. 
So, I thought it might be alright to put the perf_event_amd_iommu.h here.

Having the information in the file arch/x86/events/amd/iommu.h seems 
strange for having to specify ../../arch/x86/events/amd/iommu.h in the 
IOMMU driver.

So, you think it would be alright if move

include/linux/perf/perf_event_amd_iommu.h
to
arch/x86/include/perf/perf_event_amd_iommu.h


Suravee

Thanks,
Suravee

Re: [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Suravee Suthikulpanit]

Hi Boris,

On 2/10/16 23:41, Borislav Petkov wrote:
> On Tue, Feb 09, 2016 at 04:53:51PM -0600, Suravee Suthikulpanit wrote:
>> >This patch consolidates "arch/x86/kernel/cpu/perf_event_amd_iommu.h" and
>> >"drivers/iommu/amd_iommu_proto.h", which contain duplicate function
>> >declarations, into "include/linux/perf/perf_event_amd_iommu.h"
>> >
>> >Reviewed-by: Joerg Roedel<jroedel-l3A5Bk7waGM@public.gmane.org>
>> >Signed-off-by: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>> >---
>> >  arch/x86/kernel/cpu/perf_event_amd_iommu.c |  2 +-
>> >  arch/x86/kernel/cpu/perf_event_amd_iommu.h | 40 ------------------------------
>> >  drivers/iommu/amd_iommu_init.c             |  2 ++
>> >  drivers/iommu/amd_iommu_proto.h            |  7 ------
>> >  include/linux/perf/perf_event_amd_iommu.h  | 40 ++++++++++++++++++++++++++++++
>> >  5 files changed, 43 insertions(+), 48 deletions(-)
>> >  delete mode 100644 arch/x86/kernel/cpu/perf_event_amd_iommu.h
>> >  create mode 100644 include/linux/perf/perf_event_amd_iommu.h
> Is this a header which will be used on something else besides x86 or why
> is it being moved to include/linux/ ?
>
> If not, it should go into arch/x86/events/ with the rest of the perf
> private headers.
>
> ...
>

My goal here is to find a place that I can declare a set of function 
prototypes and macros used in the IOMMU driver and IOMMU PERF. These are 
exported by the AMD IOMMU and can be used by other drivers (e.g. IOMMU 
perf).

The reason I picked this location to place the header file is because 
there is already an existing include/linux/perf/arm_pmu.h file there. 
So, I thought it might be alright to put the perf_event_amd_iommu.h here.

Having the information in the file arch/x86/events/amd/iommu.h seems 
strange for having to specify ../../arch/x86/events/amd/iommu.h in the 
IOMMU driver.

So, you think it would be alright if move

include/linux/perf/perf_event_amd_iommu.h
to
arch/x86/include/perf/perf_event_amd_iommu.h


Suravee

Thanks,
Suravee

Re: [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Borislav Petkov]

On Thu, Feb 11, 2016 at 01:42:52AM +0700, Suravee Suthikulpanit wrote:
> The reason I picked this location to place the header file is because
> there is already an existing include/linux/perf/arm_pmu.h file there.

I don't think anything arch-specific belongs in generic kernel header
paths.

> So, I thought it might be alright to put the perf_event_amd_iommu.h
> here.
>
> Having the information in the file arch/x86/events/amd/iommu.h seems
> strange for having to specify ../../arch/x86/events/amd/iommu.h in the
> IOMMU driver.
>
> So, you think it would be alright if move
> 
> include/linux/perf/perf_event_amd_iommu.h
> to
> arch/x86/include/perf/perf_event_amd_iommu.h

If it feels strange to you, you can move it to arch/x86/include/asm/
There we put the arch-specific stuff.

Then you can do

#include <asm/perf/amd/iommu.h>

or so. I've moved it to arch/x86/events/amd/iommu.h already anyway.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Borislav Petkov]

On Thu, Feb 11, 2016 at 01:42:52AM +0700, Suravee Suthikulpanit wrote:
> The reason I picked this location to place the header file is because
> there is already an existing include/linux/perf/arm_pmu.h file there.

I don't think anything arch-specific belongs in generic kernel header
paths.

> So, I thought it might be alright to put the perf_event_amd_iommu.h
> here.
>
> Having the information in the file arch/x86/events/amd/iommu.h seems
> strange for having to specify ../../arch/x86/events/amd/iommu.h in the
> IOMMU driver.
>
> So, you think it would be alright if move
> 
> include/linux/perf/perf_event_amd_iommu.h
> to
> arch/x86/include/perf/perf_event_amd_iommu.h

If it feels strange to you, you can move it to arch/x86/include/asm/
There we put the arch-specific stuff.

Then you can do

#include <asm/perf/amd/iommu.h>

or so. I've moved it to arch/x86/events/amd/iommu.h already anyway.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Suravee Suthikulpanit]

Hi,

On 2/11/16 01:51, Borislav Petkov wrote:
>> So, I thought it might be alright to put the perf_event_amd_iommu.h
>> >here.
>> >
>> >Having the information in the file arch/x86/events/amd/iommu.h seems
>> >strange for having to specify ../../arch/x86/events/amd/iommu.h in the
>> >IOMMU driver.
>> >
>> >So, you think it would be alright if move
>> >
>> >include/linux/perf/perf_event_amd_iommu.h
>> >to
>> >arch/x86/include/perf/perf_event_amd_iommu.h
> If it feels strange to you, you can move it to arch/x86/include/asm/
> There we put the arch-specific stuff.
>
> Then you can do
>
> #include <asm/perf/amd/iommu.h>
>
> or so. I've moved it to arch/x86/events/amd/iommu.h already anyway.

Ah.. agree then ;) So, I should branch off that tree of yours with the 
file already moved. Could you point me to it?

Thanks,
Suravee

Re: [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Suravee Suthikulpanit]

Hi,

On 2/11/16 01:51, Borislav Petkov wrote:
>> So, I thought it might be alright to put the perf_event_amd_iommu.h
>> >here.
>> >
>> >Having the information in the file arch/x86/events/amd/iommu.h seems
>> >strange for having to specify ../../arch/x86/events/amd/iommu.h in the
>> >IOMMU driver.
>> >
>> >So, you think it would be alright if move
>> >
>> >include/linux/perf/perf_event_amd_iommu.h
>> >to
>> >arch/x86/include/perf/perf_event_amd_iommu.h
> If it feels strange to you, you can move it to arch/x86/include/asm/
> There we put the arch-specific stuff.
>
> Then you can do
>
> #include <asm/perf/amd/iommu.h>
>
> or so. I've moved it to arch/x86/events/amd/iommu.h already anyway.

Ah.. agree then ;) So, I should branch off that tree of yours with the 
file already moved. Could you point me to it?

Thanks,
Suravee

Re: [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Borislav Petkov]

On Thu, Feb 11, 2016 at 01:56:43AM +0700, Suravee Suthikulpanit wrote:
> Ah.. agree then ;) So, I should branch off that tree of yours with the file
> already moved. Could you point me to it?

Not my tree, tip/master:

http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git, the master branch.

Just rediff your stuff ontop.

HTH.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Borislav Petkov]

On Thu, Feb 11, 2016 at 01:56:43AM +0700, Suravee Suthikulpanit wrote:
> Ah.. agree then ;) So, I should branch off that tree of yours with the file
> already moved. Could you point me to it?

Not my tree, tip/master:

http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git, the master branch.

Just rediff your stuff ontop.

HTH.

-- 
Regards/Gruss,
    Boris.

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

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

[Suravee Suthikulpanit]

Hi Boris,

Thank you very much for catching several styling issues. I should have 
taken care of that earlier.  I'll clean them up.  Please see my other 
comments below.

Thanks,
Suravee

On 02/11/2016 12:14 AM, Borislav Petkov wrote:
> On Tue, Feb 09, 2016 at 04:53:55PM -0600, Suravee Suthikulpanit wrote:
>> [...]
>>
>> -	/* IOMMU pc counter register is only 48 bits */
>> -	count &= 0xFFFFFFFFFFFFULL;
>> +	pr_debug("perf: amd_iommu:perf_iommu_read\n");
>
> What is that debug statement good for? Can it go?
>
>  From looking at that file, it has a bunch more which are completely
> useless and can be deleted. In a separate patch.

Sure. I'll add a new patch to remove all these pr_debug.

>> [...]
>> +	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");
>
> You can define pr_fmt to "perf/amd_iommu: " at the beginning of this
> file and then you don't need to supply that prefix each time you call
> pr_*

Good point. I'll take care of this.

> And that sentence above it wrong. It should be:
>
> 	pr_err("Error initializing AMD IOMMU perf counters.\n");
>
> or something like that.
>

OK.

>> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
>> index 531b2e1..7b1b561 100644
>> --- a/drivers/iommu/amd_iommu_init.c
>> +++ b/drivers/iommu/amd_iommu_init.c
>> @@ -1133,6 +1133,9 @@ static int __init init_iommu_all(struct acpi_table_header *table)
>>   	return 0;
>>   }
>>
>> +static int _amd_iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
>> +					 u8 bank, u8 cntr, u8 fxn,
>> +					 u64 *value, bool is_write);
>
> static int _amd_iommu_this_func_name_is_def_too_long
>

I'll rename all these functions to make it shorter and less confusing.

>> [...]
>> +
>> +int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value)
>> +{
>> +	struct amd_iommu *iommu;
>> +
>> +	for_each_iommu(iommu) {
>> +		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
>> +							fxn, value, true);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(amd_iommu_pc_set_reg_val);
>
> Why isn't this EXPORT_SYMBOL_GPL?

Actually, I am just trying to match other exported functions in the AMD 
IOMMU driver. May be Joerg can let us know why these functions are not 
EXPORT_SYMBOL_GPL to begin with.

>> +	 * which should have been programmed the same way.
>> +	 * and aggregate the counter values.
>
> That comment is a sentence with a fullstop in the middle.
>
>> +	 */
>> +	for_each_iommu(iommu) {
>> +		u64 tmp;
>> +
>> +		if (i >= num)
>> +			return -EINVAL;
>> +
>> +		ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
>> +							IOMMU_PC_COUNTER_REG,
>> +							&tmp, false);
>> +		if (ret)
>> +			return ret;
>
> So if one of those intermediary calls of _amd_iommu_pc_get_set_reg_val()
> fails, we return here not having read all counters. Unwind and handle
> that error properly maybe?
>
> Same issue above.

Good point. So, for writing, I'll make sure to unset the 
registers/counters when we encounter errors.

For reading counters, I think it is acceptable to say that users cannot 
rely on the return values in the array if the function returns non-zero 
value (success).

Thanks,
Suravee

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

[Suravee Suthikulpanit]

Hi Boris,

Thank you very much for catching several styling issues. I should have 
taken care of that earlier.  I'll clean them up.  Please see my other 
comments below.

Thanks,
Suravee

On 02/11/2016 12:14 AM, Borislav Petkov wrote:
> On Tue, Feb 09, 2016 at 04:53:55PM -0600, Suravee Suthikulpanit wrote:
>> [...]
>>
>> -	/* IOMMU pc counter register is only 48 bits */
>> -	count &= 0xFFFFFFFFFFFFULL;
>> +	pr_debug("perf: amd_iommu:perf_iommu_read\n");
>
> What is that debug statement good for? Can it go?
>
>  From looking at that file, it has a bunch more which are completely
> useless and can be deleted. In a separate patch.

Sure. I'll add a new patch to remove all these pr_debug.

>> [...]
>> +	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");
>
> You can define pr_fmt to "perf/amd_iommu: " at the beginning of this
> file and then you don't need to supply that prefix each time you call
> pr_*

Good point. I'll take care of this.

> And that sentence above it wrong. It should be:
>
> 	pr_err("Error initializing AMD IOMMU perf counters.\n");
>
> or something like that.
>

OK.

>> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
>> index 531b2e1..7b1b561 100644
>> --- a/drivers/iommu/amd_iommu_init.c
>> +++ b/drivers/iommu/amd_iommu_init.c
>> @@ -1133,6 +1133,9 @@ static int __init init_iommu_all(struct acpi_table_header *table)
>>   	return 0;
>>   }
>>
>> +static int _amd_iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
>> +					 u8 bank, u8 cntr, u8 fxn,
>> +					 u64 *value, bool is_write);
>
> static int _amd_iommu_this_func_name_is_def_too_long
>

I'll rename all these functions to make it shorter and less confusing.

>> [...]
>> +
>> +int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value)
>> +{
>> +	struct amd_iommu *iommu;
>> +
>> +	for_each_iommu(iommu) {
>> +		int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
>> +							fxn, value, true);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(amd_iommu_pc_set_reg_val);
>
> Why isn't this EXPORT_SYMBOL_GPL?

Actually, I am just trying to match other exported functions in the AMD 
IOMMU driver. May be Joerg can let us know why these functions are not 
EXPORT_SYMBOL_GPL to begin with.

>> +	 * which should have been programmed the same way.
>> +	 * and aggregate the counter values.
>
> That comment is a sentence with a fullstop in the middle.
>
>> +	 */
>> +	for_each_iommu(iommu) {
>> +		u64 tmp;
>> +
>> +		if (i >= num)
>> +			return -EINVAL;
>> +
>> +		ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
>> +							IOMMU_PC_COUNTER_REG,
>> +							&tmp, false);
>> +		if (ret)
>> +			return ret;
>
> So if one of those intermediary calls of _amd_iommu_pc_get_set_reg_val()
> fails, we return here not having read all counters. Unwind and handle
> that error properly maybe?
>
> Same issue above.

Good point. So, for writing, I'll make sure to unset the 
registers/counters when we encounter errors.

For reading counters, I think it is acceptable to say that users cannot 
rely on the return values in the array if the function returns non-zero 
value (success).

Thanks,
Suravee