[PATCH v7 0/7] perf/amd/iommu: Enable multi-IOMMU support

[PATCH v7 0/7] 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-v7

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.


Suravee Suthikulpanit (7):
  perf/amd/iommu: Misc fix up perf_iommu_read
  perf/amd/iommu: Modify functions to query max banks and counters
  perf/amd/iommu: Modify IOMMU API to allow specifying IOMMU index
  perf/amd/iommu: Declare pr_fmt and remove unnecessary pr_debug
  perf/amd/iommu: Clean up perf_iommu_enable_event
  iommu/amd: Introduce amd_iommu_get_num_iommus()
  perf/amd/iommu: Enable support for multiple IOMMUs

 arch/x86/events/amd/iommu.c     | 206 ++++++++++++++++++++--------------------
 arch/x86/events/amd/iommu.h     |  17 ++--
 drivers/iommu/amd_iommu_init.c  | 119 ++++++++++++++++-------
 drivers/iommu/amd_iommu_proto.h |   9 +-
 4 files changed, 201 insertions(+), 150 deletions(-)

-- 
1.8.3.1

[PATCH v7 1/7] perf/amd/iommu: Misc fix up perf_iommu_read

[Suravee Suthikulpanit]

This patch contains the following minor fixup:
  * Fixed overflow handling since u64 delta would lose the MSB sign bit.
  * Remove unnecessary local64_set().
  * 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 | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index b28200d..f387baf 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -319,29 +319,30 @@ 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 cnt, prev;
+	s64 delta;
 	struct hw_perf_event *hwc = &event->hw;
 	pr_debug("perf: amd_iommu:perf_iommu_read\n");
 
 	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
 				_GET_BANK(event), _GET_CNTR(event),
-				IOMMU_PC_COUNTER_REG, &count, false);
+				IOMMU_PC_COUNTER_REG, &cnt, false);
 
 	/* IOMMU pc counter register is only 48 bits */
-	count &= 0xFFFFFFFFFFFFULL;
+	cnt &= 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, cnt);
+
+	/* Handle 48-bit counter overflow */
+	delta = (cnt << 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 v7 2/7] 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  | 35 +++++++++++++++++++++--------------
 drivers/iommu/amd_iommu_proto.h |  2 --
 4 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index f387baf..cf94f48 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -237,14 +237,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;
@@ -456,6 +448,11 @@ static __init int _init_perf_amd_iommu(
 	if (_init_events_attrs(perf_iommu) != 0)
 		pr_err("perf: amd_iommu: Only support raw events.\n");
 
+	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
+	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
+	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
+		return -EINVAL;
+
 	/* Init null attributes */
 	perf_iommu->null_group = NULL;
 	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
@@ -466,8 +463,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 845d173..432d867 100644
--- a/arch/x86/events/amd/iommu.h
+++ b/arch/x86/events/amd/iommu.h
@@ -24,15 +24,12 @@
 #define PC_MAX_SPEC_BNKS			64
 #define PC_MAX_SPEC_CNTRS			16
 
-/* iommu pc reg masks*/
-#define IOMMU_BASE_DEVID			0x0000
-
 /* amd_iommu_init.c external support functions */
 extern bool amd_iommu_pc_supported(void);
 
-extern u8 amd_iommu_pc_get_max_banks(u16 devid);
+extern u8 amd_iommu_pc_get_max_banks(uint idx);
 
-extern u8 amd_iommu_pc_get_max_counters(u16 devid);
+extern u8 amd_iommu_pc_get_max_counters(uint 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 157e934..a7e756b 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2706,6 +2706,19 @@ bool amd_iommu_v2_supported(void)
 }
 EXPORT_SYMBOL(amd_iommu_v2_supported);
 
+static struct amd_iommu *get_amd_iommu(uint idx)
+{
+	uint i = 0;
+	struct amd_iommu *iommu = NULL;
+
+	for_each_iommu(iommu) {
+		if (i == idx)
+			break;
+		i++;
+	}
+	return iommu;
+}
+
 /****************************************************************************
  *
  * IOMMU EFR Performance Counter support functionality. This code allows
@@ -2713,17 +2726,14 @@ bool amd_iommu_v2_supported(void)
  *
  ****************************************************************************/
 
-u8 amd_iommu_pc_get_max_banks(u16 devid)
+u8 amd_iommu_pc_get_max_banks(uint 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);
 
@@ -2733,17 +2743,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(uint 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 7eb60c1..60f2eef 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -58,8 +58,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 v7 3/7] perf/amd/iommu: Modify IOMMU API to allow specifying IOMMU index

[Suravee Suthikulpanit]

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

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

This patch also removes 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     | 45 +++++++++++-------------
 arch/x86/events/amd/iommu.h     |  8 +++--
 drivers/iommu/amd_iommu_init.c  | 77 +++++++++++++++++++++++++++++++----------
 drivers/iommu/amd_iommu_proto.h |  7 ++--
 4 files changed, 88 insertions(+), 49 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index cf94f48..9744dc8 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -248,46 +248,44 @@ 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, devid, bank, cntr,
+			     IOMMU_PC_COUNTER_SRC_REG, &reg);
 
 	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);
 	if (reg)
 		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DEVID_MATCH_REG, &reg, true);
+	amd_iommu_pc_set_reg(0, devid, bank, cntr,
+			     IOMMU_PC_DEVID_MATCH_REG, &reg);
 
 	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
 	if (reg)
 		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_PASID_MATCH_REG, &reg, true);
+	amd_iommu_pc_set_reg(0, devid, bank, cntr,
+			     IOMMU_PC_PASID_MATCH_REG, &reg);
 
 	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
 	if (reg)
 		reg |= (1UL << 31);
