[PATCH v8 0/9] perf/amd/iommu: Enable multi-IOMMU support

[PATCH v8 0/9] perf/amd/iommu: Enable multi-IOMMU support

[Suravee Suthikulpanit]

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

This patch series modifies the existing IOMMU and Perf drivers to support
systems with multiple IOMMUs by allocating an amd_iommu PMU per IOMMU instance.
This allows users to specify performance events and filters separately for each
IOMMU.

This has been tested on the new family17h-based server w/ multiple IOMMUs.

Git branch containing this patch series is available here:

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

Changes from V7 (https://lkml.org/lkml/2017/1/9/917)
  * Re-order patches to clean up first before introducing new stuff.
  * Always use amd_iommu_get_num_iommus() to access amd_iommus_present
    variable now.
  * Fix Perf IOMMU sysfs attributes initialization.
  * Miscellaneous clean up 

Changes from V6 (https://lkml.org/lkml/2016/12/23/134)
  * Renamed function parameters from devid to idx (per Joerg).
  * Removed unnecessary function declarations from amd_iommu_proto.h
    (per Joerg).

Changes from V5 (https://lkml.org/lkml/2016/2/23/370)
  * Rebased onto v4.9.
  * Remove the patch which consolidates function delclarations since
    we have not yet agreed on the appropriate place for the new header file.

Thanks,
Suravee

Suravee Suthikulpanit (9):
  perf/amd/iommu: Declare pr_fmt and remove unnecessary pr_debug
  perf/amd/iommu: Clean up perf_iommu_enable_event
  perf/amd/iommu: Misc fix up perf_iommu_read
  iommu/amd: Introduce amd_iommu_get_num_iommus()
  perf/amd/iommu: Modify functions to query max banks and counters
  perf/amd/iommu: Modify amd_iommu_pc_get_set_reg_val() API to allow
    specifying IOMMU index
  perf/amd/iommu: Check return value when set and get counter value
  perf/amd/iommu: Fix sysfs perf attribute groups
  perf/amd/iommu: Enable support for multiple IOMMUs

 arch/x86/events/amd/iommu.c     | 280 ++++++++++++++++++++--------------------
 arch/x86/events/amd/iommu.h     |  16 ++-
 drivers/iommu/amd_iommu.c       |   6 +-
 drivers/iommu/amd_iommu_init.c  | 100 +++++++++-----
 drivers/iommu/amd_iommu_proto.h |   8 +-
 drivers/iommu/amd_iommu_types.h |   3 -
 6 files changed, 217 insertions(+), 196 deletions(-)

-- 
1.8.3.1

[PATCH v8 1/9] perf/amd/iommu: Declare pr_fmt and remove unnecessary pr_debug

[Suravee Suthikulpanit]

Declare pr_fmt for perf/amd_iommu and remove unnecessary pr_debug.

Also check return value when _init_events_attrs fails.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index b28200d..44638d0 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -11,6 +11,8 @@
  * published by the Free Software Foundation.
  */
 
+#define pr_fmt(fmt)	"perf/amd_iommu: " fmt
+
 #include <linux/perf_event.h>
 #include <linux/init.h>
 #include <linux/cpumask.h>
@@ -298,7 +300,6 @@ static void perf_iommu_start(struct perf_event *event, int flags)
 {
 	struct hw_perf_event *hwc = &event->hw;
 
-	pr_debug("perf: amd_iommu:perf_iommu_start\n");
 	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
 		return;
 
@@ -323,7 +324,6 @@ static void perf_iommu_read(struct perf_event *event)
 	u64 prev_raw_count = 0ULL;
 	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),
@@ -349,8 +349,6 @@ static void perf_iommu_stop(struct perf_event *event, int flags)
 	struct hw_perf_event *hwc = &event->hw;
 	u64 config;
 
-	pr_debug("perf: amd_iommu:perf_iommu_stop\n");
-
 	if (hwc->state & PERF_HES_UPTODATE)
 		return;
 
@@ -372,7 +370,6 @@ static int perf_iommu_add(struct perf_event *event, int flags)
 	struct perf_amd_iommu *perf_iommu =
 			container_of(event->pmu, struct perf_amd_iommu, pmu);
 
-	pr_debug("perf: amd_iommu:perf_iommu_add\n");
 	event->hw.state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
 
 	/* request an iommu bank/counter */
@@ -393,7 +390,6 @@ static void perf_iommu_del(struct perf_event *event, int flags)
 	struct perf_amd_iommu *perf_iommu =
 			container_of(event->pmu, struct perf_amd_iommu, pmu);
 
-	pr_debug("perf: amd_iommu:perf_iommu_del\n");
 	perf_iommu_stop(event, PERF_EF_UPDATE);
 
 	/* clear the assigned iommu bank/counter */
@@ -444,24 +440,24 @@ static __init int _init_perf_amd_iommu(
 
 	raw_spin_lock_init(&perf_iommu->lock);
 
-	/* Init format attributes */
 	perf_iommu->format_group = &amd_iommu_format_group;
 
 	/* Init cpumask attributes to only core 0 */
 	cpumask_set_cpu(0, &iommu_cpumask);
 	perf_iommu->cpumask_group = &amd_iommu_cpumask_group;
 
-	/* Init events attributes */
-	if (_init_events_attrs(perf_iommu) != 0)
-		pr_err("perf: amd_iommu: Only support raw events.\n");
+	ret = _init_events_attrs(perf_iommu);
+	if (ret) {
+		pr_err("Error initializing AMD IOMMU perf events.\n");
+		return ret;
+	}
 
-	/* Init null attributes */
 	perf_iommu->null_group = NULL;
 	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
 
 	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
 	if (ret) {
-		pr_err("perf: amd_iommu: Failed to initialized.\n");
+		pr_err("Error initializing AMD IOMMU perf counters.\n");
 		amd_iommu_pc_exit();
 	} else {
 		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
-- 
1.8.3.1

[PATCH v8 2/9] perf/amd/iommu: Clean up perf_iommu_enable_event

[Suravee Suthikulpanit]

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

* Clean up various bitwise operations in perf_iommu_enable_event
* Make use macros BIT(x)

This should not affect logic and functionality.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 44638d0..1aa25d8 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -164,11 +164,11 @@ static int get_next_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu)
 	for (bank = 0, shift = 0; bank < max_banks; bank++) {
 		for (cntr = 0; cntr < max_cntrs; cntr++) {
 			shift = bank + (bank*3) + cntr;
-			if (perf_iommu->cntr_assign_mask & (1ULL<<shift)) {
+			if (perf_iommu->cntr_assign_mask & BIT(shift)) {
 				continue;
 			} else {
-				perf_iommu->cntr_assign_mask |= (1ULL<<shift);
-				retval = ((u16)((u16)bank<<8) | (u8)(cntr));
+				perf_iommu->cntr_assign_mask |= BIT(shift);
+				retval = ((u16)((u16)bank << 8) | (u8)(cntr));
 				goto out;
 			}
 		}
@@ -265,23 +265,23 @@ static void perf_iommu_enable_event(struct perf_event *ev)
 			_GET_BANK(ev), _GET_CNTR(ev) ,
 			 IOMMU_PC_COUNTER_SRC_REG, &reg, true);
 
-	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);
+	reg = devid | (_GET_DEVID_MASK(ev) << 32);
 	if (reg)
-		reg |= (1UL << 31);
+		reg |= BIT(31);
 	amd_iommu_pc_get_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
 			 IOMMU_PC_DEVID_MATCH_REG, &reg, true);
 
-	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
+	reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
 	if (reg)
-		reg |= (1UL << 31);
+		reg |= BIT(31);
 	amd_iommu_pc_get_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
 			 IOMMU_PC_PASID_MATCH_REG, &reg, true);
 
-	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
+	reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
 	if (reg)
-		reg |= (1UL << 31);
+		reg |= BIT(31);
 	amd_iommu_pc_get_set_reg_val(devid,
 			_GET_BANK(ev), _GET_CNTR(ev) ,
 			 IOMMU_PC_DOMID_MATCH_REG, &reg, true);
-- 
1.8.3.1

[PATCH v8 3/9] perf/amd/iommu: Misc fix up perf_iommu_read

[Suravee Suthikulpanit]

* Fix overflow handling since u64 delta would lose the MSB sign bit.
* Remove unnecessary local64_cmpxchg().
* Coding style and make use of GENMASK_ULL macro.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 1aa25d8..3f1c18a 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -320,9 +320,8 @@ 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;
-	u64 delta = 0ULL;
+	u64 count, prev;
+	s64 delta;
 	struct hw_perf_event *hwc = &event->hw;
 
 	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
@@ -330,18 +329,20 @@ static void perf_iommu_read(struct perf_event *event)
 				IOMMU_PC_COUNTER_REG, &count, false);
 
 	/* IOMMU pc counter register is only 48 bits */
-	count &= 0xFFFFFFFFFFFFULL;
+	count &= GENMASK_ULL(48, 0);
 
-	prev_raw_count =  local64_read(&hwc->prev_count);
-	if (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
-					count) != prev_raw_count)
-		return;
+	prev = local64_read(&hwc->prev_count);
 
-	/* Handling 48-bit counter overflowing */
-	delta = (count << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT);
+	/*
+	 * Since we do not enable counter overflow interrupts,
+	 * we do not have to worry about prev_count changing on us.
+	 */
+	local64_set(&hwc->prev_count, count);
+
+	/* Handle 48-bit counter overflow */
+	delta = (count << COUNTER_SHIFT) - (prev << COUNTER_SHIFT);
 	delta >>= COUNTER_SHIFT;
 	local64_add(delta, &event->count);
-
 }
 
 static void perf_iommu_stop(struct perf_event *event, int flags)
-- 
1.8.3.1

[PATCH v8 4/9] iommu/amd: Introduce amd_iommu_get_num_iommus()

[Suravee Suthikulpanit]

Introduce amd_iommu_get_num_iommus(), which returns the value of
amd_iommus_present, then replaces the direct access to the variable
which is now declared as static.

This function will also be used by Perf AMD IOMMU driver.

Cc: Borislav Petkov <bp@alien8.de>
Cc: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.h     |  2 ++
 drivers/iommu/amd_iommu.c       |  6 +++---
 drivers/iommu/amd_iommu_init.c  | 11 +++++++++--
 drivers/iommu/amd_iommu_proto.h |  1 +
 drivers/iommu/amd_iommu_types.h |  3 ---
 5 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/x86/events/amd/iommu.h b/arch/x86/events/amd/iommu.h