-	amd_iommu_pc_get_set_reg_val(devid,
-			_GET_BANK(ev), _GET_CNTR(ev) ,
-			 IOMMU_PC_DOMID_MATCH_REG, &reg, true);
+	amd_iommu_pc_set_reg(0, devid, 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_DEVID(event), _GET_BANK(event),
+			     _GET_CNTR(event), IOMMU_PC_COUNTER_SRC_REG, &reg);
 }
 
 static void perf_iommu_start(struct perf_event *event, int flags)
 {
+	u64 val;
 	struct hw_perf_event *hwc = &event->hw;
 
 	pr_debug("perf: amd_iommu:perf_iommu_start\n");
@@ -297,13 +295,13 @@ 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_get_set_reg_val(_GET_DEVID(event),
-				_GET_BANK(event), _GET_CNTR(event),
-				IOMMU_PC_COUNTER_REG, &prev_raw_count, true);
-	}
+	if (!(flags & PERF_EF_RELOAD))
+		goto enable;
+
+	val = local64_read(&hwc->prev_count);
 
+	amd_iommu_pc_set_counter(0, _GET_BANK(event), _GET_CNTR(event), &val);
+enable:
 	perf_iommu_enable_event(event);
 	perf_event_update_userpage(event);
 
@@ -316,9 +314,8 @@ static void perf_iommu_read(struct perf_event *event)
 	struct hw_perf_event *hwc = &event->hw;
 	pr_debug("perf: amd_iommu:perf_iommu_read\n");
 
-	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
-				_GET_BANK(event), _GET_CNTR(event),
-				IOMMU_PC_COUNTER_REG, &cnt, false);
+	if (amd_iommu_pc_get_counter(0, _GET_BANK(event), _GET_CNTR(event), &cnt))
+		return;
 
 	/* IOMMU pc counter register is only 48 bits */
 	cnt &= GENMASK_ULL(48, 0);
diff --git a/arch/x86/events/amd/iommu.h b/arch/x86/events/amd/iommu.h
index 432d867..342716e 100644
--- a/arch/x86/events/amd/iommu.h
+++ b/arch/x86/events/amd/iommu.h
@@ -31,7 +31,11 @@
 
 extern u8 amd_iommu_pc_get_max_counters(uint 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(uint idx, u16 devid, u8 bank, u8 cntr,
+			 u8 fxn, u64 *value);
+
+extern int amd_iommu_pc_set_counter(uint idx, u8 bank, u8 cntr, u64 *value);
+
+extern int amd_iommu_pc_get_counter(uint idx, u8 bank, u8 cntr, 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 a7e756b..c993c77 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -251,10 +251,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)
@@ -1474,6 +1470,8 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	return 0;
 }
 
+static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
+				u8 fxn, u64 *value, bool is_write);
 
 static void init_iommu_perf_ctr(struct amd_iommu *iommu)
 {
@@ -1485,8 +1483,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_get_set_reg(iommu, 0, 0, 0, &val, true) != 0) ||
+	    (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false) != 0) ||
 	    (val != val2)) {
 		pr_err("AMD-Vi: Unable to write to IOMMU perf counter.\n");
 		amd_iommu_pc_present = false;
@@ -2754,15 +2752,18 @@ u8 amd_iommu_pc_get_max_counters(uint 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 == NULL) || (fxn > 0x28) || (fxn & 7)))
 		return -ENODEV;
 
 	offset = (u32)(((0x40|bank) << 12) | (cntr << 8) | fxn);
@@ -2785,17 +2786,55 @@ 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_set_reg(uint idx, u16 devid, 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_set_reg(iommu, bank, cntr, fxn, value, true);
+}
+EXPORT_SYMBOL(amd_iommu_pc_set_reg);
+
+int amd_iommu_pc_set_counter(uint idx, u8 bank, u8 cntr, 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_get_set_reg(iommu, bank, cntr,
+				    IOMMU_PC_COUNTER_REG,
+				    value, true);
+}
+EXPORT_SYMBOL(amd_iommu_pc_set_counter);
+
+int amd_iommu_pc_get_counter(uint idx, u8 bank, u8 cntr, u64 *value)
+{
+	struct amd_iommu *iommu = get_amd_iommu(idx);
+	int ret;
+	u64 tmp;
+
+	if (!value || !iommu)
+		return -EINVAL;
+	/*
+	 * Here, we read the specified counters on all IOMMUs,
+	 * which should have been programmed the same way and
+	 * aggregate the counter values.
+	 */
+
+	ret = iommu_pc_get_set_reg(iommu, bank, cntr,
+				   IOMMU_PC_COUNTER_REG,
+				   &tmp, false);
+	if (ret)
+		return ret;
+
+	/* IOMMU pc counter register is only 48 bits */
+	*value = tmp & GENMASK_ULL(48, 0);
+
+	return 0;
 }
+EXPORT_SYMBOL(amd_iommu_pc_get_counter);
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index 60f2eef..7186c63 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -56,10 +56,9 @@ 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);
+#ifndef IOMMU_PC_COUNTER_REG
+#define IOMMU_PC_COUNTER_REG			0x00
+#endif
 
 #ifdef CONFIG_IRQ_REMAP
 extern int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
-- 
1.8.3.1

[PATCH v7 4/7] perf/amd/iommu: Declare pr_fmt and remove unnecessary pr_debug

[Suravee Suthikulpanit]

This patch declare pr_fmt for perf/amd_iommu and remove unnecessary
pr_debug.

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 | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 9744dc8..9bff41d 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>
@@ -288,7 +290,6 @@ static void perf_iommu_start(struct perf_event *event, int flags)
 	u64 val;
 	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;
 
@@ -312,7 +313,6 @@ static void perf_iommu_read(struct perf_event *event)
 	u64 cnt, prev;
 	s64 delta;
 	struct hw_perf_event *hwc = &event->hw;
-	pr_debug("perf: amd_iommu:perf_iommu_read\n");
 
 	if (amd_iommu_pc_get_counter(0, _GET_BANK(event), _GET_CNTR(event), &cnt))
 		return;
@@ -339,8 +339,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;
 
@@ -362,7 +360,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 */
@@ -383,7 +380,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 */
@@ -443,7 +439,7 @@ static __init int _init_perf_amd_iommu(
 
 	/* Init events attributes */
 	if (_init_events_attrs(perf_iommu) != 0)
-		pr_err("perf: amd_iommu: Only support raw events.\n");
+		pr_err("Only support raw events.\n");
 
 	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
 	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
@@ -456,7 +452,7 @@ static __init int _init_perf_amd_iommu(
 
 	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
 	if (ret) {
-		pr_err("perf: amd_iommu: Failed to initialized.\n");
+		pr_err("Error initializing AMD IOMMU perf counters.\n");
 		amd_iommu_pc_exit();
 	} else {
 		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
-- 
1.8.3.1

[PATCH v7 5/7] perf/amd/iommu: Clean up perf_iommu_enable_event

[Suravee Suthikulpanit]

This patch cleans 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 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 9bff41d..2403c78 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -258,21 +258,21 @@ static void perf_iommu_enable_event(struct perf_event *ev)
 	amd_iommu_pc_set_reg(0, devid, bank, cntr,
 			     IOMMU_PC_COUNTER_SRC_REG, &reg);
 
-	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);
+	reg = devid | (_GET_DEVID_MASK(ev) << 32);
 	if (reg)
-		reg |= (1UL << 31);
+		reg |= BIT(31);
 	amd_iommu_pc_set_reg(0, devid, bank, cntr,
 			     IOMMU_PC_DEVID_MATCH_REG, &reg);
 
-	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
+	reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
 	if (reg)
-		reg |= (1UL << 31);
+		reg |= BIT(31);
 	amd_iommu_pc_set_reg(0, devid, bank, cntr,
 			     IOMMU_PC_PASID_MATCH_REG, &reg);
 
-	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
+	reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
 	if (reg)
-		reg |= (1UL << 31);
+		reg |= BIT(31);
 	amd_iommu_pc_set_reg(0, devid, bank, cntr,
 			     IOMMU_PC_DOMID_MATCH_REG, &reg);
 }
-- 
1.8.3.1

[PATCH v7 6/7] iommu/amd: Introduce amd_iommu_get_num_iommus()

[Suravee Suthikulpanit]

This patch introduces amd_iommu_get_num_iommus(). This is intended for
Perf AMD IOMMU driver.

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_init.c | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/amd/iommu.h b/arch/x86/events/amd/iommu.h
index 342716e..381f1c4 100644
--- a/arch/x86/events/amd/iommu.h
+++ b/arch/x86/events/amd/iommu.h
@@ -25,6 +25,8 @@
 #define PC_MAX_SPEC_CNTRS			16
 
 /* amd_iommu_init.c external support functions */
+extern int amd_iommu_get_num_iommus(void);
+
 extern bool amd_iommu_pc_supported(void);
 
 extern u8 amd_iommu_pc_get_max_banks(uint idx);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index c993c77..c3740cb 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1329,7 +1329,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");
@@ -2717,6 +2717,11 @@ static struct amd_iommu *get_amd_iommu(uint idx)
 	return iommu;
 }
 
+int amd_iommu_get_num_iommus(void)
+{
+	return amd_iommus_present;
+}
+
 /****************************************************************************
  *
  * IOMMU EFR Performance Counter support functionality. This code allows
-- 
1.8.3.1

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

[Suravee Suthikulpanit]

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

  /sys/device/amd_iommu_x     /* where x is the IOMMU index. */

This allows users to specify different events to be programed
onto performance counters of each IOMMU.

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

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 2403c78..5fd97b5 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -35,10 +35,13 @@
 #define _GET_PASID_MASK(ev) ((ev->hw.extra_reg.config >> 16) & 0xFFFFULL)
 #define _GET_DOMID_MASK(ev) ((ev->hw.extra_reg.config >> 32) & 0xFFFFULL)
 