index 845d173..5c5c932 100644
--- a/arch/x86/events/amd/iommu.h
+++ b/arch/x86/events/amd/iommu.h
@@ -28,6 +28,8 @@
 #define IOMMU_BASE_DEVID			0x0000
 
 /* 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(u16 devid);
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 754595e..ae55485 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1228,7 +1228,7 @@ static void __domain_flush_pages(struct protection_domain *domain,
 
 	build_inv_iommu_pages(&cmd, address, size, domain->id, pde);
 
-	for (i = 0; i < amd_iommus_present; ++i) {
+	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
 		if (!domain->dev_iommu[i])
 			continue;
 
@@ -1272,7 +1272,7 @@ static void domain_flush_complete(struct protection_domain *domain)
 {
 	int i;
 
-	for (i = 0; i < amd_iommus_present; ++i) {
+	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
 		if (domain && !domain->dev_iommu[i])
 			continue;
 
@@ -3341,7 +3341,7 @@ static int __flush_pasid(struct protection_domain *domain, int pasid,
 	 * IOMMU TLB needs to be flushed before Device TLB to
 	 * prevent device TLB refill from IOMMU TLB
 	 */
-	for (i = 0; i < amd_iommus_present; ++i) {
+	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
 		if (domain->dev_iommu[i] == 0)
 			continue;
 
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 157e934..515d4c1 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -164,7 +164,9 @@ struct ivmd_header {
 
 /* Array to assign indices to IOMMUs*/
 struct amd_iommu *amd_iommus[MAX_IOMMUS];
-int amd_iommus_present;
+
+/* Number of IOMMUs present in the system */
+static int amd_iommus_present;
 
 /* IOMMUs have a non-present cache? */
 bool amd_iommu_np_cache __read_mostly;
@@ -269,6 +271,11 @@ static inline unsigned long tbl_size(int entry_size)
 	return 1UL << shift;
 }
 
+int amd_iommu_get_num_iommus(void)
+{
+	return amd_iommus_present;
+}
+
 /* Access to l1 and l2 indexed register spaces */
 
 static u32 iommu_read_l1(struct amd_iommu *iommu, u16 l1, u8 address)
@@ -1333,7 +1340,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");
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index 7eb60c1..e8f0710 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -21,6 +21,7 @@
 
 #include "amd_iommu_types.h"
 
+extern int amd_iommu_get_num_iommus(void);
 extern int amd_iommu_init_dma_ops(void);
 extern int amd_iommu_init_passthrough(void);
 extern irqreturn_t amd_iommu_int_thread(int irq, void *data);
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 0d91785..09d7a11 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -606,9 +606,6 @@ struct devid_map {
  */
 extern struct amd_iommu *amd_iommus[MAX_IOMMUS];
 
-/* Number of IOMMUs present in the system */
-extern int amd_iommus_present;
-
 /*
  * Declarations for the global list of all protection domains
  */
-- 
1.8.3.1

[PATCH v8 5/9] perf/amd/iommu: Modify functions to query max banks and counters

[Suravee Suthikulpanit]

Currently, amd_iommu_pc_get_max_[banks|counters]() use end-point
device ID to locate an IOMMU and check the reported max banks/counters.
The logic assumes that the IOMMU_BASE_DEVID belongs to the first IOMMU,
and uses it to acquire a reference to the first IOMMU, which does not work
on certain systems. Instead, we modify the function to take IOMMU index,
and use it to query the corresponded AMD IOMMU instance.

Note that we currently hard-code the IOMMU index to 0, since the current
AMD IOMMU perf implementation only supports single IOMMU. Subsequent patch
will add support for multi-IOMMU, and will use proper IOMMU index.

This patch also removes unnecessary function declaration in
amd_iommu_proto.h.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c     | 17 +++++++----------
 arch/x86/events/amd/iommu.h     |  7 ++-----
 drivers/iommu/amd_iommu_init.c  | 36 ++++++++++++++++++++++--------------
 drivers/iommu/amd_iommu_proto.h |  2 --
 4 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 3f1c18a..ec7e873 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -239,14 +239,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;
@@ -453,6 +445,11 @@ static __init int _init_perf_amd_iommu(
 		return ret;
 	}
 
+	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
+	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
+	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
+		return -EINVAL;
+
 	perf_iommu->null_group = NULL;
 	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
 
@@ -462,8 +459,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(0),
+			amd_iommu_pc_get_max_counters(0));
 	}
 
 	return ret;
diff --git a/arch/x86/events/amd/iommu.h b/arch/x86/events/amd/iommu.h
index 5c5c932..cf3dd05 100644
--- a/arch/x86/events/amd/iommu.h
+++ b/arch/x86/events/amd/iommu.h
@@ -24,17 +24,14 @@
 #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 int amd_iommu_get_num_iommus(void);
 
 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(unsigned int idx);
 
-extern u8 amd_iommu_pc_get_max_counters(u16 devid);
+extern u8 amd_iommu_pc_get_max_counters(unsigned int idx);
 
 extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr,
 			u8 fxn, u64 *value, bool is_write);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 515d4c1..ed21307d 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2713,6 +2713,20 @@ bool amd_iommu_v2_supported(void)
 }
 EXPORT_SYMBOL(amd_iommu_v2_supported);
 
+static struct amd_iommu *get_amd_iommu(unsigned int idx)
+{
+	unsigned int i = 0;
+	struct amd_iommu *iommu, *ret = NULL;
+
+	for_each_iommu(iommu) {
+		if (i++ == idx) {
+			ret = iommu;
+			break;
+		}
+	}
+	return ret;
+}
+
 /****************************************************************************
  *
  * IOMMU EFR Performance Counter support functionality. This code allows
@@ -2720,17 +2734,14 @@ bool amd_iommu_v2_supported(void)
  *
  ****************************************************************************/
 
-u8 amd_iommu_pc_get_max_banks(u16 devid)
+u8 amd_iommu_pc_get_max_banks(unsigned int idx)
 {
-	struct amd_iommu *iommu;
-	u8 ret = 0;
+	struct amd_iommu *iommu = get_amd_iommu(idx);
 
-	/* locate the iommu governing the devid */
-	iommu = amd_iommu_rlookup_table[devid];
 	if (iommu)
-		ret = iommu->max_banks;
+		return iommu->max_banks;
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL(amd_iommu_pc_get_max_banks);
 
@@ -2740,17 +2751,14 @@ 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(unsigned int idx)
 {
-	struct amd_iommu *iommu;
-	u8 ret = 0;
+	struct amd_iommu *iommu = get_amd_iommu(idx);
 
-	/* locate the iommu governing the devid */
-	iommu = amd_iommu_rlookup_table[devid];
 	if (iommu)
-		ret = iommu->max_counters;
+		return iommu->max_counters;
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL(amd_iommu_pc_get_max_counters);
 
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index e8f0710..cd2257e 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -59,8 +59,6 @@ extern int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, int pasid,
 
 /* 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);
 
-- 
1.8.3.1

[PATCH v8 6/9] perf/amd/iommu: Modify amd_iommu_pc_get_set_reg_val() API to allow specifying IOMMU index

[Suravee Suthikulpanit]

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

The current amd_iommu_pc_get_set_reg_val() cannot support multiple IOMMUs
It is also confusing since it is trying to support set and get in
one function.

So break it down to amd_iommu_pc_[get|set]_reg(),
and modifies them to allow callers to specify IOMMU index. This prepares
the driver for supporting multi-IOMMU in subsequent patch.

Also remove unnecessary function declarations in amd_iommu_proto.h.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c     | 34 ++++++++++----------------
 arch/x86/events/amd/iommu.h     |  7 ++++--
 drivers/iommu/amd_iommu_init.c  | 53 ++++++++++++++++++++++++++---------------
 drivers/iommu/amd_iommu_proto.h |  5 ----
 4 files changed, 52 insertions(+), 47 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index ec7e873..200d2e8 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -250,42 +250,36 @@ static void perf_iommu_enable_event(struct perf_event *ev)
 {
 	u8 csource = _GET_CSOURCE(ev);
 	u16 devid = _GET_DEVID(ev);
+	u8 bank = _GET_BANK(ev);
+	u8 cntr = _GET_CNTR(ev);
 	u64 reg = 0ULL;
 
 	reg = csource;
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_COUNTER_SRC_REG, &reg, true);
+	amd_iommu_pc_set_reg(0, bank, cntr,
+			     IOMMU_PC_COUNTER_SRC_REG, &reg);
 
 	reg = devid | (_GET_DEVID_MASK(ev) << 32);
 	if (reg)
 		reg |= BIT(31);
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DEVID_MATCH_REG, &reg, true);
+	amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_DEVID_MATCH_REG, &reg);
 
 	reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
 	if (reg)
 		reg |= BIT(31);
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_PASID_MATCH_REG, &reg, true);
+	amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_PASID_MATCH_REG, &reg);
 
 	reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
 	if (reg)
 		reg |= BIT(31);
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DOMID_MATCH_REG, &reg, true);
+	amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_DOMID_MATCH_REG, &reg);
 }
 
 static void perf_iommu_disable_event(struct perf_event *event)
 {
 	u64 reg = 0ULL;
 
-	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
-			_GET_BANK(event), _GET_CNTR(event),
-			IOMMU_PC_COUNTER_SRC_REG, &reg, true);
+	amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
+			     IOMMU_PC_COUNTER_SRC_REG, &reg);
 }
 
 static void perf_iommu_start(struct perf_event *event, int flags)
@@ -300,9 +294,8 @@ static void perf_iommu_start(struct perf_event *event, int flags)
 
 	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);
+		amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
+				     IOMMU_PC_COUNTER_REG, &prev_raw_count);
 	}
 
 	perf_iommu_enable_event(event);
@@ -316,9 +309,8 @@ static void perf_iommu_read(struct perf_event *event)
 	s64 delta;
 	struct hw_perf_event *hwc = &event->hw;
 
-	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
-				_GET_BANK(event), _GET_CNTR(event),
-				IOMMU_PC_COUNTER_REG, &count, false);
+	amd_iommu_pc_get_reg(0, _GET_BANK(event), _GET_CNTR(event),
+			     IOMMU_PC_COUNTER_REG, &count);
 
 	/* IOMMU pc counter register is only 48 bits */
 	count &= GENMASK_ULL(48, 0);
diff --git a/arch/x86/events/amd/iommu.h b/arch/x86/events/amd/iommu.h
index cf3dd05..cd70921 100644
--- a/arch/x86/events/amd/iommu.h
+++ b/arch/x86/events/amd/iommu.h
@@ -33,7 +33,10 @@
 
 extern u8 amd_iommu_pc_get_max_counters(unsigned int idx);
 
-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(unsigned int idx, u8 bank, u8 cntr,
+				u8 fxn, u64 *value);
+
+extern int amd_iommu_pc_get_reg(unsigned int idx, u8 bank, u8 cntr,
+				u8 fxn, u64 *value);
 
 #endif /*_PERF_EVENT_AMD_IOMMU_H_*/
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index ed21307d..5b7fb6c 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -253,10 +253,6 @@ enum iommu_init_state {
 static int __init iommu_go_to_state(enum iommu_init_state state);
 static void init_device_table_dma(void);
 
-static int iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
-				    u8 bank, u8 cntr, u8 fxn,
-				    u64 *value, bool is_write);
-
 static inline void update_last_devid(u16 devid)
 {
 	if (devid > amd_iommu_last_bdf)
@@ -1481,6 +1477,14 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	return 0;
 }
 
+#define iommu_pc_get_reg(i, b, c, f, v) \
+	iommu_pc_get_set_reg(i, b, c, f, v, false)
+
+#define iommu_pc_set_reg(i, b, c, f, v) \
+	iommu_pc_get_set_reg(i, b, c, f, v, true)
+
+static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
+				u8 fxn, u64 *value, bool is_write);
 
 static void init_iommu_perf_ctr(struct amd_iommu *iommu)
 {
@@ -1492,8 +1496,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 != iommu_pc_get_set_reg_val(iommu, 0, 0, 0, &val, true)) ||
-	    (0 != iommu_pc_get_set_reg_val(iommu, 0, 0, 0, &val2, false)) ||
+	if ((iommu_pc_set_reg(iommu, 0, 0, 0, &val)) ||
+	    (iommu_pc_get_reg(iommu, 0, 0, 0, &val2)) ||
 	    (val != val2)) {
 		pr_err("AMD-Vi: Unable to write to IOMMU perf counter.\n");
 		amd_iommu_pc_present = false;
@@ -2762,15 +2766,18 @@ u8 amd_iommu_pc_get_max_counters(unsigned int idx)
 }
 EXPORT_SYMBOL(amd_iommu_pc_get_max_counters);
 
-static int iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
-				    u8 bank, u8 cntr, u8 fxn,
-				    u64 *value, bool is_write)
+static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
+				u8 fxn, u64 *value, bool is_write)
 {
 	u32 offset;
 	u32 max_offset_lim;
 
+	/* Make sure the IOMMU PC resource is available */
+	if (!amd_iommu_pc_present)
+		return -ENODEV;
+
 	/* Check for valid iommu and pc register indexing */
-	if (WARN_ON((fxn > 0x28) || (fxn & 7)))
+	if (WARN_ON(!iommu || (fxn > 0x28) || (fxn & 7)))
 		return -ENODEV;
 
 	offset = (u32)(((0x40|bank) << 12) | (cntr << 8) | fxn);
@@ -2793,17 +2800,25 @@ static int iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
 
 	return 0;
 }