-static struct perf_amd_iommu __perf_iommu;
+#define PERF_AMD_IOMMU_NAME_SZ 16
 
 struct perf_amd_iommu {
+	struct list_head list;
 	struct pmu pmu;
+	uint idx;
+	char name[PERF_AMD_IOMMU_NAME_SZ];
 	u8 max_banks;
 	u8 max_counters;
 	u64 cntr_assign_mask;
@@ -46,6 +49,8 @@ struct perf_amd_iommu {
 	const struct attribute_group *attr_groups[4];
 };
 
+LIST_HEAD(perf_amd_iommu_list);
+
 #define format_group	attr_groups[0]
 #define cpumask_group	attr_groups[1]
 #define events_group	attr_groups[2]
@@ -204,8 +209,7 @@ static int clear_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu,
 static int perf_iommu_event_init(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
-	struct perf_amd_iommu *perf_iommu;
-	u64 config, config1;
+	struct perf_amd_iommu *pi = container_of(event->pmu, struct perf_amd_iommu, pmu);
 
 	/* test the event attr type check for PMU enumeration */
 	if (event->attr.type != event->pmu->type)
@@ -227,27 +231,17 @@ static int perf_iommu_event_init(struct perf_event *event)
 	if (event->cpu < 0)
 		return -EINVAL;
 
-	perf_iommu = &__perf_iommu;
-
-	if (event->pmu != &perf_iommu->pmu)
-		return -ENOENT;
-
-	if (perf_iommu) {
-		config = event->attr.config;
-		config1 = event->attr.config1;
-	} else {
-		return -EINVAL;
-	}
-
 	/* update the hw_perf_event struct with the iommu config data */
-	hwc->config = config;
-	hwc->extra_reg.config = config1;
+	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);
@@ -255,33 +249,34 @@ static void perf_iommu_enable_event(struct perf_event *ev)
 	u64 reg = 0ULL;
 
 	reg = csource;
-	amd_iommu_pc_set_reg(0, devid, bank, cntr,
+	amd_iommu_pc_set_reg(hwc->idx, devid, 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, devid, bank, cntr,
+	amd_iommu_pc_set_reg(hwc->idx, devid, 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, devid, bank, cntr,
+	amd_iommu_pc_set_reg(hwc->idx, devid, 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, devid, bank, cntr,
+	amd_iommu_pc_set_reg(hwc->idx, devid, 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_DEVID(event), _GET_BANK(event),
+	amd_iommu_pc_set_reg(hwc->idx, _GET_DEVID(event), _GET_BANK(event),
 			     _GET_CNTR(event), IOMMU_PC_COUNTER_SRC_REG, &reg);
 }
 
@@ -301,7 +296,7 @@ static void perf_iommu_start(struct perf_event *event, int flags)
 
 	val = local64_read(&hwc->prev_count);
 
-	amd_iommu_pc_set_counter(0, _GET_BANK(event), _GET_CNTR(event), &val);
+	amd_iommu_pc_set_counter(hwc->idx, _GET_BANK(event), _GET_CNTR(event), &val);
 enable:
 	perf_iommu_enable_event(event);
 	perf_event_update_userpage(event);
@@ -314,7 +309,7 @@ static void perf_iommu_read(struct perf_event *event)
 	s64 delta;
 	struct hw_perf_event *hwc = &event->hw;
 
-	if (amd_iommu_pc_get_counter(0, _GET_BANK(event), _GET_CNTR(event), &cnt))
+	if (amd_iommu_pc_get_counter(hwc->idx, _GET_BANK(event), _GET_CNTR(event), &cnt))
 		return;
 
 	/* IOMMU pc counter register is only 48 bits */
@@ -417,14 +412,20 @@ static __init int _init_events_attrs(struct perf_amd_iommu *perf_iommu)
 
 static __init void amd_iommu_pc_exit(void)
 {
-	if (__perf_iommu.events_group != NULL) {
-		kfree(__perf_iommu.events_group);
-		__perf_iommu.events_group = NULL;
+	struct perf_amd_iommu *pi, *next;
+
+	list_for_each_entry_safe(pi, next, &perf_amd_iommu_list, list) {
+		list_del(&pi->list);
+
+		kfree(pi->events_group);
+		pi->events_group = NULL;
+
+		kfree(pi);
 	}
 }
 
-static __init int _init_perf_amd_iommu(
-	struct perf_amd_iommu *perf_iommu, char *name)
+static __init int
+init_one_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, uint idx)
 {
 	int ret;
 
@@ -441,54 +442,62 @@ static __init int _init_perf_amd_iommu(
 	if (_init_events_attrs(perf_iommu) != 0)
 		pr_err("Only support raw events.\n");
 
-	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
-	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
+	snprintf(perf_iommu->name, PERF_AMD_IOMMU_NAME_SZ, "amd_iommu_%u", idx);
+	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;
 
 	/* Init null attributes */
 	perf_iommu->null_group = NULL;
+
+	/* Setting up PMU */
+	perf_iommu->pmu.event_init = perf_iommu_event_init,
+	perf_iommu->pmu.add = perf_iommu_add,
+	perf_iommu->pmu.del = perf_iommu_del,
+	perf_iommu->pmu.start = perf_iommu_start,
+	perf_iommu->pmu.stop = perf_iommu_stop,
+	perf_iommu->pmu.read = perf_iommu_read,
+	perf_iommu->pmu.task_ctx_nr = perf_invalid_context;
 	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
 
-	ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
+	ret = perf_pmu_register(&perf_iommu->pmu, perf_iommu->name, -1);
 	if (ret) {
 		pr_err("Error initializing AMD IOMMU perf counters.\n");
 		amd_iommu_pc_exit();
 	} else {
-		pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n",
-			amd_iommu_pc_get_max_banks(0),
-			amd_iommu_pc_get_max_counters(0));
+		pr_info("Detected %s, w/ %d banks, %d counters/bank\n",
+			perf_iommu->name,
+			amd_iommu_pc_get_max_banks(idx),
+			amd_iommu_pc_get_max_counters(idx));
+
+		list_add_tail(&perf_iommu->list, &perf_amd_iommu_list);
 	}
 
 	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,
-	},
-	.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)
 {
+	uint i;
+
 	/* Make sure the IOMMU PC resource is available */
 	if (!amd_iommu_pc_supported())
 		return -ENODEV;
 
-	_init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
+	for (i = 0 ; i < amd_iommu_get_num_iommus(); i++) {
+		int ret;
+		struct perf_amd_iommu *pi;
+
+		pi = kzalloc(sizeof(struct perf_amd_iommu), GFP_KERNEL);
+		if (!pi)
+			return -ENOMEM;
+
+		ret = init_one_perf_amd_iommu(pi, i);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
1.8.3.1

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

[Joerg Roedel]

On Mon, Jan 09, 2017 at 09:33:42PM -0600, Suthikulpanit, Suravee wrote:
> +static struct amd_iommu *get_amd_iommu(uint idx)
> +{
> +	uint i = 0;
> +	struct amd_iommu *iommu = NULL;
> +
> +	for_each_iommu(iommu) {
> +		if (i == idx)
> +			break;
> +		i++;
> +	}
> +	return iommu;
> +}

Sorry for missing that in the last review, but this function returns
just the last iommu in the list when there are less iommus than the
requested index.

Is that intentional? The following code checks for !iommu, so I guess
not. It should look more like this then:

	static struct amd_iommu *get_amd_iommu(uint idx)
	{
		uint i = 0;
		struct amd_iommu *iommu, *ret = NULL;

		for_each_iommu(iommu)
			if (i++ == idx) {
				ret = iommu;
				break;
			}
		return ret;
	}


Regards,

	Joerg

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

[Suravee Suthikulpanit]

On 1/10/17 21:43, Joerg Roedel wrote:
> On Mon, Jan 09, 2017 at 09:33:42PM -0600, Suthikulpanit, Suravee wrote:
>> +static struct amd_iommu *get_amd_iommu(uint idx)
>> +{
>> +	uint i = 0;
>> +	struct amd_iommu *iommu = NULL;
>> +
>> +	for_each_iommu(iommu) {
>> +		if (i == idx)
>> +			break;
>> +		i++;
>> +	}
>> +	return iommu;
>> +}
>
> Sorry for missing that in the last review, but this function returns
> just the last iommu in the list when there are less iommus than the
> requested index.
>
> Is that intentional? The following code checks for !iommu, so I guess
> not. It should look more like this then:
>
> 	static struct amd_iommu *get_amd_iommu(uint idx)
> 	{
> 		uint i = 0;
> 		struct amd_iommu *iommu, *ret = NULL;
>
> 		for_each_iommu(iommu)
> 			if (i++ == idx) {
> 				ret = iommu;
> 				break;
> 			}
> 		return ret;
> 	}
>
>
> Regards,
>
> 	Joerg

Right.... Thanks for catching this. Do you want me to send out V8 with 
this fix?

Suravee

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

[Boris Petkov]

On January 11, 2017 4:03:50 AM GMT+01:00, Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> wrote:
>
>Right.... Thanks for catching this. Do you want me to send out V8 with 
>this fix?
Send only the corrected version of this patch please, as a reply to this message. 

Thanks.
-- 
Sent from a small device: formatting sux and brevity is inevitable. 

Re: [PATCH v7 2/7] 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>
---

NOTE: This contains the fix in get_amd_iommu() as suggested by Joerg.

  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 f387baf..cf94f48 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -237,14 +237,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;
@@ -456,6 +448,11 @@ static __init int _init_perf_amd_iommu(
  	if (_init_events_attrs(perf_iommu) != 0)
  		pr_err("perf: amd_iommu: Only support raw events.\n");

+	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
+	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
+	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
+		return -EINVAL;
+
  	/* Init null attributes */
  	perf_iommu->null_group = NULL;
  	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
@@ -466,8 +463,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 845d173..432d867 100644
--- a/arch/x86/events/amd/iommu.h
+++ b/arch/x86/events/amd/iommu.h
@@ -24,15 +24,12 @@
  #define PC_MAX_SPEC_BNKS			64
  #define PC_MAX_SPEC_CNTRS			16

-/* iommu pc reg masks*/
-#define IOMMU_BASE_DEVID			0x0000
-
  /* amd_iommu_init.c external support functions */
  extern bool amd_iommu_pc_supported(void);

-extern u8 amd_iommu_pc_get_max_banks(u16 devid);
+extern u8 amd_iommu_pc_get_max_banks(uint idx);

-extern u8 amd_iommu_pc_get_max_counters(u16 devid);
+extern u8 amd_iommu_pc_get_max_counters(uint 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 157e934..1a13c34 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2706,6 +2706,20 @@ bool amd_iommu_v2_supported(void)
  }
  EXPORT_SYMBOL(amd_iommu_v2_supported);

+static struct amd_iommu *get_amd_iommu(uint idx)
+{
+	uint 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
@@ -2713,17 +2727,14 @@ bool amd_iommu_v2_supported(void)
   *
   ****************************************************************************/

-u8 amd_iommu_pc_get_max_banks(u16 devid)
+u8 amd_iommu_pc_get_max_banks(uint 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);

@@ -2733,17 +2744,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(uint 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 7eb60c1..60f2eef 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -58,8 +58,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

Re: [PATCH v7 1/7] perf/amd/iommu: Misc fix up perf_iommu_read

[Borislav Petkov]

On Mon, Jan 09, 2017 at 09:33:41PM -0600, Suravee Suthikulpanit wrote:
> This patch contains the following minor fixup:

For the future: never say "this patch" in the commit message - just
explain *why* the patch is needed.

>   * Fixed overflow handling since u64 delta would lose the MSB sign bit.

Past tense also reads funnily: "Fix overflow handling ..." sounds much
better.

>   * Remove unnecessary local64_set().

But you don't remove it - you add it. Huh?

>   * 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 | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index b28200d..f387baf 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -319,29 +319,30 @@ 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 cnt, prev;

"count" was fine.

> +	s64 delta;
>  	struct hw_perf_event *hwc = &event->hw;
>  	pr_debug("perf: amd_iommu:perf_iommu_read\n");

...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v7 1/7] perf/amd/iommu: Misc fix up perf_iommu_read

[Peter Zijlstra]

On Mon, Jan 09, 2017 at 09:33:41PM -0600, Suravee Suthikulpanit wrote:
> This patch contains the following minor fixup:
>   * Fixed overflow handling since u64 delta would lose the MSB sign bit.

Please explain.. afaict this actually introduces a bug.


> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index b28200d..f387baf 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -319,29 +319,30 @@ 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 cnt, prev;
> +	s64 delta;
>  	struct hw_perf_event *hwc = &event->hw;
>  	pr_debug("perf: amd_iommu:perf_iommu_read\n");
>  
>  	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
>  				_GET_BANK(event), _GET_CNTR(event),
> -				IOMMU_PC_COUNTER_REG, &count, false);
> +				IOMMU_PC_COUNTER_REG, &cnt, false);
>  
>  	/* IOMMU pc counter register is only 48 bits */
> -	count &= 0xFFFFFFFFFFFFULL;
> +	cnt &= 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.
> +	 */

So you cannot group this event with a software event that reads this
from their sample?

> +	local64_set(&hwc->prev_count, cnt);
> +
> +	/* Handle 48-bit counter overflow */
> +	delta = (cnt << 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 v7 3/7] perf/amd/iommu: Modify IOMMU API to allow specifying IOMMU index

[Borislav Petkov]

On Mon, Jan 09, 2017 at 09:33:43PM -0600, Suravee Suthikulpanit wrote:
> The current amd_iommu_pc_get_set_reg_val() cannot support multi-IOMMU.

							multiple IOMMUs.

> It is also confusing since it is trying to support set and get in
> one function.
> 
> So, this patch breaks it down to amd_iommu_pc_[get|set]_counter(),

"So break it down to..."

> and modifies them to allow callers to specify IOMMU index. This prepares
> the driver for supporting multi-IOMMU in subsequent patch.
> 
> This patch also removes unnecessary function declarations in

"Also, remove unnecessary ..."

> 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     | 45 +++++++++++-------------
>  arch/x86/events/amd/iommu.h     |  8 +++--
>  drivers/iommu/amd_iommu_init.c  | 77 +++++++++++++++++++++++++++++++----------
>  drivers/iommu/amd_iommu_proto.h |  7 ++--
>  4 files changed, 88 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index cf94f48..9744dc8 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -248,46 +248,44 @@ 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, devid, bank, cntr,
> +			     IOMMU_PC_COUNTER_SRC_REG, &reg);
>  
>  	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);

What are those 0ULL being ORed in? This is to do what exactly?

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

		reg |= BIT(31);

You can do conversion in a prepatch though.

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

...

> diff --git a/arch/x86/events/amd/iommu.h b/arch/x86/events/amd/iommu.h
> index 432d867..342716e 100644
> --- a/arch/x86/events/amd/iommu.h
> +++ b/arch/x86/events/amd/iommu.h
> @@ -31,7 +31,11 @@
>  
>  extern u8 amd_iommu_pc_get_max_counters(uint 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(uint idx, u16 devid, u8 bank, u8 cntr,
> +			 u8 fxn, u64 *value);

Align args at opening brace.

> +
> +extern int amd_iommu_pc_set_counter(uint idx, u8 bank, u8 cntr, u64 *value);
> +
> +extern int amd_iommu_pc_get_counter(uint idx, u8 bank, u8 cntr, 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 a7e756b..c993c77 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -251,10 +251,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)
> @@ -1474,6 +1470,8 @@ static int __init init_iommu_all(struct acpi_table_header *table)
>  	return 0;
>  }
>  
> +static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
> +				u8 fxn, u64 *value, bool is_write);

Can you break that one too pls? In a separate patch.

>  
>  static void init_iommu_perf_ctr(struct amd_iommu *iommu)
>  {
> @@ -1485,8 +1483,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_get_set_reg(iommu, 0, 0, 0, &val, true) != 0) ||
> +	    (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false) != 0) ||

	if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true) ||
	    iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false))

is more readable.

>  	    (val != val2)) {
>  		pr_err("AMD-Vi: Unable to write to IOMMU perf counter.\n");
>  		amd_iommu_pc_present = false;
> @@ -2754,15 +2752,18 @@ u8 amd_iommu_pc_get_max_counters(uint 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 == NULL) || (fxn > 0x28) || (fxn & 7)))

	    WARN_ON(!iommu || ...

>  		return -ENODEV;
>  
>  	offset = (u32)(((0x40|bank) << 12) | (cntr << 8) | fxn);
> @@ -2785,17 +2786,55 @@ 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_set_reg(uint idx, u16 devid, 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_set_reg(iommu, bank, cntr, fxn, value, true);
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_set_reg);
> +
> +int amd_iommu_pc_set_counter(uint idx, u8 bank, u8 cntr, 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_get_set_reg(iommu, bank, cntr,
> +				    IOMMU_PC_COUNTER_REG,
> +				    value, true);

Why isn't this masking out [63:49] like below?

> +}
> +EXPORT_SYMBOL(amd_iommu_pc_set_counter);
> +
> +int amd_iommu_pc_get_counter(uint idx, u8 bank, u8 cntr, u64 *value)
> +{
> +	struct amd_iommu *iommu = get_amd_iommu(idx);
> +	int ret;
> +	u64 tmp;
> +
> +	if (!value || !iommu)
> +		return -EINVAL;
> +	/*
> +	 * Here, we read the specified counters on all IOMMUs,
> +	 * which should have been programmed the same way and
> +	 * aggregate the counter values.
> +	 */
> +
> +	ret = iommu_pc_get_set_reg(iommu, bank, cntr,
> +				   IOMMU_PC_COUNTER_REG,
> +				   &tmp, false);
> +	if (ret)
> +		return ret;
> +
> +	/* IOMMU pc counter register is only 48 bits */
> +	*value = tmp & GENMASK_ULL(48, 0);
> +
> +	return 0;
>  }
> +EXPORT_SYMBOL(amd_iommu_pc_get_counter);
> diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
> index 60f2eef..7186c63 100644
> --- a/drivers/iommu/amd_iommu_proto.h
> +++ b/drivers/iommu/amd_iommu_proto.h
> @@ -56,10 +56,9 @@ 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);
> +#ifndef IOMMU_PC_COUNTER_REG