-EXPORT_SYMBOL(amd_iommu_pc_get_set_reg_val);
 
-int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
-				    u64 *value, bool is_write)
+int amd_iommu_pc_get_reg(unsigned int idx, u8 bank, u8 cntr, u8 fxn, u64 *value)
 {
-	struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+	struct amd_iommu *iommu = get_amd_iommu(idx);
 
-	/* Make sure the IOMMU PC resource is available */
-	if (!amd_iommu_pc_present || iommu == NULL)
-		return -ENODEV;
+	if (!iommu)
+		return -EINVAL;
+
+	return iommu_pc_get_reg(iommu, bank, cntr, fxn, value);
+}
+EXPORT_SYMBOL(amd_iommu_pc_get_reg);
+
+int amd_iommu_pc_set_reg(unsigned int idx, u8 bank, u8 cntr, u8 fxn, u64 *value)
+{
+	struct amd_iommu *iommu = get_amd_iommu(idx);
+
+	if (!iommu)
+		return -EINVAL;
 
-	return iommu_pc_get_set_reg_val(iommu, bank, cntr, fxn,
-					value, is_write);
+	return iommu_pc_set_reg(iommu, bank, cntr, fxn, value);
 }
+EXPORT_SYMBOL(amd_iommu_pc_set_reg);
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index cd2257e..466260f 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -57,11 +57,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 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
-- 
1.8.3.1

[PATCH v8 7/9] perf/amd/iommu: Check return value when set and get counter value

[Suravee Suthikulpanit]

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

In, perf_iommu_start(), we need to check the return value from
amd_iommu_set_reg(). In case of failure, we should not enable the PMU.

Also, in perf_iommu_read(), we need to check the return value from
amd_iommu_get_reg() before using the value.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 200d2e8..cc7bea4 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -284,6 +284,7 @@ static void perf_iommu_disable_event(struct perf_event *event)
 
 static void perf_iommu_start(struct perf_event *event, int flags)
 {
+	u64 val;
 	struct hw_perf_event *hwc = &event->hw;
 
 	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
@@ -292,15 +293,16 @@ static void perf_iommu_start(struct perf_event *event, int flags)
 	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
 	hwc->state = 0;
 
-	if (flags & PERF_EF_RELOAD) {
-		u64 prev_raw_count =  local64_read(&hwc->prev_count);
-		amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
-				     IOMMU_PC_COUNTER_REG, &prev_raw_count);
-	}
+	if (!(flags & PERF_EF_RELOAD))
+		return;
+
+	val = local64_read(&hwc->prev_count) & GENMASK_ULL(48, 0);
+	if (amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
+				   IOMMU_PC_COUNTER_REG, &val))
+		return;
 
 	perf_iommu_enable_event(event);
 	perf_event_update_userpage(event);
-
 }
 
 static void perf_iommu_read(struct perf_event *event)
@@ -309,8 +311,9 @@ static void perf_iommu_read(struct perf_event *event)
 	s64 delta;
 	struct hw_perf_event *hwc = &event->hw;
 
-	amd_iommu_pc_get_reg(0, _GET_BANK(event), _GET_CNTR(event),
-			     IOMMU_PC_COUNTER_REG, &count);
+	if (amd_iommu_pc_get_reg(0, _GET_BANK(event), _GET_CNTR(event),
+				 IOMMU_PC_COUNTER_REG, &count))
+		return;
 
 	/* IOMMU pc counter register is only 48 bits */
 	count &= GENMASK_ULL(48, 0);
-- 
1.8.3.1

[PATCH v8 8/9] perf/amd/iommu: Fix sysfs perf attribute groups

[Suravee Suthikulpanit]

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

Introduce static amd_iommu_attr_groups to simplify the
sysfs attributes initialization code.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c | 85 ++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 48 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index cc7bea4..223c01d 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -43,14 +43,8 @@ struct perf_amd_iommu {
 	u8 max_counters;
 	u64 cntr_assign_mask;
 	raw_spinlock_t lock;
-	const struct attribute_group *attr_groups[4];
 };
 
-#define format_group	attr_groups[0]
-#define cpumask_group	attr_groups[1]
-#define events_group	attr_groups[2]
-#define null_group	attr_groups[3]
-
 /*---------------------------------------------
  * sysfs format attributes
  *---------------------------------------------*/
@@ -81,6 +75,10 @@ struct perf_amd_iommu {
 /*---------------------------------------------
  * sysfs events attributes
  *---------------------------------------------*/
+static struct attribute_group amd_iommu_events_group = {
+	.name = "events",
+};
+
 struct amd_iommu_event_desc {
 	struct kobj_attribute attr;
 	const char *event;
@@ -388,76 +386,63 @@ static void perf_iommu_del(struct perf_event *event, int flags)
 	perf_event_update_userpage(event);
 }
 
-static __init int _init_events_attrs(struct perf_amd_iommu *perf_iommu)
+static __init int _init_events_attrs(void)
 {
-	struct attribute **attrs;
-	struct attribute_group *attr_group;
 	int i = 0, j;
+	struct attribute **attrs;
 
 	while (amd_iommu_v2_event_descs[i].attr.attr.name)
 		i++;
 
-	attr_group = kzalloc(sizeof(struct attribute *)
-		* (i + 1) + sizeof(*attr_group), GFP_KERNEL);
-	if (!attr_group)
+	attrs = kzalloc(sizeof(struct attribute **) * (i + 1), GFP_KERNEL);
+	if (!attrs)
 		return -ENOMEM;
 
-	attrs = (struct attribute **)(attr_group + 1);
 	for (j = 0; j < i; j++)
 		attrs[j] = &amd_iommu_v2_event_descs[j].attr.attr;
 
-	attr_group->name = "events";
-	attr_group->attrs = attrs;
-	perf_iommu->events_group = attr_group;
-
+	amd_iommu_events_group.attrs = attrs;
 	return 0;
 }
 
 static __init void amd_iommu_pc_exit(void)
 {
-	if (__perf_iommu.events_group != NULL) {
-		kfree(__perf_iommu.events_group);
-		__perf_iommu.events_group = NULL;
+	if (amd_iommu_events_group.attrs) {
+		kfree(amd_iommu_events_group.attrs);
+		amd_iommu_events_group.attrs = NULL;
 	}
 }
 
-static __init int _init_perf_amd_iommu(
-	struct perf_amd_iommu *perf_iommu, char *name)
+const struct attribute_group *amd_iommu_attr_groups[] = {
+	&amd_iommu_format_group,
+	&amd_iommu_cpumask_group,
+	&amd_iommu_events_group,
+	NULL,
+};
+
+static __init int
+_init_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, char *name)
 {
 	int ret;
 
 	raw_spin_lock_init(&perf_iommu->lock);
 
-	perf_iommu->format_group = &amd_iommu_format_group;
-
 	/* Init cpumask attributes to only core 0 */
 	cpumask_set_cpu(0, &iommu_cpumask);
-	perf_iommu->cpumask_group = &amd_iommu_cpumask_group;
-
-	ret = _init_events_attrs(perf_iommu);
-	if (ret) {
-		pr_err("Error initializing AMD IOMMU perf events.\n");
-		return ret;
-	}
 
 	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
 	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
 	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
 		return -EINVAL;
 
-	perf_iommu->null_group = NULL;
-	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
-
+	perf_iommu->pmu.attr_groups = amd_iommu_attr_groups;
 	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
-	if (ret) {
+	if (ret)
 		pr_err("Error initializing AMD IOMMU perf counters.\n");
-		amd_iommu_pc_exit();
-	} else {
+	else
 		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
 			amd_iommu_pc_get_max_banks(0),
 			amd_iommu_pc_get_max_counters(0));
-	}
-
 	return ret;
 }
 
@@ -471,24 +456,28 @@ static __init int _init_perf_amd_iommu(
 		.stop		= perf_iommu_stop,
 		.read		= perf_iommu_read,
 	},
-	.max_banks		= 0x00,
-	.max_counters		= 0x00,
-	.cntr_assign_mask	= 0ULL,
-	.format_group		= NULL,
-	.cpumask_group		= NULL,
-	.events_group		= NULL,
-	.null_group		= NULL,
 };
 
 static __init int amd_iommu_pc_init(void)
 {
+	int ret;
+
 	/* Make sure the IOMMU PC resource is available */
 	if (!amd_iommu_pc_supported())
 		return -ENODEV;
 
-	_init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
+	ret = _init_events_attrs();
+	if (ret)
+		goto err_out;
 
-	return 0;
+	ret = _init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
+	if (ret)
+		goto err_out;
+
+	return ret;
+err_out:
+	amd_iommu_pc_exit();
+	return ret;
 }
 
 device_initcall(amd_iommu_pc_init);
-- 
1.8.3.1

[PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

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

Add multi-IOMMU support for perf by exposing an AMD IOMMU PMU
for each IOMMU found in the system via:

  /bus/event_source/devices/amd_iommu_x

where x is the IOMMU index. This allows users to specify
different events to be programed onto performance counters
of each IOMMU.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/events/amd/iommu.c | 114 ++++++++++++++++++++++++++------------------
 1 file changed, 67 insertions(+), 47 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 223c01d..38eafbf 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -35,16 +35,21 @@
 #define _GET_PASID_MASK(ev) ((ev->hw.extra_reg.config >> 16) & 0xFFFFULL)
 #define _GET_DOMID_MASK(ev) ((ev->hw.extra_reg.config >> 32) & 0xFFFFULL)
 
-static struct perf_amd_iommu __perf_iommu;
+#define PERF_AMD_IOMMU_NAME_SZ 16
 
 struct perf_amd_iommu {
+	struct list_head list;
 	struct pmu pmu;
+	unsigned int idx;
+	char name[PERF_AMD_IOMMU_NAME_SZ];
 	u8 max_banks;
 	u8 max_counters;
 	u64 cntr_assign_mask;
 	raw_spinlock_t lock;
 };
 
+static LIST_HEAD(perf_amd_iommu_list);
+
 /*---------------------------------------------
  * sysfs format attributes
  *---------------------------------------------*/
@@ -202,8 +207,7 @@ static int clear_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu,
 static int perf_iommu_event_init(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
-	struct perf_amd_iommu *perf_iommu;
-	u64 config, config1;
+	struct perf_amd_iommu *pi;
 
 	/* test the event attr type check for PMU enumeration */
 	if (event->attr.type != event->pmu->type)
@@ -225,27 +229,18 @@ static int perf_iommu_event_init(struct perf_event *event)
 	if (event->cpu < 0)
 		return -EINVAL;
 
-	perf_iommu = &__perf_iommu;
-
-	if (event->pmu != &perf_iommu->pmu)
-		return -ENOENT;
-
-	if (perf_iommu) {
-		config = event->attr.config;
-		config1 = event->attr.config1;
-	} else {
-		return -EINVAL;
-	}
-
 	/* update the hw_perf_event struct with the iommu config data */
-	hwc->config = config;
-	hwc->extra_reg.config = config1;
+	pi = container_of(event->pmu, struct perf_amd_iommu, pmu);
+	hwc->idx              = pi->idx;
+	hwc->config           = event->attr.config;
+	hwc->extra_reg.config = event->attr.config1;
 
 	return 0;
 }
 
 static void perf_iommu_enable_event(struct perf_event *ev)
 {
+	struct hw_perf_event *hwc = &ev->hw;
 	u8 csource = _GET_CSOURCE(ev);
 	u16 devid = _GET_DEVID(ev);
 	u8 bank = _GET_BANK(ev);
@@ -253,30 +248,34 @@ static void perf_iommu_enable_event(struct perf_event *ev)
 	u64 reg = 0ULL;
 
 	reg = csource;
-	amd_iommu_pc_set_reg(0, bank, cntr,
+	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
 			     IOMMU_PC_COUNTER_SRC_REG, &reg);
 
 	reg = devid | (_GET_DEVID_MASK(ev) << 32);
 	if (reg)
 		reg |= BIT(31);
-	amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_DEVID_MATCH_REG, &reg);
+	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
+			     IOMMU_PC_DEVID_MATCH_REG, &reg);
 
 	reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
 	if (reg)
 		reg |= BIT(31);
-	amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_PASID_MATCH_REG, &reg);
+	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
+			     IOMMU_PC_PASID_MATCH_REG, &reg);
 
 	reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
 	if (reg)
 		reg |= BIT(31);
-	amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_DOMID_MATCH_REG, &reg);
+	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
+			     IOMMU_PC_DOMID_MATCH_REG, &reg);
 }
 
 static void perf_iommu_disable_event(struct perf_event *event)
 {
+	struct hw_perf_event *hwc = &event->hw;
 	u64 reg = 0ULL;
 
-	amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
+	amd_iommu_pc_set_reg(hwc->idx, _GET_BANK(event), _GET_CNTR(event),
 			     IOMMU_PC_COUNTER_SRC_REG, &reg);
 }
 
@@ -295,7 +294,7 @@ static void perf_iommu_start(struct perf_event *event, int flags)
 		return;
 
 	val = local64_read(&hwc->prev_count) & GENMASK_ULL(48, 0);
-	if (amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
+	if (amd_iommu_pc_set_reg(hwc->idx, _GET_BANK(event), _GET_CNTR(event),
 				   IOMMU_PC_COUNTER_REG, &val))
 		return;
 
@@ -309,7 +308,7 @@ static void perf_iommu_read(struct perf_event *event)
 	s64 delta;
 	struct hw_perf_event *hwc = &event->hw;
 
-	if (amd_iommu_pc_get_reg(0, _GET_BANK(event), _GET_CNTR(event),
+	if (amd_iommu_pc_get_reg(hwc->idx, _GET_BANK(event), _GET_CNTR(event),
 				 IOMMU_PC_COUNTER_REG, &count))
 		return;
 
@@ -407,6 +406,13 @@ static __init int _init_events_attrs(void)
 
 static __init void amd_iommu_pc_exit(void)
 {
+	struct perf_amd_iommu *pi, *next;
+
+	list_for_each_entry_safe(pi, next, &perf_amd_iommu_list, list) {
+		list_del(&pi->list);
+		kfree(pi);
+	}
+
 	if (amd_iommu_events_group.attrs) {
 		kfree(amd_iommu_events_group.attrs);
 		amd_iommu_events_group.attrs = NULL;
@@ -421,46 +427,46 @@ static __init void amd_iommu_pc_exit(void)
 };
 
 static __init int
-_init_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, char *name)
+init_one_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, unsigned int idx)
 {
 	int ret;
 
 	raw_spin_lock_init(&perf_iommu->lock);
 
-	/* Init cpumask attributes to only core 0 */
-	cpumask_set_cpu(0, &iommu_cpumask);
-
-	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
-	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
+	perf_iommu->idx          = idx;
+	perf_iommu->max_banks    = amd_iommu_pc_get_max_banks(idx);
+	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(idx);
 	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
 		return -EINVAL;
 
+	snprintf(perf_iommu->name, PERF_AMD_IOMMU_NAME_SZ, "amd_iommu_%u", idx);
+
+	perf_iommu->pmu.event_init  = perf_iommu_event_init,
+	perf_iommu->pmu.add         = perf_iommu_add,
+	perf_iommu->pmu.del         = perf_iommu_del,
+	perf_iommu->pmu.start       = perf_iommu_start,
+	perf_iommu->pmu.stop        = perf_iommu_stop,
+	perf_iommu->pmu.read        = perf_iommu_read,
+	perf_iommu->pmu.task_ctx_nr = perf_invalid_context;
 	perf_iommu->pmu.attr_groups = amd_iommu_attr_groups;
-	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
+
+	ret = perf_pmu_register(&perf_iommu->pmu, perf_iommu->name, -1);
 	if (ret)
 		pr_err("Error initializing AMD IOMMU perf counters.\n");
 	else
-		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
-			amd_iommu_pc_get_max_banks(0),
-			amd_iommu_pc_get_max_counters(0));
+	pr_info("Detected AMD IOMMU #%d (%d banks, %d counters/bank)\n",
+		idx, amd_iommu_pc_get_max_banks(idx),
+		amd_iommu_pc_get_max_counters(idx));
 	return ret;
 }
 
-static struct perf_amd_iommu __perf_iommu = {
-	.pmu = {
-		.task_ctx_nr    = perf_invalid_context,
-		.event_init	= perf_iommu_event_init,
-		.add		= perf_iommu_add,
-		.del		= perf_iommu_del,
-		.start		= perf_iommu_start,
-		.stop		= perf_iommu_stop,
-		.read		= perf_iommu_read,
-	},
-};
-
 static __init int amd_iommu_pc_init(void)
 {
 	int ret;
+	unsigned int i;
+
+	/* Init cpumask attributes to only core 0 */
+	cpumask_set_cpu(0, &iommu_cpumask);
 
 	/* Make sure the IOMMU PC resource is available */
 	if (!amd_iommu_pc_supported())
@@ -470,7 +476,21 @@ static __init int amd_iommu_pc_init(void)
 	if (ret)
 		goto err_out;
 
-	ret = _init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
+	for (i = 0 ; i < amd_iommu_get_num_iommus(); i++) {
+		struct perf_amd_iommu *pi;
+
+		pi = kzalloc(sizeof(struct perf_amd_iommu), GFP_KERNEL);
+		if (!pi) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		list_add_tail(&pi->list, &perf_amd_iommu_list);
+		ret = init_one_perf_amd_iommu(pi, i);
+		if (ret)
+			break;
+	}
+
 	if (ret)
 		goto err_out;
 
-- 
1.8.3.1

Re: [PATCH v8 0/9] perf/amd/iommu: Enable multi-IOMMU support

[Joerg Roedel]

On Mon, Jan 16, 2017 at 01:23:27AM -0600, Suthikulpanit, Suravee wrote:
> Suravee Suthikulpanit (9):
>   perf/amd/iommu: Declare pr_fmt and remove unnecessary pr_debug
>   perf/amd/iommu: Clean up perf_iommu_enable_event
>   perf/amd/iommu: Misc fix up perf_iommu_read
>   iommu/amd: Introduce amd_iommu_get_num_iommus()
>   perf/amd/iommu: Modify functions to query max banks and counters
>   perf/amd/iommu: Modify amd_iommu_pc_get_set_reg_val() API to allow
>     specifying IOMMU index
>   perf/amd/iommu: Check return value when set and get counter value
>   perf/amd/iommu: Fix sysfs perf attribute groups
>   perf/amd/iommu: Enable support for multiple IOMMUs

For the iommu changes:

	Acked-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH v8 2/9] perf/amd/iommu: Clean up perf_iommu_enable_event

[Borislav Petkov]

On Mon, Jan 16, 2017 at 01:23:29AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> * Clean up various bitwise operations in perf_iommu_enable_event
								  ()

It is a function.

> * Make use macros BIT(x)

"Make use"?

> This should not affect logic and functionality.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 44638d0..1aa25d8 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -164,11 +164,11 @@ static int get_next_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu)
>  	for (bank = 0, shift = 0; bank < max_banks; bank++) {
>  		for (cntr = 0; cntr < max_cntrs; cntr++) {
>  			shift = bank + (bank*3) + cntr;
> -			if (perf_iommu->cntr_assign_mask & (1ULL<<shift)) {
> +			if (perf_iommu->cntr_assign_mask & BIT(shift)) {

							   BIT_ULL()

otherwise you're introducing a bug.

>  				continue;
>  			} else {
> -				perf_iommu->cntr_assign_mask |= (1ULL<<shift);
> -				retval = ((u16)((u16)bank<<8) | (u8)(cntr));
> +				perf_iommu->cntr_assign_mask |= BIT(shift);

Ditto.

> +				retval = ((u16)((u16)bank << 8) | (u8)(cntr));

That's some ugly casting. Why?

(u8)(cntr) is supposed to do what exactly? Cut off to 255 if max_cntrs
grows bigger? Can that even happen?

Same for bank?

Then you're casting to u16 even though retval is int. Ah, because it can
be negative too.

How about this instead?

	retval = ((bank & 0xff) << 8) | (cntr & 0xff);

This way it is really obvious what you're trying to do.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v8 3/9] perf/amd/iommu: Misc fix up perf_iommu_read

[Borislav Petkov]

> Subject: Re: [PATCH v8 3/9] perf/amd/iommu: Misc fix up perf_iommu_read

Please be a bit more thorough when writing your commit messages. What is
a "misc fix up"? Perhaps it is ok for a quick'n'dirty local patch but
not when it is for upstream.

Also, function names end with "()" to denote they're functions.

On Mon, Jan 16, 2017 at 01:23:30AM -0600, Suravee Suthikulpanit wrote:
> * Fix overflow handling since u64 delta would lose the MSB sign bit.
> * Remove unnecessary local64_cmpxchg().
> * Coding style and make use of GENMASK_ULL macro.

That last one doesn't read like a sentence.

> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 1aa25d8..3f1c18a 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -320,9 +320,8 @@ 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;
> -	u64 delta = 0ULL;
> +	u64 count, prev;
> +	s64 delta;
>  	struct hw_perf_event *hwc = &event->hw;
>  
>  	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
> @@ -330,18 +329,20 @@ static void perf_iommu_read(struct perf_event *event)
>  				IOMMU_PC_COUNTER_REG, &count, false);
>  
>  	/* IOMMU pc counter register is only 48 bits */
> -	count &= 0xFFFFFFFFFFFFULL;
> +	count &= GENMASK_ULL(48, 0);

GENMASK_ULL(48, 0) gives 0x1ffffffffffff, however. Which is 49 bits.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v8 4/9] iommu/amd: Introduce amd_iommu_get_num_iommus()

[Borislav Petkov]

On Mon, Jan 16, 2017 at 01:23:31AM -0600, Suravee Suthikulpanit wrote:
> Introduce amd_iommu_get_num_iommus(), which returns the value of
> amd_iommus_present, then replaces the direct access to the variable
		     ^
		   which

> which is now declared as static.
> 
> This function will also be used by Perf AMD IOMMU driver.
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.h     |  2 ++
>  drivers/iommu/amd_iommu.c       |  6 +++---
>  drivers/iommu/amd_iommu_init.c  | 11 +++++++++--
>  drivers/iommu/amd_iommu_proto.h |  1 +
>  drivers/iommu/amd_iommu_types.h |  3 ---
>  5 files changed, 15 insertions(+), 8 deletions(-)

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v8 5/9] perf/amd/iommu: Modify functions to query max banks and counters

[Borislav Petkov]

On Mon, Jan 16, 2017 at 01:23:32AM -0600, Suravee Suthikulpanit wrote:
> Currently, amd_iommu_pc_get_max_[banks|counters]() use end-point
> device ID to locate an IOMMU and check the reported max banks/counters.
> The logic assumes that the IOMMU_BASE_DEVID belongs to the first IOMMU,
> and uses it to acquire a reference to the first IOMMU, which does not work
> on certain systems. Instead, we modify the function to take IOMMU index,
							     ^
s/we //							    an

> and use it to query the corresponded AMD IOMMU instance.

corresponding

> Note that we currently hard-code the IOMMU index to 0, since the current
> AMD IOMMU perf implementation only supports single IOMMU. Subsequent patch
					     ^		   ^
					     a		   A subsequent ...

> will add support for multi-IOMMU, and will use proper IOMMU index.
						^
s/multi-IOMMU/multiple IOMMUs/			a/the 

> 
> This patch also removes unnecessary function declaration in
> amd_iommu_proto.h.

This sentence can go away: never write "what" the patch is doing -
always "why" it is doing it.

> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c     | 17 +++++++----------
>  arch/x86/events/amd/iommu.h     |  7 ++-----
>  drivers/iommu/amd_iommu_init.c  | 36 ++++++++++++++++++++++--------------
>  drivers/iommu/amd_iommu_proto.h |  2 --
>  4 files changed, 31 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 3f1c18a..ec7e873 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -239,14 +239,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;
> @@ -453,6 +445,11 @@ static __init int _init_perf_amd_iommu(
>  		return ret;
>  	}
>  
> +	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
> +	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);