What's that for?

> +#define IOMMU_PC_COUNTER_REG			0x00
> +#endif
>  
>  #ifdef CONFIG_IRQ_REMAP
>  extern int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
> -- 
> 1.8.3.1
> 
> 

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v7 4/7] perf/amd/iommu: Declare pr_fmt and remove unnecessary pr_debug

[Borislav Petkov]

On Mon, Jan 09, 2017 at 09:33:44PM -0600, Suravee Suthikulpanit wrote:
> This patch declare pr_fmt for perf/amd_iommu and remove unnecessary

There's that "This patch" again.

> pr_debug.
> 
> 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 | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)

...

> @@ -362,7 +360,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 */
> @@ -383,7 +380,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 */
> @@ -443,7 +439,7 @@ static __init int _init_perf_amd_iommu(

That function definition is real ugly, please change it to something
more like this:

static __init int
_init_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, char *name)
{

>  
>  	/* Init events attributes */

This and all those "Init" comments are useless.

>  	if (_init_events_attrs(perf_iommu) != 0)

	if (_init_events_attrs(perf_iommu))

is better.

> -		pr_err("perf: amd_iommu: Only support raw events.\n");
> +		pr_err("Only support raw events.\n");

What does that mean? I'm wearing my user hat right now and looking at
dmesg and that appears and I'm wondering what this means.

IOW, please put yourself in the user's shoes and read those messages
we're issuing and try to imagine whether they make sense or could be
improved.

Also, looking at that driver more, this needs to die, like now:

#define format_group    attr_groups[0]
#define cpumask_group   attr_groups[1]
#define events_group    attr_groups[2]
#define null_group      attr_groups[3]

Like, kill it dead. Define a separate array, look what the other drivers
do, whatever, but this is too ugly to live.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v7 5/7] perf/amd/iommu: Clean up perf_iommu_enable_event