Align vertically on the "=".

> +	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
> +		return -EINVAL;
> +
>  	perf_iommu->null_group = NULL;
>  	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
>  
-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v8 6/9] perf/amd/iommu: Modify amd_iommu_pc_get_set_reg_val() API to allow specifying IOMMU index

[Borislav Petkov]

On Mon, Jan 16, 2017 at 01:23:33AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> The current amd_iommu_pc_get_set_reg_val() cannot support multiple IOMMUs
> It is also confusing since it is trying to support set and get in
> one function.
> 
> So break it down to amd_iommu_pc_[get|set]_reg(),
> and modifies them to allow callers to specify IOMMU index. This prepares

  and modify

> the driver for supporting multi-IOMMU in subsequent patch.
> 
> Also remove unnecessary function declarations in amd_iommu_proto.h.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c     | 34 ++++++++++----------------
>  arch/x86/events/amd/iommu.h     |  7 ++++--
>  drivers/iommu/amd_iommu_init.c  | 53 ++++++++++++++++++++++++++---------------
>  drivers/iommu/amd_iommu_proto.h |  5 ----
>  4 files changed, 52 insertions(+), 47 deletions(-)

...

> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index ed21307d..5b7fb6c 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -253,10 +253,6 @@ enum iommu_init_state {
>  static int __init iommu_go_to_state(enum iommu_init_state state);
>  static void init_device_table_dma(void);
>  
> -static int iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
> -				    u8 bank, u8 cntr, u8 fxn,
> -				    u64 *value, bool is_write);
> -
>  static inline void update_last_devid(u16 devid)
>  {
>  	if (devid > amd_iommu_last_bdf)
> @@ -1481,6 +1477,14 @@ static int __init init_iommu_all(struct acpi_table_header *table)
>  	return 0;
>  }
>  
> +#define iommu_pc_get_reg(i, b, c, f, v) \
> +	iommu_pc_get_set_reg(i, b, c, f, v, false)
> +
> +#define iommu_pc_set_reg(i, b, c, f, v) \
> +	iommu_pc_get_set_reg(i, b, c, f, v, true)


Those are completely useless indirection. Just call
iommu_pc_get_set_reg() directly.

> +
> +static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
> +				u8 fxn, u64 *value, bool is_write);
>  
>  static void init_iommu_perf_ctr(struct amd_iommu *iommu)
>  {


-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v8 7/9] perf/amd/iommu: Check return value when set and get counter value

[Borislav Petkov]

On Mon, Jan 16, 2017 at 01:23:34AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> In, perf_iommu_start(), we need to check the return value from
> amd_iommu_set_reg(). In case of failure, we should not enable the PMU.
> 
> Also, in perf_iommu_read(), we need to check the return value from
> amd_iommu_get_reg() before using the value.

So basically you wanna say:

"Add amd_iommu_{g,s}et_reg() error handling."

> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 200d2e8..cc7bea4 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -284,6 +284,7 @@ static void perf_iommu_disable_event(struct perf_event *event)
>  
>  static void perf_iommu_start(struct perf_event *event, int flags)
>  {
> +	u64 val;
>  	struct hw_perf_event *hwc = &event->hw;
>  
>  	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> @@ -292,15 +293,16 @@ static void perf_iommu_start(struct perf_event *event, int flags)
>  	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
>  	hwc->state = 0;
>  
> -	if (flags & PERF_EF_RELOAD) {
> -		u64 prev_raw_count =  local64_read(&hwc->prev_count);
> -		amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
> -				     IOMMU_PC_COUNTER_REG, &prev_raw_count);
> -	}
> +	if (!(flags & PERF_EF_RELOAD))
> +		return;

This looks wrong - you still need to do

	perf_iommu_enable_event(event);
	perf_event_update_userpage(event);

even if !PERF_EF_RELOAD.

> +	val = local64_read(&hwc->prev_count) & GENMASK_ULL(48, 0);
> +	if (amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
> +				   IOMMU_PC_COUNTER_REG, &val))
> +		return;
>  
>  	perf_iommu_enable_event(event);
>  	perf_event_update_userpage(event);
> -
>  }

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v8 8/9] perf/amd/iommu: Fix sysfs perf attribute groups

[Borislav Petkov]

On Mon, Jan 16, 2017 at 01:23:35AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Introduce static amd_iommu_attr_groups to simplify the
> sysfs attributes initialization code.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c | 85 ++++++++++++++++++++-------------------------
>  1 file changed, 37 insertions(+), 48 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index cc7bea4..223c01d 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -43,14 +43,8 @@ struct perf_amd_iommu {
>  	u8 max_counters;
>  	u64 cntr_assign_mask;
>  	raw_spinlock_t lock;
> -	const struct attribute_group *attr_groups[4];
>  };
>  
> -#define format_group	attr_groups[0]
> -#define cpumask_group	attr_groups[1]
> -#define events_group	attr_groups[2]
> -#define null_group	attr_groups[3]
> -
>  /*---------------------------------------------
>   * sysfs format attributes
>   *---------------------------------------------*/
> @@ -81,6 +75,10 @@ struct perf_amd_iommu {
>  /*---------------------------------------------
>   * sysfs events attributes
>   *---------------------------------------------*/
> +static struct attribute_group amd_iommu_events_group = {
> +	.name = "events",
> +};
> +
>  struct amd_iommu_event_desc {
>  	struct kobj_attribute attr;
>  	const char *event;
> @@ -388,76 +386,63 @@ static void perf_iommu_del(struct perf_event *event, int flags)
>  	perf_event_update_userpage(event);
>  }
>  
> -static __init int _init_events_attrs(struct perf_amd_iommu *perf_iommu)
> +static __init int _init_events_attrs(void)
>  {
> -	struct attribute **attrs;
> -	struct attribute_group *attr_group;
>  	int i = 0, j;
> +	struct attribute **attrs;
>  
>  	while (amd_iommu_v2_event_descs[i].attr.attr.name)
>  		i++;
>  
> -	attr_group = kzalloc(sizeof(struct attribute *)
> -		* (i + 1) + sizeof(*attr_group), GFP_KERNEL);
> -	if (!attr_group)
> +	attrs = kzalloc(sizeof(struct attribute **) * (i + 1), GFP_KERNEL);
> +	if (!attrs)
>  		return -ENOMEM;
>  
> -	attrs = (struct attribute **)(attr_group + 1);
>  	for (j = 0; j < i; j++)
>  		attrs[j] = &amd_iommu_v2_event_descs[j].attr.attr;
>  
> -	attr_group->name = "events";
> -	attr_group->attrs = attrs;
> -	perf_iommu->events_group = attr_group;
> -
> +	amd_iommu_events_group.attrs = attrs;
>  	return 0;
>  }
>  
>  static __init void amd_iommu_pc_exit(void)
>  {
> -	if (__perf_iommu.events_group != NULL) {
> -		kfree(__perf_iommu.events_group);
> -		__perf_iommu.events_group = NULL;
> +	if (amd_iommu_events_group.attrs) {
> +		kfree(amd_iommu_events_group.attrs);


Please integrate scripts/checkpatch.pl in your patch creation workflow.
Some of the warnings/errors *actually* make sense. Like this one:

WARNING: kfree(NULL) is safe and this check is probably not required
#98: FILE: arch/x86/events/amd/iommu.c:411:
+       if (amd_iommu_events_group.attrs) {
+               kfree(amd_iommu_events_group.attrs);

> +		amd_iommu_events_group.attrs = NULL;

You keep doing that. Why?

>  	}
>  }
>  
> -static __init int _init_perf_amd_iommu(
> -	struct perf_amd_iommu *perf_iommu, char *name)
> +const struct attribute_group *amd_iommu_attr_groups[] = {
> +	&amd_iommu_format_group,
> +	&amd_iommu_cpumask_group,
> +	&amd_iommu_events_group,
> +	NULL,
> +};
> +
> +static __init int
> +_init_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, char *name)
>  {
>  	int ret;
>  
>  	raw_spin_lock_init(&perf_iommu->lock);
>  
> -	perf_iommu->format_group = &amd_iommu_format_group;
> -
>  	/* Init cpumask attributes to only core 0 */
>  	cpumask_set_cpu(0, &iommu_cpumask);
> -	perf_iommu->cpumask_group = &amd_iommu_cpumask_group;
> -
> -	ret = _init_events_attrs(perf_iommu);
> -	if (ret) {
> -		pr_err("Error initializing AMD IOMMU perf events.\n");
> -		return ret;
> -	}
>  
>  	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
>  	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
>  	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
>  		return -EINVAL;
>  
> -	perf_iommu->null_group = NULL;
> -	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
> -
> +	perf_iommu->pmu.attr_groups = amd_iommu_attr_groups;
>  	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
> -	if (ret) {
> +	if (ret)
>  		pr_err("Error initializing AMD IOMMU perf counters.\n");
> -		amd_iommu_pc_exit();
> -	} else {
> +	else
>  		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
>  			amd_iommu_pc_get_max_banks(0),
>  			amd_iommu_pc_get_max_counters(0));
> -	}
> -
>  	return ret;
>  }
>  
> @@ -471,24 +456,28 @@ static __init int _init_perf_amd_iommu(
>  		.stop		= perf_iommu_stop,
>  		.read		= perf_iommu_read,
>  	},
> -	.max_banks		= 0x00,
> -	.max_counters		= 0x00,
> -	.cntr_assign_mask	= 0ULL,
> -	.format_group		= NULL,
> -	.cpumask_group		= NULL,
> -	.events_group		= NULL,
> -	.null_group		= NULL,
>  };
>  
>  static __init int amd_iommu_pc_init(void)
>  {
> +	int ret;
> +
>  	/* Make sure the IOMMU PC resource is available */
>  	if (!amd_iommu_pc_supported())
>  		return -ENODEV;
>  
> -	_init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
> +	ret = _init_events_attrs();
> +	if (ret)
> +		goto err_out;

So that's actually

	if (ret)
		return ret;

to propagate the -ENOMEM.

>  
> -	return 0;
> +	ret = _init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
> +	if (ret)
> +		goto err_out;

Then here you can do:

	if (ret)
		amd_iommu_pc_exit();

	return ret;

and simplify it a bit and get rid of the err_out label.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Borislav Petkov]

On Mon, Jan 16, 2017 at 01:23:36AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Add multi-IOMMU support for perf by exposing an AMD IOMMU PMU
> for each IOMMU found in the system via:
> 
>   /bus/event_source/devices/amd_iommu_x
> 
> where x is the IOMMU index. This allows users to specify
> different events to be programed onto performance counters

"programmed"

Please introduce a spellchecker into your patch creation workflow.

> of each IOMMU.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c | 114 ++++++++++++++++++++++++++------------------
>  1 file changed, 67 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 223c01d..38eafbf 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -35,16 +35,21 @@
>  #define _GET_PASID_MASK(ev) ((ev->hw.extra_reg.config >> 16) & 0xFFFFULL)
>  #define _GET_DOMID_MASK(ev) ((ev->hw.extra_reg.config >> 32) & 0xFFFFULL)
>  
> -static struct perf_amd_iommu __perf_iommu;
> +#define PERF_AMD_IOMMU_NAME_SZ 16

AMD_IOMMU_PMU_NAME_SIZE

sounds more to the point to me.

>  struct perf_amd_iommu {
> +	struct list_head list;
>  	struct pmu pmu;
> +	unsigned int idx;
> +	char name[PERF_AMD_IOMMU_NAME_SZ];
>  	u8 max_banks;
>  	u8 max_counters;
>  	u64 cntr_assign_mask;
>  	raw_spinlock_t lock;
>  };

...

> @@ -253,30 +248,34 @@ static void perf_iommu_enable_event(struct perf_event *ev)
>  	u64 reg = 0ULL;
>  
>  	reg = csource;
> -	amd_iommu_pc_set_reg(0, bank, cntr,
> +	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
>  			     IOMMU_PC_COUNTER_SRC_REG, &reg);
>  
>  	reg = devid | (_GET_DEVID_MASK(ev) << 32);
>  	if (reg)
>  		reg |= BIT(31);
> -	amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_DEVID_MATCH_REG, &reg);
> +	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
> +			     IOMMU_PC_DEVID_MATCH_REG, &reg);
>  
>  	reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
>  	if (reg)
>  		reg |= BIT(31);
> -	amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_PASID_MATCH_REG, &reg);
> +	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
> +			     IOMMU_PC_PASID_MATCH_REG, &reg);
>  
>  	reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
>  	if (reg)
>  		reg |= BIT(31);
> -	amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_DOMID_MATCH_REG, &reg);
> +	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
> +			     IOMMU_PC_DOMID_MATCH_REG, &reg);

You can let those stick out - the 80 cols rule is not a strict one:

        reg = csource;
        amd_iommu_pc_set_reg(hwc->idx, bank, cntr, IOMMU_PC_COUNTER_SRC_REG, &reg);

        reg = devid | (_GET_DEVID_MASK(ev) << 32);
        if (reg)
                reg |= BIT(31);
        amd_iommu_pc_set_reg(hwc->idx, bank, cntr, IOMMU_PC_DEVID_MATCH_REG, &reg);

        reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
        if (reg)
                reg |= BIT(31);
        amd_iommu_pc_set_reg(hwc->idx, bank, cntr, IOMMU_PC_PASID_MATCH_REG, &reg);

        reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
        if (reg)
                reg |= BIT(31);
        amd_iommu_pc_set_reg(hwc->idx, bank, cntr, IOMMU_PC_DOMID_MATCH_REG, &reg);
}

>  static void perf_iommu_disable_event(struct perf_event *event)
>  {
> +	struct hw_perf_event *hwc = &event->hw;
>  	u64 reg = 0ULL;
>  
> -	amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
> +	amd_iommu_pc_set_reg(hwc->idx, _GET_BANK(event), _GET_CNTR(event),
>  			     IOMMU_PC_COUNTER_SRC_REG, &reg);
>  }
>  
> @@ -295,7 +294,7 @@ static void perf_iommu_start(struct perf_event *event, int flags)
>  		return;
>  
>  	val = local64_read(&hwc->prev_count) & GENMASK_ULL(48, 0);
> -	if (amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
> +	if (amd_iommu_pc_set_reg(hwc->idx, _GET_BANK(event), _GET_CNTR(event),
>  				   IOMMU_PC_COUNTER_REG, &val))
>  		return;
>  
> @@ -309,7 +308,7 @@ static void perf_iommu_read(struct perf_event *event)
>  	s64 delta;
>  	struct hw_perf_event *hwc = &event->hw;
>  
> -	if (amd_iommu_pc_get_reg(0, _GET_BANK(event), _GET_CNTR(event),
> +	if (amd_iommu_pc_get_reg(hwc->idx, _GET_BANK(event), _GET_CNTR(event),
>  				 IOMMU_PC_COUNTER_REG, &count))
>  		return;
>  
> @@ -407,6 +406,13 @@ static __init int _init_events_attrs(void)
>  
>  static __init void amd_iommu_pc_exit(void)
>  {
> +	struct perf_amd_iommu *pi, *next;
> +
> +	list_for_each_entry_safe(pi, next, &perf_amd_iommu_list, list) {
> +		list_del(&pi->list);
> +		kfree(pi);
> +	}
> +
>  	if (amd_iommu_events_group.attrs) {
>  		kfree(amd_iommu_events_group.attrs);
>  		amd_iommu_events_group.attrs = NULL;
> @@ -421,46 +427,46 @@ static __init void amd_iommu_pc_exit(void)
>  };
>  
>  static __init int
> -_init_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, char *name)
> +init_one_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, unsigned int idx)
>  {
>  	int ret;
>  
>  	raw_spin_lock_init(&perf_iommu->lock);
>  
> -	/* Init cpumask attributes to only core 0 */
> -	cpumask_set_cpu(0, &iommu_cpumask);
> -
> -	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
> -	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
> +	perf_iommu->idx          = idx;
> +	perf_iommu->max_banks    = amd_iommu_pc_get_max_banks(idx);
> +	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(idx);
>  	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
>  		return -EINVAL;
>  
> +	snprintf(perf_iommu->name, PERF_AMD_IOMMU_NAME_SZ, "amd_iommu_%u", idx);
> +
> +	perf_iommu->pmu.event_init  = perf_iommu_event_init,
> +	perf_iommu->pmu.add         = perf_iommu_add,
> +	perf_iommu->pmu.del         = perf_iommu_del,
> +	perf_iommu->pmu.start       = perf_iommu_start,
> +	perf_iommu->pmu.stop        = perf_iommu_stop,
> +	perf_iommu->pmu.read        = perf_iommu_read,

This compiles but it is yucky.

You should do that instead:

static struct pmu amd_iommu_pmu = {
        .event_init  = perf_iommu_event_init,
        .add         = perf_iommu_add,
        .del         = perf_iommu_del,
        .start       = perf_iommu_start,
        .stop        = perf_iommu_stop,
        .read        = perf_iommu_read,
        .task_ctx_nr = perf_invalid_context,
        .attr_groups = amd_iommu_attr_groups,
};

...

	ret = perf_pmu_register(&amd_iommu_pmu, perf_iommu->name, -1);

Because otherwise you're carrying a struct pmu in each struct
perf_amd_iommu which has identical contents.

Now, you need to access the struct perf_amd_iommu pointer for each
IOMMU PMU in some of the functions like perf_iommu_event_init(), for
example. But for that you only need the index and to iterate the
perf_amd_iommu_list.

I wasn't able to find a good way to do that from a quick stare but
PeterZ might have a better idea...

> +	perf_iommu->pmu.task_ctx_nr = perf_invalid_context;
>  	perf_iommu->pmu.attr_groups = amd_iommu_attr_groups;
> -	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
> +
> +	ret = perf_pmu_register(&perf_iommu->pmu, perf_iommu->name, -1);
>  	if (ret)
>  		pr_err("Error initializing AMD IOMMU perf counters.\n");
>  	else
> -		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
> -			amd_iommu_pc_get_max_banks(0),
> -			amd_iommu_pc_get_max_counters(0));
> +	pr_info("Detected AMD IOMMU #%d (%d banks, %d counters/bank)\n",
> +		idx, amd_iommu_pc_get_max_banks(idx),
> +		amd_iommu_pc_get_max_counters(idx));
>  	return ret;
>  }
>  
> -static struct perf_amd_iommu __perf_iommu = {
> -	.pmu = {
> -		.task_ctx_nr    = perf_invalid_context,
> -		.event_init	= perf_iommu_event_init,
> -		.add		= perf_iommu_add,
> -		.del		= perf_iommu_del,
> -		.start		= perf_iommu_start,
> -		.stop		= perf_iommu_stop,
> -		.read		= perf_iommu_read,
> -	},
> -};
> -
>  static __init int amd_iommu_pc_init(void)
>  {
>  	int ret;
> +	unsigned int i;
> +
> +	/* Init cpumask attributes to only core 0 */
> +	cpumask_set_cpu(0, &iommu_cpumask);
>  
>  	/* Make sure the IOMMU PC resource is available */
>  	if (!amd_iommu_pc_supported())
> @@ -470,7 +476,21 @@ static __init int amd_iommu_pc_init(void)
>  	if (ret)
>  		goto err_out;
>  
> -	ret = _init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
> +	for (i = 0 ; i < amd_iommu_get_num_iommus(); i++) {
> +		struct perf_amd_iommu *pi;
> +
> +		pi = kzalloc(sizeof(struct perf_amd_iommu), GFP_KERNEL);
> +		if (!pi) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
> +		list_add_tail(&pi->list, &perf_amd_iommu_list);
> +		ret = init_one_perf_amd_iommu(pi, i);

You need to init *first* and iff you succeed, only *then* add to the
list.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v8 7/9] perf/amd/iommu: Check return value when set and get counter value

[Peter Zijlstra]

On Mon, Jan 16, 2017 at 01:23:34AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> In, perf_iommu_start(), we need to check the return value from
> amd_iommu_set_reg(). In case of failure, we should not enable the PMU.
> 
> Also, in perf_iommu_read(), we need to check the return value from
> amd_iommu_get_reg() before using the value.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/events/amd/iommu.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 200d2e8..cc7bea4 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -284,6 +284,7 @@ static void perf_iommu_disable_event(struct perf_event *event)
>  
>  static void perf_iommu_start(struct perf_event *event, int flags)
>  {
> +	u64 val;
>  	struct hw_perf_event *hwc = &event->hw;
>  
>  	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> @@ -292,15 +293,16 @@ static void perf_iommu_start(struct perf_event *event, int flags)
>  	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
>  	hwc->state = 0;
>  
> -	if (flags & PERF_EF_RELOAD) {
> -		u64 prev_raw_count =  local64_read(&hwc->prev_count);
> -		amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
> -				     IOMMU_PC_COUNTER_REG, &prev_raw_count);
> -	}
> +	if (!(flags & PERF_EF_RELOAD))
> +		return;
> +
> +	val = local64_read(&hwc->prev_count) & GENMASK_ULL(48, 0);
> +	if (amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event),
> +				   IOMMU_PC_COUNTER_REG, &val))
> +		return;