[Borislav Petkov]

On Mon, Jan 09, 2017 at 09:33:45PM -0600, Suravee Suthikulpanit wrote:
> This patch cleans 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 | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 9bff41d..2403c78 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -258,21 +258,21 @@ static void perf_iommu_enable_event(struct perf_event *ev)
>  	amd_iommu_pc_set_reg(0, devid, bank, cntr,
>  			     IOMMU_PC_COUNTER_SRC_REG, &reg);
>  
> -	reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);
> +	reg = devid | (_GET_DEVID_MASK(ev) << 32);
>  	if (reg)
> -		reg |= (1UL << 31);
> +		reg |= BIT(31);
>  	amd_iommu_pc_set_reg(0, devid, bank, cntr,
>  			     IOMMU_PC_DEVID_MATCH_REG, &reg);
>  
> -	reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
> +	reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
>  	if (reg)
> -		reg |= (1UL << 31);
> +		reg |= BIT(31);
>  	amd_iommu_pc_set_reg(0, devid, bank, cntr,
>  			     IOMMU_PC_PASID_MATCH_REG, &reg);
>  
> -	reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
> +	reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
>  	if (reg)
> -		reg |= (1UL << 31);
> +		reg |= BIT(31);
>  	amd_iommu_pc_set_reg(0, devid, bank, cntr,
>  			     IOMMU_PC_DOMID_MATCH_REG, &reg);
>  }
> -- 

Ah ok, you're doing it here, good.

For the next version of the patchset, please reorder all cleanups first
and then the patches adding functional changes/new features.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v7 6/7] iommu/amd: Introduce amd_iommu_get_num_iommus()

[Borislav Petkov]

On Mon, Jan 09, 2017 at 09:33:46PM -0600, Suravee Suthikulpanit wrote:
> This patch introduces amd_iommu_get_num_iommus(). This is intended for

There's that "This patch" again... but you get the idea :)

> Perf AMD IOMMU driver.
> 
> 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_init.c | 7 ++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/amd/iommu.h b/arch/x86/events/amd/iommu.h
> index 342716e..381f1c4 100644
> --- a/arch/x86/events/amd/iommu.h
> +++ b/arch/x86/events/amd/iommu.h
> @@ -25,6 +25,8 @@
>  #define PC_MAX_SPEC_CNTRS			16
>  
>  /* amd_iommu_init.c external support functions */
> +extern int amd_iommu_get_num_iommus(void);
> +
>  extern bool amd_iommu_pc_supported(void);
>  
>  extern u8 amd_iommu_pc_get_max_banks(uint idx);
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index c993c77..c3740cb 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1329,7 +1329,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");
> @@ -2717,6 +2717,11 @@ static struct amd_iommu *get_amd_iommu(uint idx)
>  	return iommu;
>  }
>  
> +int amd_iommu_get_num_iommus(void)
> +{
> +	return amd_iommus_present;
> +}

So this is strange. This amd_iommus_present is used by other iommu code
but then you're adding a getter. IMO, it should be cleaner if *all* code
is converted to use the getter now and not the naked variable.

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

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

Straight into the top-level devices hierarchy?

Why not add a proper hierarchy to

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

?

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

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

This uint thing has crept in:

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

"unsigned int" please.

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

static

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

Do this deref when you actually need it:

	struct perf_amd_iommu *pi;

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

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

---> here:

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

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

Align vertically on the = sign.

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

...

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

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

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


Here's also

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

is that still correct for all IOMMUs?

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

Do this ^^

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


... here, after the test has passed.

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

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

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

Align vertically on the =

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

I'd prefer:

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

where the "#%d" is idx.

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

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

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

That'll be all for now. :-)

Thanks.

-- 
Regards/Gruss,
    Boris.

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

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

[Suravee Suthikulpanit]

Boris,

On 1/13/17 00:52, Borislav Petkov wrote:
> On Mon, Jan 09, 2017 at 09:33:47PM -0600, Suravee Suthikulpanit wrote:
>> This patch adds multi-IOMMU support for perf by exposing
>> an AMD IOMMU PMU for each IOMMU found in the system via:
>>
>>   /sys/device/amd_iommu_x     /* where x is the IOMMU index. */
> Straight into the top-level devices hierarchy?
>
> Why not add a proper hierarchy to
>
> /sys/devices/system/iommu/ ...
>
> ?
>
> Jörg, don't you have a sane iommu hierarchy already in sysfs?
>

IIUC, Perf tools looks at the /sys/devices/xxxxx to identify availalble PMUs.
Are you planning to have perf tools look at /sys/devices/system/iommu/xxx instead?

Suravee

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

[Borislav Petkov]

On Fri, Jan 13, 2017 at 05:24:01PM +0700, Suravee Suthikulpanit wrote:
> IIUC, Perf tools looks at the /sys/devices/xxxxx to identify
> availalble PMUs. Are you planning to have perf tools look at
> /sys/devices/system/iommu/xxx instead?

No, I'm planning to understand what do you mean exactly. Because the
PMUs are, as I'm being pointed to, here:

#define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/"

Do you mean that, per chance?

-- 
Regards/Gruss,
    Boris.

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

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

[Suravee Suthikulpanit]

On 1/13/17 18:49, Borislav Petkov wrote:
> On Fri, Jan 13, 2017 at 05:24:01PM +0700, Suravee Suthikulpanit wrote:
>> IIUC, Perf tools looks at the /sys/devices/xxxxx to identify
>> availalble PMUs. Are you planning to have perf tools look at
>> /sys/devices/system/iommu/xxx instead?
>
> No, I'm planning to understand what do you mean exactly. Because the
> PMUs are, as I'm being pointed to, here:
>
> #define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/"
>
> Do you mean that, per chance?
>