Under what conditions could this fail? ::start() is not an interface
that expects failure, if we get this far, things should really just
work.

Re: [PATCH v8 3/9] perf/amd/iommu: Misc fix up perf_iommu_read

[Peter Zijlstra]

On Mon, Jan 16, 2017 at 01:23:30AM -0600, Suravee Suthikulpanit wrote:
>  static void perf_iommu_read(struct perf_event *event)
>  {
> -	u64 count = 0ULL;
> -	u64 prev_raw_count = 0ULL;
> -	u64 delta = 0ULL;
> +	u64 count, prev;
> +	s64 delta;

I did send that email where I told I was mistaken with that suggestion,
right? Since the counter always increments (it does, right), a negative
delta does not make sense.

>  	struct hw_perf_event *hwc = &event->hw;
>  
>  	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
> @@ -330,18 +329,20 @@ static void perf_iommu_read(struct perf_event *event)
>  				IOMMU_PC_COUNTER_REG, &count, false);
>  
>  	/* IOMMU pc counter register is only 48 bits */
> -	count &= 0xFFFFFFFFFFFFULL;
> +	count &= GENMASK_ULL(48, 0);

Why do you need that at all? If the counter is only 48 bits, what does
the hardware do with the upper bits?

>  
> -	prev_raw_count =  local64_read(&hwc->prev_count);
> -	if (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
> -					count) != prev_raw_count)
> -		return;
> +	prev = local64_read(&hwc->prev_count);

I'm still not convinced you can do away with that cmpxchg.

>  
> -	/* Handling 48-bit counter overflowing */
> -	delta = (count << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT);
> +	/*
> +	 * Since we do not enable counter overflow interrupts,
> +	 * we do not have to worry about prev_count changing on us.
> +	 */
> +	local64_set(&hwc->prev_count, count);
> +
> +	/* Handle 48-bit counter overflow */
> +	delta = (count << COUNTER_SHIFT) - (prev << COUNTER_SHIFT);
>  	delta >>= COUNTER_SHIFT;
>  	local64_add(delta, &event->count);
> -
>  }
>  
>  static void perf_iommu_stop(struct perf_event *event, int flags)
> -- 
> 1.8.3.1
> 

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Peter Zijlstra]

On Mon, Jan 16, 2017 at 01:23:36AM -0600, Suravee Suthikulpanit wrote:

> +	pi = container_of(event->pmu, struct perf_amd_iommu, pmu);
> +	hwc->idx              = pi->idx;
> +	hwc->config           = event->attr.config;
> +	hwc->extra_reg.config = event->attr.config1;

>  static void perf_iommu_enable_event(struct perf_event *ev)
>  {
> +	struct hw_perf_event *hwc = &ev->hw;
>  	u8 csource = _GET_CSOURCE(ev);
>  	u16 devid = _GET_DEVID(ev);
>  	u8 bank = _GET_BANK(ev);
> @@ -253,30 +248,34 @@ static void perf_iommu_enable_event(struct perf_event *ev)
>  	u64 reg = 0ULL;
>  
>  	reg = csource;
> -	amd_iommu_pc_set_reg(0, bank, cntr,
> +	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
>  			     IOMMU_PC_COUNTER_SRC_REG, &reg);

Please explain about this IOMMU crud, this looks like fail.

hwc->idx should be the counter, not a random pmu index.

But instead it looks like you get the counter form:

  #define _GET_CNTR(ev)       ((u8)(ev->hw.extra_reg.reg))

Which is absolutely insane.

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Borislav Petkov]

On Wed, Jan 25, 2017 at 10:46:53AM +0100, Peter Zijlstra wrote:
> Which is absolutely insane.

Right,

IMO, the simplest thing to do for your purposes is to embed a struct
amd_iommu pointer into struct perf_amd_iommu at init time so that you
don't have to do all that crazy dance in the PMU functions and iterate
over the iommus in get_amd_iommu() each time.

Which would then simplify all your other functions. For example:

int amd_iommu_pc_get_reg(unsigned int idx, u8 bank, u8 cntr, u8 fxn, u64 *value)

should be

int amd_iommu_pc_get_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, u8 fxn, u64 *value)

and you can save yourself a lot of glue code and get rid of that
get_amd_iommu() thing.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

Boris,

On 1/23/17 02:55, Borislav Petkov wrote:
>> @@ -421,46 +427,46 @@ static __init void amd_iommu_pc_exit(void)
>>  };
>>
>>  static __init int
>> -_init_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, char *name)
>> +init_one_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, unsigned int idx)
>>  {
>>  	int ret;
>>
>>  	raw_spin_lock_init(&perf_iommu->lock);
>>
>> -	/* Init cpumask attributes to only core 0 */
>> -	cpumask_set_cpu(0, &iommu_cpumask);
>> -
>> -	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
>> -	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
>> +	perf_iommu->idx          = idx;
>> +	perf_iommu->max_banks    = amd_iommu_pc_get_max_banks(idx);
>> +	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(idx);
>>  	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
>>  		return -EINVAL;
>>
>> +	snprintf(perf_iommu->name, PERF_AMD_IOMMU_NAME_SZ, "amd_iommu_%u", idx);
>> +
>> +	perf_iommu->pmu.event_init  = perf_iommu_event_init,
>> +	perf_iommu->pmu.add         = perf_iommu_add,
>> +	perf_iommu->pmu.del         = perf_iommu_del,
>> +	perf_iommu->pmu.start       = perf_iommu_start,
>> +	perf_iommu->pmu.stop        = perf_iommu_stop,
>> +	perf_iommu->pmu.read        = perf_iommu_read,
> This compiles but it is yucky.
>
> You should do that instead:
>
> static struct pmu amd_iommu_pmu = {
>         .event_init  = perf_iommu_event_init,
>         .add         = perf_iommu_add,
>         .del         = perf_iommu_del,
>         .start       = perf_iommu_start,
>         .stop        = perf_iommu_stop,
>         .read        = perf_iommu_read,
>         .task_ctx_nr = perf_invalid_context,
>         .attr_groups = amd_iommu_attr_groups,
> };
>
> ...
>
> 	ret = perf_pmu_register(&amd_iommu_pmu, perf_iommu->name, -1);
>
> Because otherwise you're carrying a struct pmu in each struct
> perf_amd_iommu which has identical contents.

Actually, only the callbacks above will be identical on each pmu, but
there are other parts of the structure which are different
(e.g. pmu->name, pmu->type, etc.) Also, we need one pmu instance per
IOMMU since each pmu reference will get assigned to perf_event, and
also used to reference back to struct perf_amd_iommu. Note that each
pmu can also have different events.

> Now, you need to access the struct perf_amd_iommu pointer for each
> IOMMU PMU in some of the functions like perf_iommu_event_init(), for
> example. But for that you only need the index and to iterate the
> perf_amd_iommu_list.

I think replacing the index w/ pointer to amd_iommu is a good idea.
I'm changing this in V9.

Thanks,
Suravee
>
> I wasn't able to find a good way to do that from a quick stare but
> PeterZ might have a better idea...
>

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

Peter,

On 1/25/17 16:46, Peter Zijlstra wrote:
> On Mon, Jan 16, 2017 at 01:23:36AM -0600, Suravee Suthikulpanit wrote:
>
>> +	pi = container_of(event->pmu, struct perf_amd_iommu, pmu);
>> +	hwc->idx              = pi->idx;
>> +	hwc->config           = event->attr.config;
>> +	hwc->extra_reg.config = event->attr.config1;
>
>>  static void perf_iommu_enable_event(struct perf_event *ev)
>>  {
>> +	struct hw_perf_event *hwc = &ev->hw;
>>  	u8 csource = _GET_CSOURCE(ev);
>>  	u16 devid = _GET_DEVID(ev);
>>  	u8 bank = _GET_BANK(ev);
>> @@ -253,30 +248,34 @@ static void perf_iommu_enable_event(struct perf_event *ev)
>>  	u64 reg = 0ULL;
>>
>>  	reg = csource;
>> -	amd_iommu_pc_set_reg(0, bank, cntr,
>> +	amd_iommu_pc_set_reg(hwc->idx, bank, cntr,
>>  			     IOMMU_PC_COUNTER_SRC_REG, &reg);
>
> Please explain about this IOMMU crud, this looks like fail.
> hwc->idx should be the counter, not a random pmu index.

Ok, I thought this was not used by the code, so I used it for
holding PMU index. This might not be a good idea.
I will change this part per Boris comment in the reply of this
thread.

> But instead it looks like you get the counter form:
>
>   #define _GET_CNTR(ev)       ((u8)(ev->hw.extra_reg.reg))
>
> Which is absolutely insane.
>

So, the IOMMU counters are grouped into bank, and there could be
many banks. I use the extra_reg.reg to hold the bank and counter
indices. This will be used to program onto the counter configuration
register. This is handled in get_next_avail_iommu_bnk_cntr() and
clear_avail_iommu_bnk_cntr().

Thanks,
Suravee

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

Boris,

On 1/25/17 16:55, Borislav Petkov wrote:
> On Wed, Jan 25, 2017 at 10:46:53AM +0100, Peter Zijlstra wrote:
>> Which is absolutely insane.
>
> Right,
>
> IMO, the simplest thing to do for your purposes is to embed a struct
> amd_iommu pointer into struct perf_amd_iommu at init time so that you
> don't have to do all that crazy dance in the PMU functions and iterate
> over the iommus in get_amd_iommu() each time.
>
> Which would then simplify all your other functions. For example:
>
> int amd_iommu_pc_get_reg(unsigned int idx, u8 bank, u8 cntr, u8 fxn, u64 *value)
>
> should be
>
> int amd_iommu_pc_get_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, u8 fxn, u64 *value)
>
> and you can save yourself a lot of glue code and get rid of that
> get_amd_iommu() thing.
>

That's a good idea. Thanks. I'll do this in V9.

Suravee

Re: [PATCH v8 3/9] perf/amd/iommu: Misc fix up perf_iommu_read

[Suravee Suthikulpanit]

Peter,

On 1/23/17 19:33, Peter Zijlstra wrote:
> On Mon, Jan 16, 2017 at 01:23:30AM -0600, Suravee Suthikulpanit wrote:
>>  static void perf_iommu_read(struct perf_event *event)
>>  {
>> -	u64 count = 0ULL;
>> -	u64 prev_raw_count = 0ULL;
>> -	u64 delta = 0ULL;
>> +	u64 count, prev;
>> +	s64 delta;
>
> I did send that email where I told I was mistaken with that suggestion,
> right? Since the counter always increments (it does, right), a negative
> delta does not make sense.

Right, I sent this out before your reply email in V7. I'll change the delta
back to u64. I'll fix this in V9.

>
>>  	struct hw_perf_event *hwc = &event->hw;
>>
>>  	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
>> @@ -330,18 +329,20 @@ static void perf_iommu_read(struct perf_event *event)
>>  				IOMMU_PC_COUNTER_REG, &count, false);
>>
>>  	/* IOMMU pc counter register is only 48 bits */
>> -	count &= 0xFFFFFFFFFFFFULL;
>> +	count &= GENMASK_ULL(48, 0);
>
> Why do you need that at all? If the counter is only 48 bits, what does
> the hardware do with the upper bits?

So, bit 48-63 are reserved. I just try to ensure that the HW does not use them
for anything (also probably in the future).

>>
>> -	prev_raw_count =  local64_read(&hwc->prev_count);
>> -	if (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
>> -					count) != prev_raw_count)
>> -		return;
>> +	prev = local64_read(&hwc->prev_count);
>
> I'm still not convinced you can do away with that cmpxchg.

Ok, I can just leave the local64_read() in.

Thanks,
Suravee

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 08:57:52AM +0700, Suravee Suthikulpanit wrote:
> >But instead it looks like you get the counter form:
> >
> >  #define _GET_CNTR(ev)       ((u8)(ev->hw.extra_reg.reg))
> >
> >Which is absolutely insane.
> >
> 
> So, the IOMMU counters are grouped into bank, and there could be
> many banks. I use the extra_reg.reg to hold the bank and counter
> indices. This will be used to program onto the counter configuration
> register. This is handled in get_next_avail_iommu_bnk_cntr() and
> clear_avail_iommu_bnk_cntr().

But this is crazy. That's not what extra_regs are for. Also, who cares
about the banks, why is this exposed?

That is, I would very much expect a linear range of counters. You can
always decompose this counter number if you really need to somewhere
down near the hardware accessors.

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

Peter,

On 2/14/17 19:31, Peter Zijlstra wrote:
> On Tue, Feb 07, 2017 at 08:57:52AM +0700, Suravee Suthikulpanit wrote:
>>> But instead it looks like you get the counter form:
>>>
>>>  #define _GET_CNTR(ev)       ((u8)(ev->hw.extra_reg.reg))
>>>
>>> Which is absolutely insane.
>>>
>>
>> So, the IOMMU counters are grouped into bank, and there could be
>> many banks. I use the extra_reg.reg to hold the bank and counter
>> indices. This will be used to program onto the counter configuration
>> register. This is handled in get_next_avail_iommu_bnk_cntr() and
>> clear_avail_iommu_bnk_cntr().
>
> But this is crazy. That's not what extra_regs are for.

Ok, I understand that we should not be using the extra_regs
since it is intended for other purposes. Please see more detail below.

> Also, who cares about the banks, why is this exposed?

The bank and counter values are not exposed to the user-space.
The amd_iommu PMU only expose, csource, devid, domid, pasid, devid_mask,
domid_mask, and pasid_mask as event attributes.

> That is, I would very much expect a linear range of counters. You can
> always decompose this counter number if you really need to somewhere
> down near the hardware accessors.
>

Actually, the counters are treated as linear range of counters. For example,
the IOMMU hardware has 2 banks with 4 counters/bank. So, we have total of 8
counters. The driver then assigns an index to each events when an event is added.
Here, the bank/counter are derived from the assigned index, and stored in
the perf_event as bank and counter values.

However, I have looked into reworking to not use the extra_regs, and I see
that the union in struct hw_perf_event currently contains various PMU-specific
structures (hardware, software, tracepoint, intel_cqm, itrace, amd_power,
and breakpoint).

For amd_iommu PMU, we need additional registers for holding amd_iommu-specific
parameters. So, it seems that we can just introduce amd_iommu-specific struct
instead of re-using the existing structure for hardware events.

I'm planning to add the following structure in the same union:

     union {
         ......
                 struct { /* amd_iommu */
                         u8      iommu_csource;
                         u8      iommu_bank;
                         u8      iommu_cntr;
                         u16     iommu_devid;
                         u16     iommu_devid_msk;
                         u16     iommu_domid;
                         u16     iommu_domid_msk;
                         u32     iommu_pasid;
                         u32     iommu_pasid_msk;
                 };
     };

Please let me know what you think, of if I am still missing your points.

Thanks,
Suravee

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Peter Zijlstra]

On Fri, Feb 24, 2017 at 12:43:19AM +0700, Suravee Suthikulpanit wrote:

> >Also, who cares about the banks, why is this exposed?
> 
> The bank and counter values are not exposed to the user-space.
> The amd_iommu PMU only expose, csource, devid, domid, pasid, devid_mask,
> domid_mask, and pasid_mask as event attributes.

Ah good, for a little while I was worried the BANK stuff came from
userspace; I misread extra_reg.config and extra_reg.reg, the former
being perf_event_attr::config1 and the latter holding the bank thing.

> >That is, I would very much expect a linear range of counters. You can
> >always decompose this counter number if you really need to somewhere
> >down near the hardware accessors.
> >
> 
> Actually, the counters are treated as linear range of counters. For example,
> the IOMMU hardware has 2 banks with 4 counters/bank. So, we have total of 8
> counters. The driver then assigns an index to each events when an event is added.
> Here, the bank/counter are derived from the assigned index, and stored in
> the perf_event as bank and counter values.
> 
> However, I have looked into reworking to not use the extra_regs, and I see
> that the union in struct hw_perf_event currently contains various PMU-specific
> structures (hardware, software, tracepoint, intel_cqm, itrace, amd_power,
> and breakpoint).
> 
> For amd_iommu PMU, we need additional registers for holding amd_iommu-specific
> parameters. So, it seems that we can just introduce amd_iommu-specific struct
> instead of re-using the existing structure for hardware events.
> 
> I'm planning to add the following structure in the same union:
> 
>     union {
>         ......
>                 struct { /* amd_iommu */
>                         u8      iommu_csource;
>                         u8      iommu_bank;
>                         u8      iommu_cntr;
>                         u16     iommu_devid;
>                         u16     iommu_devid_msk;
>                         u16     iommu_domid;
>                         u16     iommu_domid_msk;
>                         u32     iommu_pasid;
>                         u32     iommu_pasid_msk;
>                 };
>     };
> 
> Please let me know what you think, of if I am still missing your points.

Yes, adding a struct to that union is fine and clarifies things. And
just because I'm weird like that, there's a u8 hole after iommu_cntr.

Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

On 2/24/17 01:11, Peter Zijlstra wrote:
>> However, I have looked into reworking to not use the extra_regs, and I see
>> that the union in struct hw_perf_event currently contains various PMU-specific
>> structures (hardware, software, tracepoint, intel_cqm, itrace, amd_power,
>> and breakpoint).
>>
>> For amd_iommu PMU, we need additional registers for holding amd_iommu-specific
>> parameters. So, it seems that we can just introduce amd_iommu-specific struct
>> instead of re-using the existing structure for hardware events.
>>
>> I'm planning to add the following structure in the same union:
>>
>>     union {
>>         ......
>>                 struct { /* amd_iommu */
>>                         u8      iommu_csource;
>>                         u8      iommu_bank;
>>                         u8      iommu_cntr;
>>                         u16     iommu_devid;
>>                         u16     iommu_devid_msk;
>>                         u16     iommu_domid;
>>                         u16     iommu_domid_msk;
>>                         u32     iommu_pasid;
>>                         u32     iommu_pasid_msk;
>>                 };
>>     };
>>
>> Please let me know what you think, of if I am still missing your points.
> Yes, adding a struct to that union is fine and clarifies things. And
> just because I'm weird like that, there's a u8 hole after iommu_cntr.

Ok, I'll update this in V10 that I'll be sending out this week.

Thanks,
Suravee