Ah, okay. I can see the following directory path:

# ls -l  /sys/devices/
drwxr-xr-x  5 root root 0 Jan 13 20:36 amd_iommu_0

# ls -l /sys/bus/event_source/devices/
lrwxrwxrwx 1 root root 0 Jan 13 20:33 amd_iommu_0 -> ../../../devices/amd_iommu_0

I'll update the commit log to mention /bus/event_source/devices/amd_iommu_X instead.

Thanks,
S

Re: [PATCH v7 4/7] perf/amd/iommu: Declare pr_fmt and remove unnecessary pr_debug

[Suravee Suthikulpanit]

On 1/12/17 17:19, Borislav Petkov wrote:
> Also, looking at that driver more, this needs to die, like now:
>
> #define format_group    attr_groups[0]
> #define cpumask_group   attr_groups[1]
> #define events_group    attr_groups[2]
> #define null_group      attr_groups[3]
>
> Like, kill it dead. Define a separate array, look what the other drivers
> do, whatever, but this is too ugly to live.
>
> Thanks.
>
> --

Okay... you got it :)

S

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

[Borislav Petkov]

On Sat, Jan 14, 2017 at 09:58:29AM +0700, Suravee Suthikulpanit wrote:
> I'll update the commit log to mention
> /bus/event_source/devices/amd_iommu_X instead.

Yes, so /sys/devices/ contains *all* devices on the system and the iommu
ones appear there too but since in the commit message you were talking
about PMUs but pointing at generic devices, it did confuse me.

But you're doing perf_pmu_register() anyway so the hierarchy will be fine.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v7 1/7] perf/amd/iommu: Misc fix up perf_iommu_read

[Suravee Suthikulpanit]

Peter,

On 1/11/17 18:57, Peter Zijlstra wrote:
> On Mon, Jan 09, 2017 at 09:33:41PM -0600, Suravee Suthikulpanit wrote:
>> This patch contains the following minor fixup:
>>   * Fixed overflow handling since u64 delta would lose the MSB sign bit.
>
> Please explain.. afaict this actually introduces a bug.

I'm changing the u64 to s64 ..... (see below)

>
>> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
>> index b28200d..f387baf 100644
>> --- a/arch/x86/events/amd/iommu.c
>> +++ b/arch/x86/events/amd/iommu.c
>> @@ -319,29 +319,30 @@ 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 cnt, prev;
>> +	s64 delta;

.... (here) because we had a discussion (https://lkml.org/lkml/2016/2/18/325),
and you suggested the following:

     Your overflow handling is broken, you want delta to be s64. Otherwise:

	delta >>= COUNTER_SHIFT;

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

>>  	struct hw_perf_event *hwc = &event->hw;
>>  	pr_debug("perf: amd_iommu:perf_iommu_read\n");
>>
>>  	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
>>  				_GET_BANK(event), _GET_CNTR(event),
>> -				IOMMU_PC_COUNTER_REG, &count, false);
>> +				IOMMU_PC_COUNTER_REG, &cnt, false);
>>
>>  	/* IOMMU pc counter register is only 48 bits */
>> -	count &= 0xFFFFFFFFFFFFULL;
>> +	cnt &= 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.
>> +	 */
>
> So you cannot group this event with a software event that reads this
> from their sample?

Not sure if I understand you point here. When you say sample, I assume you mean
the profiling mode used w/ perf record. These counters are not supported for
sampling mode. So, we only perf stat (i.e. counting mode).

Thanks,
Suravee

Re: [PATCH v7 1/7] perf/amd/iommu: Misc fix up perf_iommu_read

[Peter Zijlstra]

On Sun, Jan 15, 2017 at 09:36:10AM +0700, Suravee Suthikulpanit wrote:
> Peter,
> 
> On 1/11/17 18:57, Peter Zijlstra wrote:
> >On Mon, Jan 09, 2017 at 09:33:41PM -0600, Suravee Suthikulpanit wrote:
> >>This patch contains the following minor fixup:
> >>  * Fixed overflow handling since u64 delta would lose the MSB sign bit.
> >
> >Please explain.. afaict this actually introduces a bug.
> 
> I'm changing the u64 to s64 ..... (see below)
> 
> >
> >>diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> >>index b28200d..f387baf 100644
> >>--- a/arch/x86/events/amd/iommu.c
> >>+++ b/arch/x86/events/amd/iommu.c
> >>@@ -319,29 +319,30 @@ 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 cnt, prev;
> >>+	s64 delta;
> 
> .... (here) because we had a discussion (https://lkml.org/lkml/2016/2/18/325),
> and you suggested the following:
> 
>     Your overflow handling is broken, you want delta to be s64. Otherwise:
> 
> 	delta >>= COUNTER_SHIFT;
> 
>     ends up as a SHR and you loose the MSB sign bits.

Yah, I was wrong :-), please see:

  7f612a7f0bc1 ("perf/x86: Fix full width counter, counter overflow")


> >> 	struct hw_perf_event *hwc = &event->hw;
> >> 	pr_debug("perf: amd_iommu:perf_iommu_read\n");
> >>
> >> 	amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
> >> 				_GET_BANK(event), _GET_CNTR(event),
> >>-				IOMMU_PC_COUNTER_REG, &count, false);
> >>+				IOMMU_PC_COUNTER_REG, &cnt, false);
> >>
> >> 	/* IOMMU pc counter register is only 48 bits */
> >>-	count &= 0xFFFFFFFFFFFFULL;
> >>+	cnt &= 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.
> >>+	 */
> >
> >So you cannot group this event with a software event that reads this
> >from their sample?
> 
> Not sure if I understand you point here. When you say sample, I assume you mean
> the profiling mode used w/ perf record. These counters are not supported for
> sampling mode. So, we only perf stat (i.e. counting mode).

But you can group them with a software event, right? And then that
software event can do sampling (using a timer for instance) and read the
other counters in the group using PERF_SAMPLE_READ and
PERF_FORMAT_GROUP.