[PATCH v4 0/3] iommu/arm-smmu-v3: Parse out event records

[PATCH v4 0/3] iommu/arm-smmu-v3: Parse out event records

[Pranjal Shrivastava]

Enhance the arm-smmu-v3 driver to parse out useful information from
event records into a structure for better event handling & logging.

As requested by Nicolin, some sample logs powered by QEMU:

[   85.410290] arm-smmu-v3 arm-smmu-v3.0.auto: Unexpected event received: F_PERMISSION
[   85.410290]  master: 0000:00:01.0 sid: 0x00000008.0x00000
[   85.410290]  iova = 0xffffc0d0 ipa = 0x0
[   85.410290]  Unpriv | Data | Write | S1 | Input address caused fault
[   85.410290]  STAG = 0x0 Stall = 0x0
[   85.411681] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x13 received: master 0000:00:01.0:
[   85.412045] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000013
[   85.412347] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
[   85.412630] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000ffffc0d0
[   85.412921] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000

..and a sample log for devices with a good name:

[   43.405187] arm-smmu-v3 arm-smmu-v3.0.auto: Unexpected event received: F_TRANSLATION
[   43.405187]  master: igb sid: 0x00000008.0x00000
[   43.405187]  iova = 0xfffff040 ipa = 0x0
[   43.405187]  Unpriv | Data | Write | S1 | CD fetch
[   43.405187]  STAG = 0x0 Stall = 0x0
[   43.406302] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x10 received: master igb:
[   43.406622] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000010
[   43.406875] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[   43.407131] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
[   43.407397] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000

v4
 * Re-arranged the series to first introduce struct arm_smmu_event
 * Improved the complex ternary expression that prints TTRnW info
 * Added consistent spacing to the logs & resized log strings
 * Moved ratelimiting within `arm_smmu_dump_event`
 * Refactored master_name printing by getting a ref to the device
 * Refactored `arm_smmu_handle_evt` to avoid redundant master lookup

v3
 https://lore.kernel.org/all/20240928005143.2378938-1-praan@google.com/
 * Fixed a potential race and null pointer deref for arm_smmu_master
 * Improved the logging approach by using multiple strings
 * Added logs for STAG & STALL fields for relevant events
 * Invoked the log function within `arm_smmu_handle_evt` routine
 * Rebased the changes

v2
 https://lore.kernel.org/linux-iommu/20240827193026.3993039-1-praan@google.com/
 * Addressed review comments
 * Introduced `struct arm_smmu_event` to hold relevant event fields
 * Broke out helper functions to populate & dump event info
 * Modified the event handler routines to use `struct arm_smmu_event`

v1
 https://lore.kernel.org/linux-iommu/20240816211722.1404070-1-praan@google.com/


Pranjal Shrivastava (3):
  iommu/arm-smmu-v3: Introduce struct arm_smmu_event
  iommu/arm-smmu-v3: Log better event records
  iommu/arm-smmu-v3: Avoid redundant master lookup in events

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 174 ++++++++++++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  33 ++++
 2 files changed, 172 insertions(+), 35 deletions(-)

-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

Introduce `struct arm_smmu_event` to represent event records.
Parse out relevant fields from raw event records for ease and
use the new `struct arm_smmu_event` instead.

Signed-off-by: Daniel Mentz <danielmentz@google.com>
Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 59 ++++++++++++++-------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 20 +++++++
 2 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 737c5b882355..2f1108e5de51 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1757,17 +1757,16 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
 }
 
 /* IRQ and event handlers */
-static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
+static int arm_smmu_handle_evt(struct arm_smmu_event *event)
 {
 	int ret = 0;
 	u32 perm = 0;
 	struct arm_smmu_master *master;
-	bool ssid_valid = evt[0] & EVTQ_0_SSV;
-	u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
 	struct iopf_fault fault_evt = { };
+	struct arm_smmu_device *smmu = event->smmu;
 	struct iommu_fault *flt = &fault_evt.fault;
 
-	switch (FIELD_GET(EVTQ_0_ID, evt[0])) {
+	switch (event->id) {
 	case EVT_ID_TRANSLATION_FAULT:
 	case EVT_ID_ADDR_SIZE_FAULT:
 	case EVT_ID_ACCESS_FAULT:
@@ -1777,35 +1776,35 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
 		return -EOPNOTSUPP;
 	}
 
-	if (!(evt[1] & EVTQ_1_STALL))
+	if (!event->stall)
 		return -EOPNOTSUPP;
 
-	if (evt[1] & EVTQ_1_RnW)
+	if (event->read)
 		perm |= IOMMU_FAULT_PERM_READ;
 	else
 		perm |= IOMMU_FAULT_PERM_WRITE;
 
-	if (evt[1] & EVTQ_1_InD)
+	if (event->instruction)
 		perm |= IOMMU_FAULT_PERM_EXEC;
 
-	if (evt[1] & EVTQ_1_PnU)
+	if (event->privileged)
 		perm |= IOMMU_FAULT_PERM_PRIV;
 
 	flt->type = IOMMU_FAULT_PAGE_REQ;
 	flt->prm = (struct iommu_fault_page_request) {
 		.flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
-		.grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
+		.grpid = event->stag,
 		.perm = perm,
-		.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
+		.addr = event->iova,
 	};
 
-	if (ssid_valid) {
+	if (event->ssid_valid) {
 		flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
-		flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
+		flt->prm.pasid = event->ssid;
 	}
 
 	mutex_lock(&smmu->streams_mutex);
-	master = arm_smmu_find_master(smmu, sid);
+	master = arm_smmu_find_master(smmu, event->sid);
 	if (!master) {
 		ret = -EINVAL;
 		goto out_unlock;
@@ -1817,28 +1816,48 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
 	return ret;
 }
 
+static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
+					struct arm_smmu_event *event)
+{
+	/* Pick out the good stuff */
+	event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
+	event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
+	event->ssid_valid = event->raw[0] & EVTQ_0_SSV;
+	event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
+	event->privileged = FIELD_GET(EVTQ_1_PnU, event->raw[1]);
+	event->instruction = FIELD_GET(EVTQ_1_InD, event->raw[1]);
+	event->s2 = FIELD_GET(EVTQ_1_S2, event->raw[1]);
+	event->read = FIELD_GET(EVTQ_1_RnW, event->raw[1]);
+	event->stag = FIELD_GET(EVTQ_1_STAG, event->raw[1]);
+	event->stall = event->raw[1] & EVTQ_1_STALL;
+	event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
+	event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
+	event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
+	event->smmu = smmu;
+}
+
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
 	int i, ret;
+	struct arm_smmu_event evt;
 	struct arm_smmu_device *smmu = dev;
 	struct arm_smmu_queue *q = &smmu->evtq.q;
 	struct arm_smmu_ll_queue *llq = &q->llq;
 	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
 				      DEFAULT_RATELIMIT_BURST);
-	u64 evt[EVTQ_ENT_DWORDS];
 
 	do {
-		while (!queue_remove_raw(q, evt)) {
-			u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
+		while (!queue_remove_raw(q, evt.raw)) {
 
-			ret = arm_smmu_handle_evt(smmu, evt);
+			arm_smmu_get_event_from_raw(smmu, &evt);
+			ret = arm_smmu_handle_evt(&evt);
 			if (!ret || !__ratelimit(&rs))
 				continue;
 
-			dev_info(smmu->dev, "event 0x%02x received:\n", id);
-			for (i = 0; i < ARRAY_SIZE(evt); ++i)
+			dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
+			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
 				dev_info(smmu->dev, "\t0x%016llx\n",
-					 (unsigned long long)evt[i]);
+					(unsigned long long)evt.raw[i]);
 
 			cond_resched();
 		}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 1e9952ca989f..8a42d7b701fb 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -437,6 +437,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
 
 #define EVTQ_0_ID			GENMASK_ULL(7, 0)
 
+/* Events */
 #define EVT_ID_TRANSLATION_FAULT	0x10
 #define EVT_ID_ADDR_SIZE_FAULT		0x11
 #define EVT_ID_ACCESS_FAULT		0x12
@@ -452,6 +453,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
 #define EVTQ_1_RnW			(1UL << 35)
 #define EVTQ_1_S2			(1UL << 39)
 #define EVTQ_1_CLASS			GENMASK_ULL(41, 40)
+#define EVTQ_1_CLASS_TT			0x1
 #define EVTQ_1_TT_READ			(1UL << 44)
 #define EVTQ_2_ADDR			GENMASK_ULL(63, 0)
 #define EVTQ_3_IPA			GENMASK_ULL(51, 12)
@@ -771,6 +773,24 @@ struct arm_smmu_stream {
 	struct rb_node			node;
 };
 
+struct arm_smmu_event {
+	struct arm_smmu_device		*smmu;
+	u8				id;
+	u8				class;
+	u16				stag;
+	u32				sid;
+	u32				ssid;
+	u64				iova;
+	u64				ipa;
+	u64				raw[EVTQ_ENT_DWORDS];
+	bool				stall;
+	bool				ssid_valid;
+	bool				privileged;
+	bool				instruction;
+	bool				s2;
+	bool				read;
+};
+
 /* SMMU private data for each master */
 struct arm_smmu_master {
 	struct arm_smmu_device		*smmu;
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Pranjal Shrivastava]

Currently, the driver dumps the raw hex for a received event record.
Improve this by leveraging `struct arm_smmu_event` for event fields
and log human-readable event records with meaningful information.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 109 ++++++++++++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  13 +++
 2 files changed, 113 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 2f1108e5de51..4477cf86cb8e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -83,6 +83,34 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ 0, NULL},
 };
 
+static const char * const event_str[] = {
+	/* Bad config events */
+	[EVT_ID_BAD_SID_CONFIG] = "C_BAD_STREAMID",
+	[EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
+	[EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
+	[EVT_ID_BAD_SSID_CONFIG] = "C_BAD_SUBSTREAMID",
+	[EVT_ID_STREAM_DISABLED] = "F_STREAM_DISABLED",
+
+	/* Bad translation events */
+	[EVT_ID_TRANSLATION_FAULT] = "F_TRANSLATION",
+	[EVT_ID_ADDR_SIZE_FAULT] = "F_ADDR_SIZE",
+	[EVT_ID_ACCESS_FAULT] = "F_ACCESS",
+	[EVT_ID_PERMISSION_FAULT] = "F_PERMISSION",
+
+	/* Bad fetch events */
+	[EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
+	[EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
+	[EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FAULT",
+	[EVT_ID_MAX] = NULL,
+};
+
+static const char * const event_class_str[] = {
+	[0] = "CD fetch",
+	[1] = "Stage 1 translation table fetch",
+	[2] = "Input address caused fault ",
+	[3] = "Reserved",
+};
+
 static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
 				    struct arm_smmu_device *smmu, u32 flags);
 static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
@@ -1756,6 +1784,60 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
 	return rb_entry(node, struct arm_smmu_stream, node)->master;
 }
 
+static void arm_smmu_dump_raw_event(struct arm_smmu_event *event)
+{
+	int i;
+	struct arm_smmu_device *smmu = event->smmu;
+
+	dev_err(smmu->dev, "event 0x%02x received: master %s:\n",
+		event->id, event->master_name);
+
+	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
+		dev_err(smmu->dev, "\t0x%016llx\n", event->raw[i]);
+}
+
+static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
+{
+	struct arm_smmu_device *smmu = evt->smmu;
+	char title[100] = {0};
+	char mastr[100] = {0};
+	char addrs[100] = {0};
+	char flags[100] = {0};
+	char other[50] = {0};
+
+	if (!__ratelimit(rs))
+		return;
+
+	snprintf(title, 100, "Unexpected event received: %s\n",	event_str[evt->id]);
+	snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",
+		 evt->master_name, evt->sid, evt->ssid);
+
+	switch (evt->id) {
+	case EVT_ID_TRANSLATION_FAULT:
+	case EVT_ID_ADDR_SIZE_FAULT:
+	case EVT_ID_ACCESS_FAULT:
+	case EVT_ID_PERMISSION_FAULT:
+		snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
+		snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
+		snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
+			 evt->privileged ? "Priv | " : "Unpriv | ",
+			 evt->instruction ? "Inst | " : "Data | ",
+			 evt->read ? "Read | " : "Write | ",
+			 evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
+			 evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");
+		break;
+
+	case EVT_ID_STE_FETCH_FAULT:
+	case EVT_ID_CD_FETCH_FAULT:
+	case EVT_ID_VMS_FETCH_FAULT:
+		snprintf(addrs, 100, "\tFetch address: %#llx\n", evt->ipa);
+		break;
+	}
+
+	dev_err(smmu->dev, "%s%s%s%s%s", title, mastr, addrs, flags, other);
+	arm_smmu_dump_raw_event(evt);
+}
+
 /* IRQ and event handlers */
 static int arm_smmu_handle_evt(struct arm_smmu_event *event)
 {
@@ -1819,6 +1901,8 @@ static int arm_smmu_handle_evt(struct arm_smmu_event *event)
 static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
 					struct arm_smmu_event *event)
 {
+	struct arm_smmu_master *master;
+
 	/* Pick out the good stuff */
 	event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
 	event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
@@ -1833,12 +1917,24 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
 	event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
 	event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
 	event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
+	event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, event->raw[1]);
+	event->ttrnw_valid = false;
 	event->smmu = smmu;
+	event->dev = NULL;
+
+	if (event->id == EVT_ID_PERMISSION_FAULT)
+		event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
+
+	mutex_lock(&smmu->streams_mutex);
+	master = arm_smmu_find_master(smmu, event->sid);
+	if (master)
+		event->dev = get_device(master->dev);
+	mutex_unlock(&smmu->streams_mutex);
+	event->master_name = event->dev ? dev_name(event->dev) : "(unassigned sid)";
 }
 
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
-	int i, ret;
 	struct arm_smmu_event evt;
 	struct arm_smmu_device *smmu = dev;
 	struct arm_smmu_queue *q = &smmu->evtq.q;
@@ -1850,15 +1946,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 		while (!queue_remove_raw(q, evt.raw)) {
 
 			arm_smmu_get_event_from_raw(smmu, &evt);
-			ret = arm_smmu_handle_evt(&evt);
-			if (!ret || !__ratelimit(&rs))
-				continue;
-
-			dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
-			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
-				dev_info(smmu->dev, "\t0x%016llx\n",
-					(unsigned long long)evt.raw[i]);
+			if (arm_smmu_handle_evt(&evt))
+				arm_smmu_dump_event(&evt, &rs);
 
+			put_device(evt.dev);
 			cond_resched();
 		}
 
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 8a42d7b701fb..820b08f872af 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -438,10 +438,19 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
 #define EVTQ_0_ID			GENMASK_ULL(7, 0)
 
 /* Events */
+#define EVT_ID_BAD_SID_CONFIG		0x02
+#define EVT_ID_STE_FETCH_FAULT		0x03
+#define EVT_ID_BAD_STE_CONFIG		0x04
+#define EVT_ID_STREAM_DISABLED		0x06
+#define EVT_ID_BAD_SSID_CONFIG		0x08
+#define EVT_ID_CD_FETCH_FAULT		0x09
+#define EVT_ID_BAD_CD_CONFIG		0x0a
 #define EVT_ID_TRANSLATION_FAULT	0x10
 #define EVT_ID_ADDR_SIZE_FAULT		0x11
 #define EVT_ID_ACCESS_FAULT		0x12
 #define EVT_ID_PERMISSION_FAULT		0x13
+#define EVT_ID_VMS_FETCH_FAULT		0x25
+#define EVT_ID_MAX			0xff
 
 #define EVTQ_0_SSV			(1UL << 11)
 #define EVTQ_0_SSID			GENMASK_ULL(31, 12)
@@ -774,7 +783,9 @@ struct arm_smmu_stream {
 };
 
 struct arm_smmu_event {
+	const char			*master_name;
 	struct arm_smmu_device		*smmu;
+	struct device			*dev;
 	u8				id;
 	u8				class;
 	u16				stag;
@@ -789,6 +800,8 @@ struct arm_smmu_event {
 	bool				instruction;
 	bool				s2;
 	bool				read;
+	bool				ttrnw;
+	bool				ttrnw_valid;
 };
 
 /* SMMU private data for each master */
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH v4 3/3] iommu/arm-smmu-v3: Avoid redundant master lookup in events

[Pranjal Shrivastava]

Remove the call to `arm_smmu_find_master` in `arm_smmu_handle_evt`.

The call was primarily to retrieve `master->dev` ptr for reporting iommu
device faults. Since this info is already available in the `event->dev`
field, we no longer need to lookup the master to fetch the dev pointer.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 4477cf86cb8e..3eebc0856086 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1841,11 +1841,8 @@ static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_sta
 /* IRQ and event handlers */
 static int arm_smmu_handle_evt(struct arm_smmu_event *event)
 {
-	int ret = 0;
 	u32 perm = 0;
-	struct arm_smmu_master *master;
 	struct iopf_fault fault_evt = { };
-	struct arm_smmu_device *smmu = event->smmu;
 	struct iommu_fault *flt = &fault_evt.fault;
 
 	switch (event->id) {
@@ -1885,17 +1882,14 @@ static int arm_smmu_handle_evt(struct arm_smmu_event *event)
 		flt->prm.pasid = event->ssid;
 	}
 
-	mutex_lock(&smmu->streams_mutex);
-	master = arm_smmu_find_master(smmu, event->sid);
-	if (!master) {
-		ret = -EINVAL;
-		goto out_unlock;
-	}
+	/*
+	 * If the master wasn't found while reading the event or
+	 * get_device() returned NULL, we shouldn't report a fault.
+	 */
+	if (!event->dev)
+		return -EINVAL;
 
-	ret = iommu_report_device_fault(master->dev, &fault_evt);
-out_unlock:
-	mutex_unlock(&smmu->streams_mutex);
-	return ret;
+	return iommu_report_device_fault(event->dev, &fault_evt);
 }
 
 static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH v4 0/3] iommu/arm-smmu-v3: Parse out event records

[Nicolin Chen]

On Fri, Oct 18, 2024 at 06:00:19PM +0000, Pranjal Shrivastava wrote:
 
> As requested by Nicolin, some sample logs powered by QEMU:
> 
> [   85.410290] arm-smmu-v3 arm-smmu-v3.0.auto: Unexpected event received: F_PERMISSION
> [   85.410290]  master: 0000:00:01.0 sid: 0x00000008.0x00000

Thanks!

Should we print "sid.ssid:" v.s. "sid:"?

Nicolin

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Nicolin Chen]

On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> Introduce `struct arm_smmu_event` to represent event records.
> Parse out relevant fields from raw event records for ease and
> use the new `struct arm_smmu_event` instead.
> 
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> Signed-off-by: Pranjal Shrivastava <praan@google.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

With some nits:

> +static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> +                                       struct arm_smmu_event *event)
> +{
> +       /* Pick out the good stuff */
> +       event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> +       event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> +       event->ssid_valid = event->raw[0] & EVTQ_0_SSV;
> +       event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;

	event->ssid = event->ssid_valid ?
		      FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;

Or maybe would better memset(0) the event, and then:
	if (event->ssid_valid)
		event->ssid = FIELD_GET(EVTQ_0_SSID, event->raw[0]);

>  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)

>         do {
> -               while (!queue_remove_raw(q, evt)) {
> -                       u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> +               while (!queue_remove_raw(q, evt.raw)) {
> 
> -                       ret = arm_smmu_handle_evt(smmu, evt);

Could drop that blank line in-between as well.

> +                       arm_smmu_get_event_from_raw(smmu, &evt);
> +                       ret = arm_smmu_handle_evt(&evt);
>                         if (!ret || !__ratelimit(&rs))
>                                 continue;
> 
> -                       dev_info(smmu->dev, "event 0x%02x received:\n", id);
> -                       for (i = 0; i < ARRAY_SIZE(evt); ++i)
> +                       dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> +                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
>                                 dev_info(smmu->dev, "\t0x%016llx\n",
> -                                        (unsigned long long)evt[i]);
> +                                       (unsigned long long)evt.raw[i]);
                                          ^
Could keep the previous indentation -----|

Nicolin

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Nicolin Chen]

On Fri, Oct 18, 2024 at 06:00:21PM +0000, Pranjal Shrivastava wrote:
> +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> +{
> +       struct arm_smmu_device *smmu = evt->smmu;
> +       char title[100] = {0};
> +       char mastr[100] = {0};
> +       char addrs[100] = {0};
> +       char flags[100] = {0};
> +       char other[50] = {0};
> +
> +       if (!__ratelimit(rs))
> +               return;
> +
> +       snprintf(title, 100, "Unexpected event received: %s\n", event_str[evt->id]);
> +       snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> +                evt->master_name, evt->sid, evt->ssid);

Likely I mentioned in the cover-letter, maybe "sid.ssid:"?

> +       switch (evt->id) {
> +       case EVT_ID_TRANSLATION_FAULT:
> +       case EVT_ID_ADDR_SIZE_FAULT:
> +       case EVT_ID_ACCESS_FAULT:
> +       case EVT_ID_PERMISSION_FAULT:
> +               snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
> +               snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
> +               snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
> +                        evt->privileged ? "Priv | " : "Unpriv | ",
> +                        evt->instruction ? "Inst | " : "Data | ",
> +                        evt->read ? "Read | " : "Write | ",
> +                        evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
> +                        evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");

Should the last one be "TTD Read |" : "TTD Write|"?
Otherwise, it would be "S2 || TTD Read" combined.

>  static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
>                                         struct arm_smmu_event *event)
>  {
> +       struct arm_smmu_master *master;
> +
>         /* Pick out the good stuff */
>         event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
>         event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> @@ -1833,12 +1917,24 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
>         event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
>         event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
>         event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> +       event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, event->raw[1]);
> +       event->ttrnw_valid = false;
>         event->smmu = smmu;
> +       event->dev = NULL;
> +
> +       if (event->id == EVT_ID_PERMISSION_FAULT)
> +               event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
> +
> +       mutex_lock(&smmu->streams_mutex);
> +       master = arm_smmu_find_master(smmu, event->sid);
> +       if (master)
> +               event->dev = get_device(master->dev);

Here, get_device is called upon a valid master...

>  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>  {
> -       int i, ret;
>         struct arm_smmu_event evt;
>         struct arm_smmu_device *smmu = dev;
>         struct arm_smmu_queue *q = &smmu->evtq.q;
> @@ -1850,15 +1946,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>                 while (!queue_remove_raw(q, evt.raw)) {
> 
>                         arm_smmu_get_event_from_raw(smmu, &evt);
> -                       ret = arm_smmu_handle_evt(&evt);
> -                       if (!ret || !__ratelimit(&rs))
> -                               continue;
> -
> -                       dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> -                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> -                               dev_info(smmu->dev, "\t0x%016llx\n",
> -                                       (unsigned long long)evt.raw[i]);
> +                       if (arm_smmu_handle_evt(&evt))
> +                               arm_smmu_dump_event(&evt, &rs);
> 
> +                       put_device(evt.dev);

then, here it puts unconditionally.

Maybe we do need a memset(0) to the event, then here
			if (evet.dev)
				put_device(evt.dev);

Thanks
Nicolin

Re: [PATCH v4 3/3] iommu/arm-smmu-v3: Avoid redundant master lookup in events

[Nicolin Chen]

On Fri, Oct 18, 2024 at 06:00:22PM +0000, Pranjal Shrivastava wrote:
> 
> Remove the call to `arm_smmu_find_master` in `arm_smmu_handle_evt`.
> 
> The call was primarily to retrieve `master->dev` ptr for reporting iommu
> device faults. Since this info is already available in the `event->dev`
> field, we no longer need to lookup the master to fetch the dev pointer.
> 
> Signed-off-by: Pranjal Shrivastava <praan@google.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Nicolin Chen]

On Fri, Oct 18, 2024 at 07:06:49PM -0700, Nicolin Chen wrote:
> > +                       put_device(evt.dev);
> 
> then, here it puts unconditionally.
> 
> Maybe we do need a memset(0) to the event, then here
>                         if (evet.dev)
>                                 put_device(evt.dev);

Oh, put_device does that already. So we are fine here. Pls ignore.

With the other two nits, it looks good to me,
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Nicolin

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Fri, Oct 18, 2024 at 06:56:14PM -0700, Nicolin Chen wrote:
> On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> > Introduce `struct arm_smmu_event` to represent event records.
> > Parse out relevant fields from raw event records for ease and
> > use the new `struct arm_smmu_event` instead.
> > 
> > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> With some nits:
> 
> > +static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> > +                                       struct arm_smmu_event *event)
> > +{
> > +       /* Pick out the good stuff */
> > +       event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> > +       event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > +       event->ssid_valid = event->raw[0] & EVTQ_0_SSV;
> > +       event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
> 
> 	event->ssid = event->ssid_valid ?
> 		      FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
> 
> Or maybe would better memset(0) the event, and then:
> 	if (event->ssid_valid)
> 		event->ssid = FIELD_GET(EVTQ_0_SSID, event->raw[0]);
> 

I like the former better but I think we should also memset(event, 0)

> >  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> 
> >         do {
> > -               while (!queue_remove_raw(q, evt)) {
> > -                       u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> > +               while (!queue_remove_raw(q, evt.raw)) {
> > 
> > -                       ret = arm_smmu_handle_evt(smmu, evt);
> 
> Could drop that blank line in-between as well.
> 

Ack.

> > +                       arm_smmu_get_event_from_raw(smmu, &evt);
> > +                       ret = arm_smmu_handle_evt(&evt);
> >                         if (!ret || !__ratelimit(&rs))
> >                                 continue;
> > 
> > -                       dev_info(smmu->dev, "event 0x%02x received:\n", id);
> > -                       for (i = 0; i < ARRAY_SIZE(evt); ++i)
> > +                       dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > +                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> >                                 dev_info(smmu->dev, "\t0x%016llx\n",
> > -                                        (unsigned long long)evt[i]);
> > +                                       (unsigned long long)evt.raw[i]);
>                                           ^
> Could keep the previous indentation -----|
> 

Ack.

> Nicolin

Thanks!
Pranjal

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Pranjal Shrivastava]

On Fri, Oct 18, 2024 at 07:06:49PM -0700, Nicolin Chen wrote:
> On Fri, Oct 18, 2024 at 06:00:21PM +0000, Pranjal Shrivastava wrote:
> > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > +{
> > +       struct arm_smmu_device *smmu = evt->smmu;
> > +       char title[100] = {0};
> > +       char mastr[100] = {0};
> > +       char addrs[100] = {0};
> > +       char flags[100] = {0};
> > +       char other[50] = {0};
> > +
> > +       if (!__ratelimit(rs))
> > +               return;
> > +
> > +       snprintf(title, 100, "Unexpected event received: %s\n", event_str[evt->id]);
> > +       snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> > +                evt->master_name, evt->sid, evt->ssid);
> 
> Likely I mentioned in the cover-letter, maybe "sid.ssid:"?
> 

+1. I like the idea. Maybe let's update the log in ppr to follow this
as well? Does that sound good?

> > +       switch (evt->id) {
> > +       case EVT_ID_TRANSLATION_FAULT:
> > +       case EVT_ID_ADDR_SIZE_FAULT:
> > +       case EVT_ID_ACCESS_FAULT:
> > +       case EVT_ID_PERMISSION_FAULT:
> > +               snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
> > +               snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
> > +               snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
> > +                        evt->privileged ? "Priv | " : "Unpriv | ",
> > +                        evt->instruction ? "Inst | " : "Data | ",
> > +                        evt->read ? "Read | " : "Write | ",
> > +                        evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
> > +                        evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");
> 
> Should the last one be "TTD Read |" : "TTD Write|"?
> Otherwise, it would be "S2 || TTD Read" combined.

Umm.. I don't expect the event_class_str[evt->class] to be NULL ever.
Hence the logs would always be in the following format:

Unpriv | Data | Write | S2 | <class_str> | TTD <Read/Write>

> 
> >  static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> >                                         struct arm_smmu_event *event)
> >  {
> > +       struct arm_smmu_master *master;
> > +
> >         /* Pick out the good stuff */
> >         event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> >         event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > @@ -1833,12 +1917,24 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> >         event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
> >         event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
> >         event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> > +       event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, event->raw[1]);
> > +       event->ttrnw_valid = false;
> >         event->smmu = smmu;
> > +       event->dev = NULL;
> > +
> > +       if (event->id == EVT_ID_PERMISSION_FAULT)
> > +               event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
> > +
> > +       mutex_lock(&smmu->streams_mutex);
> > +       master = arm_smmu_find_master(smmu, event->sid);
> > +       if (master)
> > +               event->dev = get_device(master->dev);
> 
> Here, get_device is called upon a valid master...
> 
> >  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >  {
> > -       int i, ret;
> >         struct arm_smmu_event evt;
> >         struct arm_smmu_device *smmu = dev;
> >         struct arm_smmu_queue *q = &smmu->evtq.q;
> > @@ -1850,15 +1946,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >                 while (!queue_remove_raw(q, evt.raw)) {
> > 
> >                         arm_smmu_get_event_from_raw(smmu, &evt);
> > -                       ret = arm_smmu_handle_evt(&evt);
> > -                       if (!ret || !__ratelimit(&rs))
> > -                               continue;
> > -
> > -                       dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > -                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > -                               dev_info(smmu->dev, "\t0x%016llx\n",
> > -                                       (unsigned long long)evt.raw[i]);
> > +                       if (arm_smmu_handle_evt(&evt))
> > +                               arm_smmu_dump_event(&evt, &rs);
> > 
> > +                       put_device(evt.dev);
> 
> then, here it puts unconditionally.
> 
> Maybe we do need a memset(0) to the event, then here
> 			if (evet.dev)
> 				put_device(evt.dev);

`put_device` takes care of NULL args as well.

> 
> Thanks
> Nicolin

Thanks!
Pranjal

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Pranjal Shrivastava]

On Fri, Oct 18, 2024 at 09:51:40PM -0700, Nicolin Chen wrote:
> On Fri, Oct 18, 2024 at 07:06:49PM -0700, Nicolin Chen wrote:
> > > +                       put_device(evt.dev);
> > 
> > then, here it puts unconditionally.
> > 
> > Maybe we do need a memset(0) to the event, then here
> >                         if (evet.dev)
> >                                 put_device(evt.dev);
> 
> Oh, put_device does that already. So we are fine here. Pls ignore.

Yes. Ack.

> 
> With the other two nits, it looks good to me,
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> Nicolin

Thanks
Pranjal

Re: [PATCH v4 0/3] iommu/arm-smmu-v3: Parse out event records

[Pranjal Shrivastava]

On Fri, Oct 18, 2024 at 06:45:57PM -0700, Nicolin Chen wrote:
> On Fri, Oct 18, 2024 at 06:00:19PM +0000, Pranjal Shrivastava wrote:
>  
> > As requested by Nicolin, some sample logs powered by QEMU:
> > 
> > [   85.410290] arm-smmu-v3 arm-smmu-v3.0.auto: Unexpected event received: F_PERMISSION
> > [   85.410290]  master: 0000:00:01.0 sid: 0x00000008.0x00000
> 
> Thanks!
> 
> Should we print "sid.ssid:" v.s. "sid:"?

Ack. I like the idea. Btw, a sample of the longest log would be:

[   85.410290] arm-smmu-v3 arm-smmu-v3.0.auto: Unexpected event received: F_PERMISSION
[   85.410290]  master: 0000:00:01.0 sid.ssid: 0x00000008.0x00000
[   85.410290]  iova = 0xffffc0d0 ipa = 0x0
[   85.410290]  Unpriv | Data | Write | S2 | Stage 1 translation table fetch | TTD Write
[   85.410290]  STAG = 0x0 Stall = 0x0

(Note: I've hacked something manually to print the longest strings)

> 
> Nicolin

Thanks,
Pranjal

Re: [PATCH v4 0/3] iommu/arm-smmu-v3: Parse out event records

[Nicolin Chen]

On Mon, Oct 21, 2024 at 06:33:47AM +0000, Pranjal Shrivastava wrote:
> On Fri, Oct 18, 2024 at 06:45:57PM -0700, Nicolin Chen wrote:
> > On Fri, Oct 18, 2024 at 06:00:19PM +0000, Pranjal Shrivastava wrote:
> >
> > > As requested by Nicolin, some sample logs powered by QEMU:
> > >
> > > [   85.410290] arm-smmu-v3 arm-smmu-v3.0.auto: Unexpected event received: F_PERMISSION
> > > [   85.410290]  master: 0000:00:01.0 sid: 0x00000008.0x00000
> >
> > Thanks!
> >
> > Should we print "sid.ssid:" v.s. "sid:"?
> 
> Ack. I like the idea. Btw, a sample of the longest log would be:
> 
> [   85.410290] arm-smmu-v3 arm-smmu-v3.0.auto: Unexpected event received: F_PERMISSION
> [   85.410290]  master: 0000:00:01.0 sid.ssid: 0x00000008.0x00000
> [   85.410290]  iova = 0xffffc0d0 ipa = 0x0
> [   85.410290]  Unpriv | Data | Write | S2 | Stage 1 translation table fetch | TTD Write
> [   85.410290]  STAG = 0x0 Stall = 0x0
> 
> (Note: I've hacked something manually to print the longest strings)

LGTM!

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Nicolin Chen]

On Mon, Oct 21, 2024 at 06:26:50AM +0000, Pranjal Shrivastava wrote:
> On Fri, Oct 18, 2024 at 07:06:49PM -0700, Nicolin Chen wrote:
> > On Fri, Oct 18, 2024 at 06:00:21PM +0000, Pranjal Shrivastava wrote:
> > > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > > +{
> > > +       struct arm_smmu_device *smmu = evt->smmu;
> > > +       char title[100] = {0};
> > > +       char mastr[100] = {0};
> > > +       char addrs[100] = {0};
> > > +       char flags[100] = {0};
> > > +       char other[50] = {0};
> > > +
> > > +       if (!__ratelimit(rs))
> > > +               return;
> > > +
> > > +       snprintf(title, 100, "Unexpected event received: %s\n", event_str[evt->id]);
> > > +       snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> > > +                evt->master_name, evt->sid, evt->ssid);
> >
> > Likely I mentioned in the cover-letter, maybe "sid.ssid:"?
> >
> 
> +1. I like the idea. Maybe let's update the log in ppr to follow this
> as well? Does that sound good?

Oh right, you mentioned that once.. Yea, I think that would be
nicer.

> > > +       switch (evt->id) {
> > > +       case EVT_ID_TRANSLATION_FAULT:
> > > +       case EVT_ID_ADDR_SIZE_FAULT:
> > > +       case EVT_ID_ACCESS_FAULT:
> > > +       case EVT_ID_PERMISSION_FAULT:
> > > +               snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
> > > +               snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
> > > +               snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
> > > +                        evt->privileged ? "Priv | " : "Unpriv | ",
> > > +                        evt->instruction ? "Inst | " : "Data | ",
> > > +                        evt->read ? "Read | " : "Write | ",
> > > +                        evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
> > > +                        evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");
> >
> > Should the last one be "TTD Read |" : "TTD Write|"?
> > Otherwise, it would be "S2 || TTD Read" combined.
> 
> Umm.. I don't expect the event_class_str[evt->class] to be NULL ever.
> Hence the logs would always be in the following format:
> 
> Unpriv | Data | Write | S2 | <class_str> | TTD <Read/Write>

Ah, I overlooked that one :) We are fine then.

THanks
Nicolin

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Will Deacon]

On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> +struct arm_smmu_event {
> +	struct arm_smmu_device		*smmu;
> +	u8				id;
> +	u8				class;
> +	u16				stag;
> +	u32				sid;
> +	u32				ssid;
> +	u64				iova;
> +	u64				ipa;
> +	u64				raw[EVTQ_ENT_DWORDS];
> +	bool				stall;
> +	bool				ssid_valid;
> +	bool				privileged;
> +	bool				instruction;
> +	bool				s2;
> +	bool				read;
> +};

minor nit, but it might be worth seeing what pahole says about the
layout of this structure in case you've got a bunch of wasted padding
thanks to the mixed-size fields.

Will

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Will Deacon]

On Fri, Oct 18, 2024 at 06:00:21PM +0000, Pranjal Shrivastava wrote:
> Currently, the driver dumps the raw hex for a received event record.
> Improve this by leveraging `struct arm_smmu_event` for event fields
> and log human-readable event records with meaningful information.
> 
> Signed-off-by: Pranjal Shrivastava <praan@google.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 109 ++++++++++++++++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  13 +++
>  2 files changed, 113 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 2f1108e5de51..4477cf86cb8e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -83,6 +83,34 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>  	{ 0, NULL},
>  };
>  
> +static const char * const event_str[] = {
> +	/* Bad config events */
> +	[EVT_ID_BAD_SID_CONFIG] = "C_BAD_STREAMID",
> +	[EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
> +	[EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
> +	[EVT_ID_BAD_SSID_CONFIG] = "C_BAD_SUBSTREAMID",
> +	[EVT_ID_STREAM_DISABLED] = "F_STREAM_DISABLED",
> +
> +	/* Bad translation events */
> +	[EVT_ID_TRANSLATION_FAULT] = "F_TRANSLATION",
> +	[EVT_ID_ADDR_SIZE_FAULT] = "F_ADDR_SIZE",
> +	[EVT_ID_ACCESS_FAULT] = "F_ACCESS",
> +	[EVT_ID_PERMISSION_FAULT] = "F_PERMISSION",
> +
> +	/* Bad fetch events */
> +	[EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
> +	[EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
> +	[EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FAULT",
> +	[EVT_ID_MAX] = NULL,
> +};
> +
> +static const char * const event_class_str[] = {
> +	[0] = "CD fetch",
> +	[1] = "Stage 1 translation table fetch",
> +	[2] = "Input address caused fault ",
> +	[3] = "Reserved",
> +};
> +
>  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>  				    struct arm_smmu_device *smmu, u32 flags);
>  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
> @@ -1756,6 +1784,60 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
>  	return rb_entry(node, struct arm_smmu_stream, node)->master;
>  }
>  
> +static void arm_smmu_dump_raw_event(struct arm_smmu_event *event)
> +{
> +	int i;
> +	struct arm_smmu_device *smmu = event->smmu;
> +
> +	dev_err(smmu->dev, "event 0x%02x received: master %s:\n",
> +		event->id, event->master_name);
> +
> +	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> +		dev_err(smmu->dev, "\t0x%016llx\n", event->raw[i]);
> +}
> +
> +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> +{
> +	struct arm_smmu_device *smmu = evt->smmu;
> +	char title[100] = {0};
> +	char mastr[100] = {0};
> +	char addrs[100] = {0};
> +	char flags[100] = {0};
> +	char other[50] = {0};

I haven't followed previous versions of this series in detail, but
allocating 450 bytes on the stack for this seems excessive to me.

Will

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Pranjal Shrivastava]

On Thu, Oct 24, 2024 at 02:15:12PM +0100, Will Deacon wrote:
> On Fri, Oct 18, 2024 at 06:00:21PM +0000, Pranjal Shrivastava wrote:
> > Currently, the driver dumps the raw hex for a received event record.
> > Improve this by leveraging `struct arm_smmu_event` for event fields
> > and log human-readable event records with meaningful information.
> > 
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 109 ++++++++++++++++++--
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  13 +++
> >  2 files changed, 113 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 2f1108e5de51..4477cf86cb8e 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -83,6 +83,34 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
> >  	{ 0, NULL},
> >  };
> >  
> > +static const char * const event_str[] = {
> > +	/* Bad config events */
> > +	[EVT_ID_BAD_SID_CONFIG] = "C_BAD_STREAMID",
> > +	[EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
> > +	[EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
> > +	[EVT_ID_BAD_SSID_CONFIG] = "C_BAD_SUBSTREAMID",
> > +	[EVT_ID_STREAM_DISABLED] = "F_STREAM_DISABLED",
> > +
> > +	/* Bad translation events */
> > +	[EVT_ID_TRANSLATION_FAULT] = "F_TRANSLATION",
> > +	[EVT_ID_ADDR_SIZE_FAULT] = "F_ADDR_SIZE",
> > +	[EVT_ID_ACCESS_FAULT] = "F_ACCESS",
> > +	[EVT_ID_PERMISSION_FAULT] = "F_PERMISSION",
> > +
> > +	/* Bad fetch events */
> > +	[EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
> > +	[EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
> > +	[EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FAULT",
> > +	[EVT_ID_MAX] = NULL,
> > +};
> > +
> > +static const char * const event_class_str[] = {
> > +	[0] = "CD fetch",
> > +	[1] = "Stage 1 translation table fetch",
> > +	[2] = "Input address caused fault ",
> > +	[3] = "Reserved",
> > +};
> > +
> >  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> >  				    struct arm_smmu_device *smmu, u32 flags);
> >  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
> > @@ -1756,6 +1784,60 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> >  	return rb_entry(node, struct arm_smmu_stream, node)->master;
> >  }
> >  
> > +static void arm_smmu_dump_raw_event(struct arm_smmu_event *event)
> > +{
> > +	int i;
> > +	struct arm_smmu_device *smmu = event->smmu;
> > +
> > +	dev_err(smmu->dev, "event 0x%02x received: master %s:\n",
> > +		event->id, event->master_name);
> > +
> > +	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > +		dev_err(smmu->dev, "\t0x%016llx\n", event->raw[i]);
> > +}
> > +
> > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > +{
> > +	struct arm_smmu_device *smmu = evt->smmu;
> > +	char title[100] = {0};
> > +	char mastr[100] = {0};
> > +	char addrs[100] = {0};
> > +	char flags[100] = {0};
> > +	char other[50] = {0};
> 
> I haven't followed previous versions of this series in detail, but
> allocating 450 bytes on the stack for this seems excessive to me.

Hmm, I think we can reduce the title, mastr, addrs string to 50 chars
and the `other` can be reduced to 30? Or maybe we can avoid printing the
`stag` and `stall` fields altogether which should reduce it by 200 bytes
We can save another 20 bytes by reducing the `flags` len to 80 bytes,
overall saving us 220 bytes of stack space.

I introduced this in v3 [1] based on suggestions in v2 reviews [2].
I can revert to the approach followed in v2 [3] as well.

LMK what's your vote?
> 
> Will

Thanks,
Pranjal

[1] https://lore.kernel.org/all/20240928005143.2378938-1-praan@google.com/
[2] https://lore.kernel.org/linux-iommu/ZtAW0hVPFD6JbLTL@Asurada-Nvidia/
[3] https://lore.kernel.org/linux-iommu/20240827193026.3993039-2-praan@google.com/

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Thu, Oct 24, 2024 at 02:11:48PM +0100, Will Deacon wrote:
> On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> > +struct arm_smmu_event {
> > +	struct arm_smmu_device		*smmu;
> > +	u8				id;
> > +	u8				class;
> > +	u16				stag;
> > +	u32				sid;
> > +	u32				ssid;
> > +	u64				iova;
> > +	u64				ipa;
> > +	u64				raw[EVTQ_ENT_DWORDS];
> > +	bool				stall;
> > +	bool				ssid_valid;
> > +	bool				privileged;
> > +	bool				instruction;
> > +	bool				s2;
> > +	bool				read;
> > +};
> 
> minor nit, but it might be worth seeing what pahole says about the
> layout of this structure in case you've got a bunch of wasted padding
> thanks to the mixed-size fields.

Interesting! I didn't know what pahole was, let me see how to use it!

> 
> Will

Thanks,
Pranjal

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Thu, Oct 24, 2024 at 02:11:48PM +0100, Will Deacon wrote:
> On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> > +struct arm_smmu_event {
> > +	struct arm_smmu_device		*smmu;
> > +	u8				id;
> > +	u8				class;
> > +	u16				stag;
> > +	u32				sid;
> > +	u32				ssid;
> > +	u64				iova;
> > +	u64				ipa;
> > +	u64				raw[EVTQ_ENT_DWORDS];
> > +	bool				stall;
> > +	bool				ssid_valid;
> > +	bool				privileged;
> > +	bool				instruction;
> > +	bool				s2;
> > +	bool				read;
> > +};
> 
> minor nit, but it might be worth seeing what pahole says about the
> layout of this structure in case you've got a bunch of wasted padding
> thanks to the mixed-size fields.

I ran pahole with this, looks like there's only one 4-byte hole but the
cacheline aligment is bad (for a 64-byte cacheline):

struct arm_smmu_event {
	const char  *              master_name;          /*     0     8 */
	struct arm_smmu_device *   smmu;                 /*     8     8 */
	struct device *            dev;                  /*    16     8 */
	u8                         id;                   /*    24     1 */
	u8                         class;                /*    25     1 */
	u16                        stag;                 /*    26     2 */
	u32                        sid;                  /*    28     4 */
	u32                        ssid;                 /*    32     4 */

	/* XXX 4 bytes hole, try to pack */

	u64                        iova;                 /*    40     8 */
	u64                        ipa;                  /*    48     8 */
	u64                        raw[4];               /*    56    32 */
	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
	bool                       stall;                /*    88     1 */
	bool                       ssid_valid;           /*    89     1 */
	bool                       privileged;           /*    90     1 */
	bool                       instruction;          /*    91     1 */
	bool                       s2;                   /*    92     1 */
	bool                       read;                 /*    93     1 */
	bool                       ttrnw;                /*    94     1 */
	bool                       ttrnw_valid;          /*    95     1 */

	/* size: 96, cachelines: 2, members: 19 */
	/* sum members: 92, holes: 1, sum holes: 4 */
	/* last cacheline: 32 bytes */
};

I don't think we can do much about the 4-byte hole as the members occupy
92 bytes only. I assume a single 4-byte hole shall be fine?

However, for cacheline aligment we can move the 3 top pointer-members,
`master_name`,`smmu` & `dev` which improves the cacheline aligment:

struct arm_smmu_event {
	u8                         id;                   /*     0     1 */
	u8                         class;                /*     1     1 */
	u16                        stag;                 /*     2     2 */
	u32                        sid;                  /*     4     4 */
	u32                        ssid;                 /*     8     4 */

	/* XXX 4 bytes hole, try to pack */

	u64                        iova;                 /*    16     8 */
	u64                        ipa;                  /*    24     8 */
	u64                        raw[4];               /*    32    32 */
	/* --- cacheline 1 boundary (64 bytes) --- */
	bool                       stall;                /*    64     1 */
	bool                       ssid_valid;           /*    65     1 */
	bool                       privileged;           /*    66     1 */
	bool                       instruction;          /*    67     1 */
	bool                       s2;                   /*    68     1 */
	bool                       read;                 /*    69     1 */
	bool                       ttrnw;                /*    70     1 */
	bool                       ttrnw_valid;          /*    71     1 */
	const char  *              master_name;          /*    72     8 */
	struct arm_smmu_device *   smmu;                 /*    80     8 */
	struct device *            dev;                  /*    88     8 */

	/* size: 96, cachelines: 2, members: 19 */
	/* sum members: 92, holes: 1, sum holes: 4 */
	/* last cacheline: 32 bytes */
};

I'll fix this in the next version of the patch. Thanks!

> 
> Will

Thanks,
Praan

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Jason Gunthorpe]

On Thu, Oct 24, 2024 at 05:02:08PM +0000, Pranjal Shrivastava wrote:
> 	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> 	bool                       stall;                /*    88     1 */
> 	bool                       ssid_valid;           /*    89     1 */
> 	bool                       privileged;           /*    90     1 */
> 	bool                       instruction;          /*    91     1 */
> 	bool                       s2;                   /*    92     1 */
> 	bool                       read;                 /*    93     1 */
> 	bool                       ttrnw;                /*    94     1 */
> 	bool                       ttrnw_valid;          /*    95     1 */

Linus has had negative things to say about lists of bools in
structs. Use a bitfield?

Jason

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Thu, Oct 24, 2024 at 02:03:29PM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 24, 2024 at 05:02:08PM +0000, Pranjal Shrivastava wrote:
> > 	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> > 	bool                       stall;                /*    88     1 */
> > 	bool                       ssid_valid;           /*    89     1 */
> > 	bool                       privileged;           /*    90     1 */
> > 	bool                       instruction;          /*    91     1 */
> > 	bool                       s2;                   /*    92     1 */
> > 	bool                       read;                 /*    93     1 */
> > 	bool                       ttrnw;                /*    94     1 */
> > 	bool                       ttrnw_valid;          /*    95     1 */
> 
> Linus has had negative things to say about lists of bools in
> structs. Use a bitfield?

Hmm, I agree, each bool is a waste of 7-bits. I tried the following:

struct arm_smmu_event {
	u8				id;
	u8				class;
	u16				stag;

	/* Group the boolean values together */
	unsigned int stall:1;
	unsigned int ssid_valid:1;
	unsigned int privileged:1;
	unsigned int instruction:1;
	unsigned int s2:1;
	unsigned int read:1;
	unsigned int ttrnw:1;
	unsigned int ttrnw_valid:1;

	u32				sid;
	u32				ssid;
	u64				iova;
	u64				ipa;
	u64				raw[EVTQ_ENT_DWORDS];
	const char			*master_name;
	struct arm_smmu_device		*smmu;
	struct device			*dev;
};

This reduces the hole to a 3-byte (24-bit hole).
Also, since we have exactly 8 bits, we can pack them in a union too:

struct arm_smmu_event {
	u8				id;
	u8				class;
	u16				stag;
	
	/* Group the boolean values together */
	union {
		struct {
			unsigned int stall:1;
			unsigned int ssid_valid:1;
			unsigned int privileged:1;
			unsigned int instruction:1;
			unsigned int s2:1;
			unsigned int read:1;
			unsigned int ttrnw:1;
			unsigned int ttrnw_valid:1;
		};
		u8 flags;
	};

	u32				sid;
	u32				ssid;
	u64				iova;
	u64				ipa;
	u64				raw[EVTQ_ENT_DWORDS];
	const char			*master_name;
	struct arm_smmu_device		*smmu;
	struct device			*dev;
};

I like the former better since we won't have to change the union if
future revisions of the hw add new flags. Any preferences?

> 
> Jason

Thanks,
Praan

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Nicolin Chen]

On Thu, Oct 24, 2024 at 02:15:12PM +0100, Will Deacon wrote:
> > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > +{
> > +     struct arm_smmu_device *smmu = evt->smmu;
> > +     char title[100] = {0};
> > +     char mastr[100] = {0};
> > +     char addrs[100] = {0};
> > +     char flags[100] = {0};
> > +     char other[50] = {0};
> 
> I haven't followed previous versions of this series in detail, but
> allocating 450 bytes on the stack for this seems excessive to me.

If "450B" raises a red flag, should we do something about these?

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c: In function ‘arm_smmu_atc_inv_master.isra’:
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2065:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c: In function ‘arm_smmu_sync_cd’:
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:1218:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c: In function ‘__arm_smmu_tlb_inv_range’:
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2213:1: warning: the frame size of 1072 bytes is larger than 1024 bytes [-Wframe-larger-than=]
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c: In function ‘arm_smmu_atc_inv_domain’:
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2118:1: warning: the frame size of 1120 bytes is larger than 1024 bytes [-Wframe-larger-than=]

Seems that struct arm_smmu_cmdq_batch is too big. Any suggestion?

Thanks
Nicolin

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Jason Gunthorpe]

On Thu, Oct 24, 2024 at 05:37:11PM +0000, Pranjal Shrivastava wrote:
> On Thu, Oct 24, 2024 at 02:03:29PM -0300, Jason Gunthorpe wrote:
> > On Thu, Oct 24, 2024 at 05:02:08PM +0000, Pranjal Shrivastava wrote:
> > > 	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> > > 	bool                       stall;                /*    88     1 */
> > > 	bool                       ssid_valid;           /*    89     1 */
> > > 	bool                       privileged;           /*    90     1 */
> > > 	bool                       instruction;          /*    91     1 */
> > > 	bool                       s2;                   /*    92     1 */
> > > 	bool                       read;                 /*    93     1 */
> > > 	bool                       ttrnw;                /*    94     1 */
> > > 	bool                       ttrnw_valid;          /*    95     1 */
> > 
> > Linus has had negative things to say about lists of bools in
> > structs. Use a bitfield?
> 
> Hmm, I agree, each bool is a waste of 7-bits. I tried the following:
> 
> struct arm_smmu_event {
> 	u8				id;
> 	u8				class;
> 	u16				stag;
> 
> 	/* Group the boolean values together */
> 	unsigned int stall:1;
> 	unsigned int ssid_valid:1;
> 	unsigned int privileged:1;
> 	unsigned int instruction:1;
> 	unsigned int s2:1;
> 	unsigned int read:1;
> 	unsigned int ttrnw:1;
> 	unsigned int ttrnw_valid:1;

You can make this a u8

> struct arm_smmu_event {
> 	u8				id;
> 	u8				class;
> 	u16				stag;
> 	
> 	/* Group the boolean values together */
> 	union {
> 		struct {
> 			unsigned int stall:1;
> 			unsigned int ssid_valid:1;
> 			unsigned int privileged:1;
> 			unsigned int instruction:1;
> 			unsigned int s2:1;
> 			unsigned int read:1;
> 			unsigned int ttrnw:1;
> 			unsigned int ttrnw_valid:1;
> 		};
> 		u8 flags;

I wouldn't do this..

Jason

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Mon, Oct 28, 2024 at 09:23:53AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 24, 2024 at 05:37:11PM +0000, Pranjal Shrivastava wrote:
> > On Thu, Oct 24, 2024 at 02:03:29PM -0300, Jason Gunthorpe wrote:
> > > On Thu, Oct 24, 2024 at 05:02:08PM +0000, Pranjal Shrivastava wrote:
> > > > 	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> > > > 	bool                       stall;                /*    88     1 */
> > > > 	bool                       ssid_valid;           /*    89     1 */
> > > > 	bool                       privileged;           /*    90     1 */
> > > > 	bool                       instruction;          /*    91     1 */
> > > > 	bool                       s2;                   /*    92     1 */
> > > > 	bool                       read;                 /*    93     1 */
> > > > 	bool                       ttrnw;                /*    94     1 */
> > > > 	bool                       ttrnw_valid;          /*    95     1 */
> > > 
> > > Linus has had negative things to say about lists of bools in
> > > structs. Use a bitfield?
> > 
> > Hmm, I agree, each bool is a waste of 7-bits. I tried the following:
> > 
> > struct arm_smmu_event {
> > 	u8				id;
> > 	u8				class;
> > 	u16				stag;
> > 
> > 	/* Group the boolean values together */
> > 	unsigned int stall:1;
> > 	unsigned int ssid_valid:1;
> > 	unsigned int privileged:1;
> > 	unsigned int instruction:1;
> > 	unsigned int s2:1;
> > 	unsigned int read:1;
> > 	unsigned int ttrnw:1;
> > 	unsigned int ttrnw_valid:1;
> 
> You can make this a u8

Ack, I'll update this with the v5 of this series.

> 
> > struct arm_smmu_event {
> > 	u8				id;
> > 	u8				class;
> > 	u16				stag;
> > 	
> > 	/* Group the boolean values together */
> > 	union {
> > 		struct {
> > 			unsigned int stall:1;
> > 			unsigned int ssid_valid:1;
> > 			unsigned int privileged:1;
> > 			unsigned int instruction:1;
> > 			unsigned int s2:1;
> > 			unsigned int read:1;
> > 			unsigned int ttrnw:1;
> > 			unsigned int ttrnw_valid:1;
> > 		};
> > 		u8 flags;
> 
> I wouldn't do this..

Ack.

> 
> Jason

Thanks,
Praan

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Will Deacon]

On Thu, Oct 24, 2024 at 12:00:47PM -0700, Nicolin Chen wrote:
> On Thu, Oct 24, 2024 at 02:15:12PM +0100, Will Deacon wrote:
> > > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > > +{
> > > +     struct arm_smmu_device *smmu = evt->smmu;
> > > +     char title[100] = {0};
> > > +     char mastr[100] = {0};
> > > +     char addrs[100] = {0};
> > > +     char flags[100] = {0};
> > > +     char other[50] = {0};
> > 
> > I haven't followed previous versions of this series in detail, but
> > allocating 450 bytes on the stack for this seems excessive to me.
> 
> If "450B" raises a red flag, should we do something about these?
> 
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c: In function ‘arm_smmu_atc_inv_master.isra’:
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2065:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c: In function ‘arm_smmu_sync_cd’:
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:1218:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c: In function ‘__arm_smmu_tlb_inv_range’:
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2213:1: warning: the frame size of 1072 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c: In function ‘arm_smmu_atc_inv_domain’:
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2118:1: warning: the frame size of 1120 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> Seems that struct arm_smmu_cmdq_batch is too big. Any suggestion?

I've not seen those warnings before, but it would be good to address
them as well!

Will

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Will Deacon]

On Thu, Oct 24, 2024 at 02:14:42PM +0000, Pranjal Shrivastava wrote:
> On Thu, Oct 24, 2024 at 02:15:12PM +0100, Will Deacon wrote:
> > On Fri, Oct 18, 2024 at 06:00:21PM +0000, Pranjal Shrivastava wrote:
> > > Currently, the driver dumps the raw hex for a received event record.
> > > Improve this by leveraging `struct arm_smmu_event` for event fields
> > > and log human-readable event records with meaningful information.
> > > 
> > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > ---
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 109 ++++++++++++++++++--
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  13 +++
> > >  2 files changed, 113 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > index 2f1108e5de51..4477cf86cb8e 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > @@ -83,6 +83,34 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
> > >  	{ 0, NULL},
> > >  };
> > >  
> > > +static const char * const event_str[] = {
> > > +	/* Bad config events */
> > > +	[EVT_ID_BAD_SID_CONFIG] = "C_BAD_STREAMID",
> > > +	[EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
> > > +	[EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
> > > +	[EVT_ID_BAD_SSID_CONFIG] = "C_BAD_SUBSTREAMID",
> > > +	[EVT_ID_STREAM_DISABLED] = "F_STREAM_DISABLED",
> > > +
> > > +	/* Bad translation events */
> > > +	[EVT_ID_TRANSLATION_FAULT] = "F_TRANSLATION",
> > > +	[EVT_ID_ADDR_SIZE_FAULT] = "F_ADDR_SIZE",
> > > +	[EVT_ID_ACCESS_FAULT] = "F_ACCESS",
> > > +	[EVT_ID_PERMISSION_FAULT] = "F_PERMISSION",
> > > +
> > > +	/* Bad fetch events */
> > > +	[EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
> > > +	[EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
> > > +	[EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FAULT",
> > > +	[EVT_ID_MAX] = NULL,
> > > +};
> > > +
> > > +static const char * const event_class_str[] = {
> > > +	[0] = "CD fetch",
> > > +	[1] = "Stage 1 translation table fetch",
> > > +	[2] = "Input address caused fault ",
> > > +	[3] = "Reserved",
> > > +};
> > > +
> > >  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> > >  				    struct arm_smmu_device *smmu, u32 flags);
> > >  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
> > > @@ -1756,6 +1784,60 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> > >  	return rb_entry(node, struct arm_smmu_stream, node)->master;
> > >  }
> > >  
> > > +static void arm_smmu_dump_raw_event(struct arm_smmu_event *event)
> > > +{
> > > +	int i;
> > > +	struct arm_smmu_device *smmu = event->smmu;
> > > +
> > > +	dev_err(smmu->dev, "event 0x%02x received: master %s:\n",
> > > +		event->id, event->master_name);
> > > +
> > > +	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > > +		dev_err(smmu->dev, "\t0x%016llx\n", event->raw[i]);
> > > +}
> > > +
> > > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > > +{
> > > +	struct arm_smmu_device *smmu = evt->smmu;
> > > +	char title[100] = {0};
> > > +	char mastr[100] = {0};
> > > +	char addrs[100] = {0};
> > > +	char flags[100] = {0};
> > > +	char other[50] = {0};
> > 
> > I haven't followed previous versions of this series in detail, but
> > allocating 450 bytes on the stack for this seems excessive to me.
> 
> Hmm, I think we can reduce the title, mastr, addrs string to 50 chars
> and the `other` can be reduced to 30? Or maybe we can avoid printing the
> `stag` and `stall` fields altogether which should reduce it by 200 bytes
> We can save another 20 bytes by reducing the `flags` len to 80 bytes,
> overall saving us 220 bytes of stack space.
> 
> I introduced this in v3 [1] based on suggestions in v2 reviews [2].
> I can revert to the approach followed in v2 [3] as well.
> 
> LMK what's your vote?

Keeping the strings concise is one thing and we should do that regardless.
However, I'm questioning (a) why we need so many arrays rather than just
e.g. char linebuf[...] and (b) why it needs to be on the stack at all.

Will

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Pranjal Shrivastava]

On Tue, Oct 29, 2024 at 06:53:46PM +0000, Will Deacon wrote:
> On Thu, Oct 24, 2024 at 02:14:42PM +0000, Pranjal Shrivastava wrote:
> > On Thu, Oct 24, 2024 at 02:15:12PM +0100, Will Deacon wrote:
> > > On Fri, Oct 18, 2024 at 06:00:21PM +0000, Pranjal Shrivastava wrote:
> > > > Currently, the driver dumps the raw hex for a received event record.
> > > > Improve this by leveraging `struct arm_smmu_event` for event fields
> > > > and log human-readable event records with meaningful information.
> > > > 
> > > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > > ---
> > > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 109 ++++++++++++++++++--
> > > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  13 +++
> > > >  2 files changed, 113 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > index 2f1108e5de51..4477cf86cb8e 100644
> > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > @@ -83,6 +83,34 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
> > > >  	{ 0, NULL},
> > > >  };
> > > >  
> > > > +static const char * const event_str[] = {
> > > > +	/* Bad config events */
> > > > +	[EVT_ID_BAD_SID_CONFIG] = "C_BAD_STREAMID",
> > > > +	[EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
> > > > +	[EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
> > > > +	[EVT_ID_BAD_SSID_CONFIG] = "C_BAD_SUBSTREAMID",
> > > > +	[EVT_ID_STREAM_DISABLED] = "F_STREAM_DISABLED",
> > > > +
> > > > +	/* Bad translation events */
> > > > +	[EVT_ID_TRANSLATION_FAULT] = "F_TRANSLATION",
> > > > +	[EVT_ID_ADDR_SIZE_FAULT] = "F_ADDR_SIZE",
> > > > +	[EVT_ID_ACCESS_FAULT] = "F_ACCESS",
> > > > +	[EVT_ID_PERMISSION_FAULT] = "F_PERMISSION",
> > > > +
> > > > +	/* Bad fetch events */
> > > > +	[EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
> > > > +	[EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
> > > > +	[EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FAULT",
> > > > +	[EVT_ID_MAX] = NULL,
> > > > +};
> > > > +
> > > > +static const char * const event_class_str[] = {
> > > > +	[0] = "CD fetch",
> > > > +	[1] = "Stage 1 translation table fetch",
> > > > +	[2] = "Input address caused fault ",
> > > > +	[3] = "Reserved",
> > > > +};
> > > > +
> > > >  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> > > >  				    struct arm_smmu_device *smmu, u32 flags);
> > > >  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
> > > > @@ -1756,6 +1784,60 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> > > >  	return rb_entry(node, struct arm_smmu_stream, node)->master;
> > > >  }
> > > >  
> > > > +static void arm_smmu_dump_raw_event(struct arm_smmu_event *event)
> > > > +{
> > > > +	int i;
> > > > +	struct arm_smmu_device *smmu = event->smmu;
> > > > +
> > > > +	dev_err(smmu->dev, "event 0x%02x received: master %s:\n",
> > > > +		event->id, event->master_name);
> > > > +
> > > > +	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > > > +		dev_err(smmu->dev, "\t0x%016llx\n", event->raw[i]);
> > > > +}
> > > > +
> > > > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > > > +{
> > > > +	struct arm_smmu_device *smmu = evt->smmu;
> > > > +	char title[100] = {0};
> > > > +	char mastr[100] = {0};
> > > > +	char addrs[100] = {0};
> > > > +	char flags[100] = {0};
> > > > +	char other[50] = {0};
> > > 
> > > I haven't followed previous versions of this series in detail, but
> > > allocating 450 bytes on the stack for this seems excessive to me.
> > 
> > Hmm, I think we can reduce the title, mastr, addrs string to 50 chars
> > and the `other` can be reduced to 30? Or maybe we can avoid printing the
> > `stag` and `stall` fields altogether which should reduce it by 200 bytes
> > We can save another 20 bytes by reducing the `flags` len to 80 bytes,
> > overall saving us 220 bytes of stack space.
> > 
> > I introduced this in v3 [1] based on suggestions in v2 reviews [2].
> > I can revert to the approach followed in v2 [3] as well.
> > 
> > LMK what's your vote?
> 
> Keeping the strings concise is one thing and we should do that regardless.

Hmm.. the longest string is the one printing all the flags, an example:

[   85.410290]  Unpriv | Data | Write | S2 | Stage 1 translation table fetch | TTD Write

One idea to reduce this is, maybe just print a letter for abbreviate and
trim the event class string? Something like:

[   85.410290]  Un | D | W | S2 | S1 TT fetch | TTDW
[   85.410290]  Prv | I | R | S1 | S1 TT fetch | TTDR
[   85.410290]  Prv | D | R | S1 | Input addr fault

> However, I'm questioning (a) why we need so many arrays rather than just
> e.g. char linebuf[...] and (b) why it needs to be on the stack at all.
> 

a) The arrays were for breaking up the log in multiple parts and print
only the relevant parts for a particular type of event. Earlier we had
something similar to the linebuf (basically not needing the linebuf):

dev_err(smmu->dev, "Fault: %s client %s sid 0x%08x.0x%05x:\n\tiova = %#llx ipa = %#llx\n (%s%s%s%s%s%s)\n",
	evts[event->id], event->master_name, event->sid, event->ssid,
	event->iova, event->ipa,
	event->privileged ? "Priv " : "Unpriv ",
	event->instruction ? "Inst " : "Data ",
	event->read ? "Read " : "Write ",
	event->stage ? "S2 " : "S1 ", class_str[event->class],
	((event->id == EVT_ID_PERMISSION_FAULT) && 
	 eevent->class == EVTQ_1_CLASS_TT)) ?
	 (FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ?
	  " TTDRead" : " TTD Write") : "");

Since I had 3 different logs for 3 different categories of events, it
was decided to break up the logs into smaller parts and print only the
relevant ones during the review of the v2 patch[1].

b) Hmm, I was thinking around the re-entrancy of `arm_smmu_dump_event`?
Since all events are queued up and handled later.. I was thinking of a
situation where the 2 instances of the evtq_thread would be running on 2
cores simultaneously handling different events?

I haven't looked into the core interrupt handling but I'm assuming the
following situation:

1. Interrupt Occurs: When the hardware interrupt triggers, the CPU takes
the interrupt and executes the first half of the interrupt handler.

2 Thread is "Woken Up": After Acking the interrupt, he associated irq
thread will "wake up" i.e. the evtq_thread is  marked as "ready" to run.

3. Now, the scheduler sees the thread as "ready" and schedules it on a
free cpu say, on CPU0.

4. Second Interrupt (Before First Finishes): If another interrupt
for an event occurs before the first instance of the evtq_thread
has finished executing, it will again mark the thread as "ready".

5. The scheduler might now see evtq_thread is ready again and 
schedule it on a different CPU (say CPU1). Now you have two
instances of the same thread running concurrently..

I'll take a look into the core interrupt handling code to verify if the
above is even a possibility or some hallucination of mine.

But in case we are sure that the above wouldn't happen, I think we
can consider moving these buffers to global scope. I'd just like to
avoid the race conditions and locking around the log buffers.

> Will

Thanks,
Praan

[1] https://lore.kernel.org/linux-iommu/ZtAW0hVPFD6JbLTL@Asurada-Nvidia/

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Robin Murphy]

On 2024-10-18 7:00 pm, Pranjal Shrivastava wrote:
> Introduce `struct arm_smmu_event` to represent event records.
> Parse out relevant fields from raw event records for ease and
> use the new `struct arm_smmu_event` instead.
> 
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> Signed-off-by: Pranjal Shrivastava <praan@google.com>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 59 ++++++++++++++-------
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 20 +++++++
>   2 files changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 737c5b882355..2f1108e5de51 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1757,17 +1757,16 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
>   }
>   
>   /* IRQ and event handlers */
> -static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> +static int arm_smmu_handle_evt(struct arm_smmu_event *event)
>   {
>   	int ret = 0;
>   	u32 perm = 0;
>   	struct arm_smmu_master *master;
> -	bool ssid_valid = evt[0] & EVTQ_0_SSV;
> -	u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
>   	struct iopf_fault fault_evt = { };
> +	struct arm_smmu_device *smmu = event->smmu;
>   	struct iommu_fault *flt = &fault_evt.fault;
>   
> -	switch (FIELD_GET(EVTQ_0_ID, evt[0])) {
> +	switch (event->id) {
>   	case EVT_ID_TRANSLATION_FAULT:
>   	case EVT_ID_ADDR_SIZE_FAULT:
>   	case EVT_ID_ACCESS_FAULT:
> @@ -1777,35 +1776,35 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
>   		return -EOPNOTSUPP;
>   	}
>   
> -	if (!(evt[1] & EVTQ_1_STALL))
> +	if (!event->stall)
>   		return -EOPNOTSUPP;
>   
> -	if (evt[1] & EVTQ_1_RnW)
> +	if (event->read)
>   		perm |= IOMMU_FAULT_PERM_READ;
>   	else
>   		perm |= IOMMU_FAULT_PERM_WRITE;
>   
> -	if (evt[1] & EVTQ_1_InD)
> +	if (event->instruction)
>   		perm |= IOMMU_FAULT_PERM_EXEC;
>   
> -	if (evt[1] & EVTQ_1_PnU)
> +	if (event->privileged)
>   		perm |= IOMMU_FAULT_PERM_PRIV;
>   
>   	flt->type = IOMMU_FAULT_PAGE_REQ;
>   	flt->prm = (struct iommu_fault_page_request) {
>   		.flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> -		.grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> +		.grpid = event->stag,
>   		.perm = perm,
> -		.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> +		.addr = event->iova,
>   	};
>   
> -	if (ssid_valid) {
> +	if (event->ssid_valid) {
>   		flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> -		flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
> +		flt->prm.pasid = event->ssid;
>   	}
>   
>   	mutex_lock(&smmu->streams_mutex);
> -	master = arm_smmu_find_master(smmu, sid);
> +	master = arm_smmu_find_master(smmu, event->sid);
>   	if (!master) {
>   		ret = -EINVAL;
>   		goto out_unlock;
> @@ -1817,28 +1816,48 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
>   	return ret;
>   }
>   
> +static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> +					struct arm_smmu_event *event)

One would kind of expect "get event from raw" to take a raw thing and 
return an event... personally I'd still just inline this in 
arm_smmu_handle_evt() itself, but otherwise something like 
arm_smmu_decode_evt() would seem a more logical and obvious name at this 
point.

> +{
> +	/* Pick out the good stuff */
> +	event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> +	event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> +	event->ssid_valid = event->raw[0] & EVTQ_0_SSV;
> +	event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
> +	event->privileged = FIELD_GET(EVTQ_1_PnU, event->raw[1]);
> +	event->instruction = FIELD_GET(EVTQ_1_InD, event->raw[1]);
> +	event->s2 = FIELD_GET(EVTQ_1_S2, event->raw[1]);
> +	event->read = FIELD_GET(EVTQ_1_RnW, event->raw[1]);
> +	event->stag = FIELD_GET(EVTQ_1_STAG, event->raw[1]);
> +	event->stall = event->raw[1] & EVTQ_1_STALL;
> +	event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
> +	event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
> +	event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> +	event->smmu = smmu;

The SMMU pointer isn't part of the raw event record... TBH I'd be 
inclined to leave it entirely separate, but if you really do want to 
hide it in the arm_smmu_event, at least keep things simple and 
initialise it outside the loop - it's not like it's ever going to change 
between events.

Thanks,
Robin.

> +}
> +
>   static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>   {
>   	int i, ret;
> +	struct arm_smmu_event evt;
>   	struct arm_smmu_device *smmu = dev;
>   	struct arm_smmu_queue *q = &smmu->evtq.q;
>   	struct arm_smmu_ll_queue *llq = &q->llq;
>   	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
>   				      DEFAULT_RATELIMIT_BURST);
> -	u64 evt[EVTQ_ENT_DWORDS];
>   
>   	do {
> -		while (!queue_remove_raw(q, evt)) {
> -			u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> +		while (!queue_remove_raw(q, evt.raw)) {
>   
> -			ret = arm_smmu_handle_evt(smmu, evt);
> +			arm_smmu_get_event_from_raw(smmu, &evt);
> +			ret = arm_smmu_handle_evt(&evt);
>   			if (!ret || !__ratelimit(&rs))
>   				continue;
>   
> -			dev_info(smmu->dev, "event 0x%02x received:\n", id);
> -			for (i = 0; i < ARRAY_SIZE(evt); ++i)
> +			dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> +			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
>   				dev_info(smmu->dev, "\t0x%016llx\n",
> -					 (unsigned long long)evt[i]);
> +					(unsigned long long)evt.raw[i]);
>   
>   			cond_resched();
>   		}
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 1e9952ca989f..8a42d7b701fb 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -437,6 +437,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
>   
>   #define EVTQ_0_ID			GENMASK_ULL(7, 0)
>   
> +/* Events */
>   #define EVT_ID_TRANSLATION_FAULT	0x10
>   #define EVT_ID_ADDR_SIZE_FAULT		0x11
>   #define EVT_ID_ACCESS_FAULT		0x12
> @@ -452,6 +453,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
>   #define EVTQ_1_RnW			(1UL << 35)
>   #define EVTQ_1_S2			(1UL << 39)
>   #define EVTQ_1_CLASS			GENMASK_ULL(41, 40)
> +#define EVTQ_1_CLASS_TT			0x1
>   #define EVTQ_1_TT_READ			(1UL << 44)
>   #define EVTQ_2_ADDR			GENMASK_ULL(63, 0)
>   #define EVTQ_3_IPA			GENMASK_ULL(51, 12)
> @@ -771,6 +773,24 @@ struct arm_smmu_stream {
>   	struct rb_node			node;
>   };
>   
> +struct arm_smmu_event {
> +	struct arm_smmu_device		*smmu;
> +	u8				id;
> +	u8				class;
> +	u16				stag;
> +	u32				sid;
> +	u32				ssid;
> +	u64				iova;
> +	u64				ipa;
> +	u64				raw[EVTQ_ENT_DWORDS];
> +	bool				stall;
> +	bool				ssid_valid;
> +	bool				privileged;
> +	bool				instruction;
> +	bool				s2;
> +	bool				read;
> +};
> +
>   /* SMMU private data for each master */
>   struct arm_smmu_master {
>   	struct arm_smmu_device		*smmu;

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Robin Murphy]

On 2024-10-18 7:00 pm, Pranjal Shrivastava wrote:
> Currently, the driver dumps the raw hex for a received event record.
> Improve this by leveraging `struct arm_smmu_event` for event fields
> and log human-readable event records with meaningful information.
> 
> Signed-off-by: Pranjal Shrivastava <praan@google.com>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 109 ++++++++++++++++++--
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  13 +++
>   2 files changed, 113 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 2f1108e5de51..4477cf86cb8e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -83,6 +83,34 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>   	{ 0, NULL},
>   };
>   
> +static const char * const event_str[] = {
> +	/* Bad config events */
> +	[EVT_ID_BAD_SID_CONFIG] = "C_BAD_STREAMID",
> +	[EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
> +	[EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
> +	[EVT_ID_BAD_SSID_CONFIG] = "C_BAD_SUBSTREAMID",
> +	[EVT_ID_STREAM_DISABLED] = "F_STREAM_DISABLED",
> +
> +	/* Bad translation events */
> +	[EVT_ID_TRANSLATION_FAULT] = "F_TRANSLATION",
> +	[EVT_ID_ADDR_SIZE_FAULT] = "F_ADDR_SIZE",
> +	[EVT_ID_ACCESS_FAULT] = "F_ACCESS",
> +	[EVT_ID_PERMISSION_FAULT] = "F_PERMISSION",
> +
> +	/* Bad fetch events */
> +	[EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
> +	[EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
> +	[EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FAULT",
> +	[EVT_ID_MAX] = NULL,

"(null)" is not a particularly obvious or helpful description of an 
unknown/imp-def event.

> +};
> +
> +static const char * const event_class_str[] = {
> +	[0] = "CD fetch",
> +	[1] = "Stage 1 translation table fetch",
> +	[2] = "Input address caused fault ",

I'm wondering how to parse "caused fault" here, but either way it 
doesn't seem to line up with the other two descriptions.

> +	[3] = "Reserved",
> +};
> +
>   static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>   				    struct arm_smmu_device *smmu, u32 flags);
>   static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
> @@ -1756,6 +1784,60 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
>   	return rb_entry(node, struct arm_smmu_stream, node)->master;
>   }
>   
> +static void arm_smmu_dump_raw_event(struct arm_smmu_event *event)
> +{
> +	int i;
> +	struct arm_smmu_device *smmu = event->smmu;
> +
> +	dev_err(smmu->dev, "event 0x%02x received: master %s:\n",

I'm mildly wary of automated parsing tools scanning for things like 
IOMMU faults, so I think it would be safest to leave any existing 
messages exactly as they are unless there's a particularly compelling 
reason to change them. And I don't believe that redundantly showing the 
same thing twice in one report is that.

Arguably more personal preference, but as before with the v2 driver, I 
also think it seems neater and more logical to show the raw data first 
in the context of the notification, and then have the decode follow as a 
clarification.

> +		event->id, event->master_name);
> +
> +	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> +		dev_err(smmu->dev, "\t0x%016llx\n", event->raw[i]);
> +}
> +
> +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> +{
> +	struct arm_smmu_device *smmu = evt->smmu;
> +	char title[100] = {0};
> +	char mastr[100] = {0};
> +	char addrs[100] = {0};
> +	char flags[100] = {0};
> +	char other[50] = {0};
> +
> +	if (!__ratelimit(rs))
> +		return;
> +
> +	snprintf(title, 100, "Unexpected event received: %s\n",	event_str[evt->id]);

...that also makes it easier to avoid redundant waffle like this and 
look less like two separate things have happened.

> +	snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",

TBH I find that StreamID format even harder to read than the raw event 
record itself :/

> +		 evt->master_name, evt->sid, evt->ssid);
> +
> +	switch (evt->id) {
> +	case EVT_ID_TRANSLATION_FAULT:
> +	case EVT_ID_ADDR_SIZE_FAULT:
> +	case EVT_ID_ACCESS_FAULT:
> +	case EVT_ID_PERMISSION_FAULT:
> +		snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
> +		snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
> +		snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
> +			 evt->privileged ? "Priv | " : "Unpriv | ",
> +			 evt->instruction ? "Inst | " : "Data | ",
> +			 evt->read ? "Read | " : "Write | ",
> +			 evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
> +			 evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");

Why needlessly duplicate all the constant parts which could be in the 
format string itself?

> +		break;
> +
> +	case EVT_ID_STE_FETCH_FAULT:
> +	case EVT_ID_CD_FETCH_FAULT:
> +	case EVT_ID_VMS_FETCH_FAULT:
> +		snprintf(addrs, 100, "\tFetch address: %#llx\n", evt->ipa);
> +		break;
> +	}
> +
> +	dev_err(smmu->dev, "%s%s%s%s%s", title, mastr, addrs, flags, other);
> +	arm_smmu_dump_raw_event(evt);
> +}
> +
>   /* IRQ and event handlers */
>   static int arm_smmu_handle_evt(struct arm_smmu_event *event)
>   {
> @@ -1819,6 +1901,8 @@ static int arm_smmu_handle_evt(struct arm_smmu_event *event)
>   static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
>   					struct arm_smmu_event *event)
>   {
> +	struct arm_smmu_master *master;
> +
>   	/* Pick out the good stuff */
>   	event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
>   	event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> @@ -1833,12 +1917,24 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
>   	event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
>   	event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
>   	event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> +	event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, event->raw[1]);
> +	event->ttrnw_valid = false;
>   	event->smmu = smmu;
> +	event->dev = NULL;
> +
> +	if (event->id == EVT_ID_PERMISSION_FAULT)
> +		event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
> +
> +	mutex_lock(&smmu->streams_mutex);
> +	master = arm_smmu_find_master(smmu, event->sid);
> +	if (master)
> +		event->dev = get_device(master->dev);
> +	mutex_unlock(&smmu->streams_mutex);
> +	event->master_name = event->dev ? dev_name(event->dev) : "(unassigned sid)";

As I said before, this shouldn't be here; we've saved the device, so we 
can derive its name at the point where we actually need its name.

Thanks,
Robin.

>   }
>   
>   static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>   {
> -	int i, ret;
>   	struct arm_smmu_event evt;
>   	struct arm_smmu_device *smmu = dev;
>   	struct arm_smmu_queue *q = &smmu->evtq.q;
> @@ -1850,15 +1946,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>   		while (!queue_remove_raw(q, evt.raw)) {
>   
>   			arm_smmu_get_event_from_raw(smmu, &evt);
> -			ret = arm_smmu_handle_evt(&evt);
> -			if (!ret || !__ratelimit(&rs))
> -				continue;
> -
> -			dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> -			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> -				dev_info(smmu->dev, "\t0x%016llx\n",
> -					(unsigned long long)evt.raw[i]);
> +			if (arm_smmu_handle_evt(&evt))
> +				arm_smmu_dump_event(&evt, &rs);
>   
> +			put_device(evt.dev);
>   			cond_resched();
>   		}
>   
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 8a42d7b701fb..820b08f872af 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -438,10 +438,19 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
>   #define EVTQ_0_ID			GENMASK_ULL(7, 0)
>   
>   /* Events */
> +#define EVT_ID_BAD_SID_CONFIG		0x02
> +#define EVT_ID_STE_FETCH_FAULT		0x03
> +#define EVT_ID_BAD_STE_CONFIG		0x04
> +#define EVT_ID_STREAM_DISABLED		0x06
> +#define EVT_ID_BAD_SSID_CONFIG		0x08
> +#define EVT_ID_CD_FETCH_FAULT		0x09
> +#define EVT_ID_BAD_CD_CONFIG		0x0a
>   #define EVT_ID_TRANSLATION_FAULT	0x10
>   #define EVT_ID_ADDR_SIZE_FAULT		0x11
>   #define EVT_ID_ACCESS_FAULT		0x12
>   #define EVT_ID_PERMISSION_FAULT		0x13
> +#define EVT_ID_VMS_FETCH_FAULT		0x25
> +#define EVT_ID_MAX			0xff
>   
>   #define EVTQ_0_SSV			(1UL << 11)
>   #define EVTQ_0_SSID			GENMASK_ULL(31, 12)
> @@ -774,7 +783,9 @@ struct arm_smmu_stream {
>   };
>   
>   struct arm_smmu_event {
> +	const char			*master_name;
>   	struct arm_smmu_device		*smmu;
> +	struct device			*dev;
>   	u8				id;
>   	u8				class;
>   	u16				stag;
> @@ -789,6 +800,8 @@ struct arm_smmu_event {
>   	bool				instruction;
>   	bool				s2;
>   	bool				read;
> +	bool				ttrnw;
> +	bool				ttrnw_valid;
>   };
>   
>   /* SMMU private data for each master */

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Fri, Nov 01, 2024 at 02:41:07PM +0000, Robin Murphy wrote:
> On 2024-10-18 7:00 pm, Pranjal Shrivastava wrote:
> > Introduce `struct arm_smmu_event` to represent event records.
> > Parse out relevant fields from raw event records for ease and
> > use the new `struct arm_smmu_event` instead.
> > 
> > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 59 ++++++++++++++-------
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 20 +++++++
> >   2 files changed, 59 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 737c5b882355..2f1108e5de51 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -1757,17 +1757,16 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> >   }
> >   /* IRQ and event handlers */
> > -static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > +static int arm_smmu_handle_evt(struct arm_smmu_event *event)
> >   {
> >   	int ret = 0;
> >   	u32 perm = 0;
> >   	struct arm_smmu_master *master;
> > -	bool ssid_valid = evt[0] & EVTQ_0_SSV;
> > -	u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> >   	struct iopf_fault fault_evt = { };
> > +	struct arm_smmu_device *smmu = event->smmu;
> >   	struct iommu_fault *flt = &fault_evt.fault;
> > -	switch (FIELD_GET(EVTQ_0_ID, evt[0])) {
> > +	switch (event->id) {
> >   	case EVT_ID_TRANSLATION_FAULT:
> >   	case EVT_ID_ADDR_SIZE_FAULT:
> >   	case EVT_ID_ACCESS_FAULT:
> > @@ -1777,35 +1776,35 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >   		return -EOPNOTSUPP;
> >   	}
> > -	if (!(evt[1] & EVTQ_1_STALL))
> > +	if (!event->stall)
> >   		return -EOPNOTSUPP;
> > -	if (evt[1] & EVTQ_1_RnW)
> > +	if (event->read)
> >   		perm |= IOMMU_FAULT_PERM_READ;
> >   	else
> >   		perm |= IOMMU_FAULT_PERM_WRITE;
> > -	if (evt[1] & EVTQ_1_InD)
> > +	if (event->instruction)
> >   		perm |= IOMMU_FAULT_PERM_EXEC;
> > -	if (evt[1] & EVTQ_1_PnU)
> > +	if (event->privileged)
> >   		perm |= IOMMU_FAULT_PERM_PRIV;
> >   	flt->type = IOMMU_FAULT_PAGE_REQ;
> >   	flt->prm = (struct iommu_fault_page_request) {
> >   		.flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> > -		.grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> > +		.grpid = event->stag,
> >   		.perm = perm,
> > -		.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> > +		.addr = event->iova,
> >   	};
> > -	if (ssid_valid) {
> > +	if (event->ssid_valid) {
> >   		flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> > -		flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
> > +		flt->prm.pasid = event->ssid;
> >   	}
> >   	mutex_lock(&smmu->streams_mutex);
> > -	master = arm_smmu_find_master(smmu, sid);
> > +	master = arm_smmu_find_master(smmu, event->sid);
> >   	if (!master) {
> >   		ret = -EINVAL;
> >   		goto out_unlock;
> > @@ -1817,28 +1816,48 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >   	return ret;
> >   }
> > +static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> > +					struct arm_smmu_event *event)
> 
> One would kind of expect "get event from raw" to take a raw thing and return
> an event... personally I'd still just inline this in arm_smmu_handle_evt()

Hmm.. I think we can do both of those things.

We can kzalloc struct arm_smmu_event and return it, that should also
reduce the stack size. However, I'm unsure if that'd slow things down
because kzalloc may be a little more expensive than a local variable..

For the inlining, just to ensure I understand, we're looking to keep the
old arm_smmu_handle_evt(smmu, raw_evt) as is and then decode the event
within arm_smmu_handle_evt before we start handling it, right?

I'm fine with both of the suggestions above, let me know your vote?

> itself, but otherwise something like arm_smmu_decode_evt() would seem a more
> logical and obvious name at this point.
> 
> > +{
> > +	/* Pick out the good stuff */
> > +	event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> > +	event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > +	event->ssid_valid = event->raw[0] & EVTQ_0_SSV;
> > +	event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
> > +	event->privileged = FIELD_GET(EVTQ_1_PnU, event->raw[1]);
> > +	event->instruction = FIELD_GET(EVTQ_1_InD, event->raw[1]);
> > +	event->s2 = FIELD_GET(EVTQ_1_S2, event->raw[1]);
> > +	event->read = FIELD_GET(EVTQ_1_RnW, event->raw[1]);
> > +	event->stag = FIELD_GET(EVTQ_1_STAG, event->raw[1]);
> > +	event->stall = event->raw[1] & EVTQ_1_STALL;
> > +	event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
> > +	event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
> > +	event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> > +	event->smmu = smmu;
> 
> The SMMU pointer isn't part of the raw event record... TBH I'd be inclined
> to leave it entirely separate, but if you really do want to hide it in the
> arm_smmu_event, at least keep things simple and initialise it outside the
> loop - it's not like it's ever going to change between events.

Yea.. honestly, just init-ing one member at a different time makes it
look a little weird if we allocate `arm_smmu_event` dynamically.
If we go ahead with allocating the `arm_smmu_event` using k*alloc, I
think it's best to remove the `smmu` member from the struct and pass it
around in functions.

> 
> Thanks,
> Robin.
> 

Thanks,
Praan

> > +}
> > +
> >   static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >   {
> >   	int i, ret;
> > +	struct arm_smmu_event evt;
> >   	struct arm_smmu_device *smmu = dev;
> >   	struct arm_smmu_queue *q = &smmu->evtq.q;
> >   	struct arm_smmu_ll_queue *llq = &q->llq;
> >   	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> >   				      DEFAULT_RATELIMIT_BURST);
> > -	u64 evt[EVTQ_ENT_DWORDS];
> >   	do {
> > -		while (!queue_remove_raw(q, evt)) {
> > -			u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> > +		while (!queue_remove_raw(q, evt.raw)) {
> > -			ret = arm_smmu_handle_evt(smmu, evt);
> > +			arm_smmu_get_event_from_raw(smmu, &evt);
> > +			ret = arm_smmu_handle_evt(&evt);
> >   			if (!ret || !__ratelimit(&rs))
> >   				continue;
> > -			dev_info(smmu->dev, "event 0x%02x received:\n", id);
> > -			for (i = 0; i < ARRAY_SIZE(evt); ++i)
> > +			dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > +			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> >   				dev_info(smmu->dev, "\t0x%016llx\n",
> > -					 (unsigned long long)evt[i]);
> > +					(unsigned long long)evt.raw[i]);
> >   			cond_resched();
> >   		}
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > index 1e9952ca989f..8a42d7b701fb 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -437,6 +437,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> >   #define EVTQ_0_ID			GENMASK_ULL(7, 0)
> > +/* Events */
> >   #define EVT_ID_TRANSLATION_FAULT	0x10
> >   #define EVT_ID_ADDR_SIZE_FAULT		0x11
> >   #define EVT_ID_ACCESS_FAULT		0x12
> > @@ -452,6 +453,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> >   #define EVTQ_1_RnW			(1UL << 35)
> >   #define EVTQ_1_S2			(1UL << 39)
> >   #define EVTQ_1_CLASS			GENMASK_ULL(41, 40)
> > +#define EVTQ_1_CLASS_TT			0x1
> >   #define EVTQ_1_TT_READ			(1UL << 44)
> >   #define EVTQ_2_ADDR			GENMASK_ULL(63, 0)
> >   #define EVTQ_3_IPA			GENMASK_ULL(51, 12)
> > @@ -771,6 +773,24 @@ struct arm_smmu_stream {
> >   	struct rb_node			node;
> >   };
> > +struct arm_smmu_event {
> > +	struct arm_smmu_device		*smmu;
> > +	u8				id;
> > +	u8				class;
> > +	u16				stag;
> > +	u32				sid;
> > +	u32				ssid;
> > +	u64				iova;
> > +	u64				ipa;
> > +	u64				raw[EVTQ_ENT_DWORDS];
> > +	bool				stall;
> > +	bool				ssid_valid;
> > +	bool				privileged;
> > +	bool				instruction;
> > +	bool				s2;
> > +	bool				read;
> > +};
> > +
> >   /* SMMU private data for each master */
> >   struct arm_smmu_master {
> >   	struct arm_smmu_device		*smmu;
> 

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Pranjal Shrivastava]

On Fri, Nov 01, 2024 at 03:05:24PM +0000, Robin Murphy wrote:
> On 2024-10-18 7:00 pm, Pranjal Shrivastava wrote:
> > Currently, the driver dumps the raw hex for a received event record.
> > Improve this by leveraging `struct arm_smmu_event` for event fields
> > and log human-readable event records with meaningful information.
> > 
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 109 ++++++++++++++++++--
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  13 +++
> >   2 files changed, 113 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 2f1108e5de51..4477cf86cb8e 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -83,6 +83,34 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
> >   	{ 0, NULL},
> >   };
> > +static const char * const event_str[] = {
> > +	/* Bad config events */
> > +	[EVT_ID_BAD_SID_CONFIG] = "C_BAD_STREAMID",
> > +	[EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
> > +	[EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
> > +	[EVT_ID_BAD_SSID_CONFIG] = "C_BAD_SUBSTREAMID",
> > +	[EVT_ID_STREAM_DISABLED] = "F_STREAM_DISABLED",
> > +
> > +	/* Bad translation events */
> > +	[EVT_ID_TRANSLATION_FAULT] = "F_TRANSLATION",
> > +	[EVT_ID_ADDR_SIZE_FAULT] = "F_ADDR_SIZE",
> > +	[EVT_ID_ACCESS_FAULT] = "F_ACCESS",
> > +	[EVT_ID_PERMISSION_FAULT] = "F_PERMISSION",
> > +
> > +	/* Bad fetch events */
> > +	[EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
> > +	[EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
> > +	[EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FAULT",
> > +	[EVT_ID_MAX] = NULL,
> 
> "(null)" is not a particularly obvious or helpful description of an
> unknown/imp-def event.

We can either add all the event strings or have a null check before we
print, i.e. if the sting fetched is NULL we can print "UNKNOWN" or
something. I like the latter approach better, LMK what you think?

> 
> > +};
> > +
> > +static const char * const event_class_str[] = {
> > +	[0] = "CD fetch",
> > +	[1] = "Stage 1 translation table fetch",
> > +	[2] = "Input address caused fault ",
> 
> I'm wondering how to parse "caused fault" here, but either way it doesn't
> seem to line up with the other two descriptions.
> 

Hmm, this was added as per the review comments in v2 [1], it was decided
to keep the spec-defined strings for event_class as is, if we have a
strong preference against this, then maybe we can go ahead with the
following spec-defined abbreviations:

static const char * const class_str[] = {
	[0] = "CD",  // or "CD Fetch"
	[1] = "TTD", // or "S1 TT fetch"
	[2] = "IN",
	[3] = "RES",
};

> > +	[3] = "Reserved",
> > +};
> > +
> >   static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> >   				    struct arm_smmu_device *smmu, u32 flags);
> >   static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
> > @@ -1756,6 +1784,60 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> >   	return rb_entry(node, struct arm_smmu_stream, node)->master;
> >   }
> > +static void arm_smmu_dump_raw_event(struct arm_smmu_event *event)
> > +{
> > +	int i;
> > +	struct arm_smmu_device *smmu = event->smmu;
> > +
> > +	dev_err(smmu->dev, "event 0x%02x received: master %s:\n",
> 
> I'm mildly wary of automated parsing tools scanning for things like IOMMU
> faults, so I think it would be safest to leave any existing messages exactly
> as they are unless there's a particularly compelling reason to change them.
> And I don't believe that redundantly showing the same thing twice in one
> report is that.

Ack. Will leave this as is.

> 
> Arguably more personal preference, but as before with the v2 driver, I also
> think it seems neater and more logical to show the raw data first in the
> context of the notification, and then have the decode follow as a
> clarification.
> 

Ack. I'll log the raw event first and then the decoded message.
However, in the legacy log, we use dev_err for each dword which *may* be
interrupted by other logs, so should we have some message explaining
that the following is just a decoded version of the above log? 
For example:

[   85.411681] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x13 received: master 0000:00:01.0:
[   85.412045] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000013
[   85.412347] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
[   85.412630] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000ffffc0d0
[   85.412921] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[   85.410290] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x13: F_PERMISSION
[   85.410290]  master: 0000:00:01.0 sid: 0x00000008.0x00000
[   85.410290]  iova = 0xffffc0d0 ipa = 0x0
[   85.410290]  Unpriv | Data | Write | S1 | Input address caused fault
[   85.410290]  STAG = 0x0 Stall = 0x0

Personally, I think the above log looks better..

> > +		event->id, event->master_name);
> > +
> > +	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > +		dev_err(smmu->dev, "\t0x%016llx\n", event->raw[i]);
> > +}
> > +
> > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > +{
> > +	struct arm_smmu_device *smmu = evt->smmu;
> > +	char title[100] = {0};
> > +	char mastr[100] = {0};
> > +	char addrs[100] = {0};
> > +	char flags[100] = {0};
> > +	char other[50] = {0};
> > +
> > +	if (!__ratelimit(rs))
> > +		return;
> > +
> > +	snprintf(title, 100, "Unexpected event received: %s\n",	event_str[evt->id]);
> 
> ...that also makes it easier to avoid redundant waffle like this and look
> less like two separate things have happened.
> 

Ack. Please refer to the log above, I think it improves this as well..

> > +	snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> 
> TBH I find that StreamID format even harder to read than the raw event
> record itself :/
> 

Ouch! Nicolin didn't like this too :( 
I was just trying to follow the format used in the arm_smmu_handle_ppr
logs. Let's log the streamID and SSID separately then?
Maybe something like:

[   85.410290]  master: 0000:00:01.0 sid: 0x00000008 ssid: 0x00000

> > +		 evt->master_name, evt->sid, evt->ssid);
> > +
> > +	switch (evt->id) {
> > +	case EVT_ID_TRANSLATION_FAULT:
> > +	case EVT_ID_ADDR_SIZE_FAULT:
> > +	case EVT_ID_ACCESS_FAULT:
> > +	case EVT_ID_PERMISSION_FAULT:
> > +		snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
> > +		snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
> > +		snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
> > +			 evt->privileged ? "Priv | " : "Unpriv | ",
> > +			 evt->instruction ? "Inst | " : "Data | ",
> > +			 evt->read ? "Read | " : "Write | ",
> > +			 evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
> > +			 evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");
> 
> Why needlessly duplicate all the constant parts which could be in the format
> string itself?
> 

Umm.. I'm not sure I understand? Are you referring to the spaces and
the ' | ' characters?

> > +		break;
> > +
> > +	case EVT_ID_STE_FETCH_FAULT:
> > +	case EVT_ID_CD_FETCH_FAULT:
> > +	case EVT_ID_VMS_FETCH_FAULT:
> > +		snprintf(addrs, 100, "\tFetch address: %#llx\n", evt->ipa);
> > +		break;
> > +	}
> > +
> > +	dev_err(smmu->dev, "%s%s%s%s%s", title, mastr, addrs, flags, other);
> > +	arm_smmu_dump_raw_event(evt);
> > +}
> > +
> >   /* IRQ and event handlers */
> >   static int arm_smmu_handle_evt(struct arm_smmu_event *event)
> >   {
> > @@ -1819,6 +1901,8 @@ static int arm_smmu_handle_evt(struct arm_smmu_event *event)
> >   static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> >   					struct arm_smmu_event *event)
> >   {
> > +	struct arm_smmu_master *master;
> > +
> >   	/* Pick out the good stuff */
> >   	event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> >   	event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > @@ -1833,12 +1917,24 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> >   	event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
> >   	event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
> >   	event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> > +	event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, event->raw[1]);
> > +	event->ttrnw_valid = false;
> >   	event->smmu = smmu;
> > +	event->dev = NULL;
> > +
> > +	if (event->id == EVT_ID_PERMISSION_FAULT)
> > +		event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
> > +
> > +	mutex_lock(&smmu->streams_mutex);
> > +	master = arm_smmu_find_master(smmu, event->sid);
> > +	if (master)
> > +		event->dev = get_device(master->dev);
> > +	mutex_unlock(&smmu->streams_mutex);
> > +	event->master_name = event->dev ? dev_name(event->dev) : "(unassigned sid)";
> 
> As I said before, this shouldn't be here; we've saved the device, so we can
> derive its name at the point where we actually need its name.

Ack, will move this within `arm_smmu_dump_event`

> 
> Thanks,
> Robin.
> 

Thanks,
Praan

[1] https://lore.kernel.org/linux-iommu/ZtZEFKnKasX6433r@Asurada-Nvidia/

> >   }
> >   static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >   {
> > -	int i, ret;
> >   	struct arm_smmu_event evt;
> >   	struct arm_smmu_device *smmu = dev;
> >   	struct arm_smmu_queue *q = &smmu->evtq.q;
> > @@ -1850,15 +1946,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >   		while (!queue_remove_raw(q, evt.raw)) {
> >   			arm_smmu_get_event_from_raw(smmu, &evt);
> > -			ret = arm_smmu_handle_evt(&evt);
> > -			if (!ret || !__ratelimit(&rs))
> > -				continue;
> > -
> > -			dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > -			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > -				dev_info(smmu->dev, "\t0x%016llx\n",
> > -					(unsigned long long)evt.raw[i]);
> > +			if (arm_smmu_handle_evt(&evt))
> > +				arm_smmu_dump_event(&evt, &rs);
> > +			put_device(evt.dev);
> >   			cond_resched();
> >   		}
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > index 8a42d7b701fb..820b08f872af 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -438,10 +438,19 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> >   #define EVTQ_0_ID			GENMASK_ULL(7, 0)
> >   /* Events */
> > +#define EVT_ID_BAD_SID_CONFIG		0x02
> > +#define EVT_ID_STE_FETCH_FAULT		0x03
> > +#define EVT_ID_BAD_STE_CONFIG		0x04
> > +#define EVT_ID_STREAM_DISABLED		0x06
> > +#define EVT_ID_BAD_SSID_CONFIG		0x08
> > +#define EVT_ID_CD_FETCH_FAULT		0x09
> > +#define EVT_ID_BAD_CD_CONFIG		0x0a
> >   #define EVT_ID_TRANSLATION_FAULT	0x10
> >   #define EVT_ID_ADDR_SIZE_FAULT		0x11
> >   #define EVT_ID_ACCESS_FAULT		0x12
> >   #define EVT_ID_PERMISSION_FAULT		0x13
> > +#define EVT_ID_VMS_FETCH_FAULT		0x25
> > +#define EVT_ID_MAX			0xff
> >   #define EVTQ_0_SSV			(1UL << 11)
> >   #define EVTQ_0_SSID			GENMASK_ULL(31, 12)
> > @@ -774,7 +783,9 @@ struct arm_smmu_stream {
> >   };
> >   struct arm_smmu_event {
> > +	const char			*master_name;
> >   	struct arm_smmu_device		*smmu;
> > +	struct device			*dev;
> >   	u8				id;
> >   	u8				class;
> >   	u16				stag;
> > @@ -789,6 +800,8 @@ struct arm_smmu_event {
> >   	bool				instruction;
> >   	bool				s2;
> >   	bool				read;
> > +	bool				ttrnw;
> > +	bool				ttrnw_valid;
> >   };
> >   /* SMMU private data for each master */
> 

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Daniel Mentz]

On Fri, Nov 1, 2024 at 8:08 AM Pranjal Shrivastava <praan@google.com> wrote:
>
> On Fri, Nov 01, 2024 at 02:41:07PM +0000, Robin Murphy wrote:
> > On 2024-10-18 7:00 pm, Pranjal Shrivastava wrote:
> > > Introduce `struct arm_smmu_event` to represent event records.
> > > Parse out relevant fields from raw event records for ease and
> > > use the new `struct arm_smmu_event` instead.
> > >
> > > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > ---
> > >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 59 ++++++++++++++-------
> > >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 20 +++++++
> > >   2 files changed, 59 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > index 737c5b882355..2f1108e5de51 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > @@ -1757,17 +1757,16 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> > >   }
> > >   /* IRQ and event handlers */
> > > -static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > > +static int arm_smmu_handle_evt(struct arm_smmu_event *event)
> > >   {
> > >     int ret = 0;
> > >     u32 perm = 0;
> > >     struct arm_smmu_master *master;
> > > -   bool ssid_valid = evt[0] & EVTQ_0_SSV;
> > > -   u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> > >     struct iopf_fault fault_evt = { };
> > > +   struct arm_smmu_device *smmu = event->smmu;
> > >     struct iommu_fault *flt = &fault_evt.fault;
> > > -   switch (FIELD_GET(EVTQ_0_ID, evt[0])) {
> > > +   switch (event->id) {
> > >     case EVT_ID_TRANSLATION_FAULT:
> > >     case EVT_ID_ADDR_SIZE_FAULT:
> > >     case EVT_ID_ACCESS_FAULT:
> > > @@ -1777,35 +1776,35 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > >             return -EOPNOTSUPP;
> > >     }
> > > -   if (!(evt[1] & EVTQ_1_STALL))
> > > +   if (!event->stall)
> > >             return -EOPNOTSUPP;
> > > -   if (evt[1] & EVTQ_1_RnW)
> > > +   if (event->read)
> > >             perm |= IOMMU_FAULT_PERM_READ;
> > >     else
> > >             perm |= IOMMU_FAULT_PERM_WRITE;
> > > -   if (evt[1] & EVTQ_1_InD)
> > > +   if (event->instruction)
> > >             perm |= IOMMU_FAULT_PERM_EXEC;
> > > -   if (evt[1] & EVTQ_1_PnU)
> > > +   if (event->privileged)
> > >             perm |= IOMMU_FAULT_PERM_PRIV;
> > >     flt->type = IOMMU_FAULT_PAGE_REQ;
> > >     flt->prm = (struct iommu_fault_page_request) {
> > >             .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> > > -           .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> > > +           .grpid = event->stag,
> > >             .perm = perm,
> > > -           .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> > > +           .addr = event->iova,
> > >     };
> > > -   if (ssid_valid) {
> > > +   if (event->ssid_valid) {
> > >             flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> > > -           flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
> > > +           flt->prm.pasid = event->ssid;
> > >     }
> > >     mutex_lock(&smmu->streams_mutex);
> > > -   master = arm_smmu_find_master(smmu, sid);
> > > +   master = arm_smmu_find_master(smmu, event->sid);
> > >     if (!master) {
> > >             ret = -EINVAL;
> > >             goto out_unlock;
> > > @@ -1817,28 +1816,48 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > >     return ret;
> > >   }
> > > +static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> > > +                                   struct arm_smmu_event *event)
> >
> > One would kind of expect "get event from raw" to take a raw thing and return
> > an event... personally I'd still just inline this in arm_smmu_handle_evt()
>
> Hmm.. I think we can do both of those things.
>
> We can kzalloc struct arm_smmu_event and return it, that should also
> reduce the stack size. However, I'm unsure if that'd slow things down
> because kzalloc may be a little more expensive than a local variable..

I don't think stack size is a concern here. Therefore, I would advise
against kzalloc. Keep it simple by allocating on the stack.

> For the inlining, just to ensure I understand, we're looking to keep the
> old arm_smmu_handle_evt(smmu, raw_evt) as is and then decode the event
> within arm_smmu_handle_evt before we start handling it, right?
>
> I'm fine with both of the suggestions above, let me know your vote?
>
> > itself, but otherwise something like arm_smmu_decode_evt() would seem a more
> > logical and obvious name at this point.
> >
> > > +{
> > > +   /* Pick out the good stuff */

I'd remove this comment.

> > > +   event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> > > +   event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > > +   event->ssid_valid = event->raw[0] & EVTQ_0_SSV;
> > > +   event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
> > > +   event->privileged = FIELD_GET(EVTQ_1_PnU, event->raw[1]);
> > > +   event->instruction = FIELD_GET(EVTQ_1_InD, event->raw[1]);
> > > +   event->s2 = FIELD_GET(EVTQ_1_S2, event->raw[1]);
> > > +   event->read = FIELD_GET(EVTQ_1_RnW, event->raw[1]);
> > > +   event->stag = FIELD_GET(EVTQ_1_STAG, event->raw[1]);
> > > +   event->stall = event->raw[1] & EVTQ_1_STALL;
> > > +   event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
> > > +   event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
> > > +   event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> > > +   event->smmu = smmu;
> >
> > The SMMU pointer isn't part of the raw event record... TBH I'd be inclined
> > to leave it entirely separate, but if you really do want to hide it in the
> > arm_smmu_event, at least keep things simple and initialise it outside the
> > loop - it's not like it's ever going to change between events.
>
> Yea.. honestly, just init-ing one member at a different time makes it
> look a little weird if we allocate `arm_smmu_event` dynamically.
> If we go ahead with allocating the `arm_smmu_event` using k*alloc, I
> think it's best to remove the `smmu` member from the struct and pass it
> around in functions.

I am in favor of removing the smmu member from struct arm_smmu_event.
That would be consistent with other structs like arm_smmu_cmdq and
arm_smmu_queue_poll.

>
> >
> > Thanks,
> > Robin.
> >
>
> Thanks,
> Praan
>
> > > +}
> > > +
> > >   static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> > >   {
> > >     int i, ret;
> > > +   struct arm_smmu_event evt;
> > >     struct arm_smmu_device *smmu = dev;
> > >     struct arm_smmu_queue *q = &smmu->evtq.q;
> > >     struct arm_smmu_ll_queue *llq = &q->llq;
> > >     static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> > >                                   DEFAULT_RATELIMIT_BURST);
> > > -   u64 evt[EVTQ_ENT_DWORDS];
> > >     do {
> > > -           while (!queue_remove_raw(q, evt)) {
> > > -                   u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> > > +           while (!queue_remove_raw(q, evt.raw)) {
> > > -                   ret = arm_smmu_handle_evt(smmu, evt);
> > > +                   arm_smmu_get_event_from_raw(smmu, &evt);
> > > +                   ret = arm_smmu_handle_evt(&evt);
> > >                     if (!ret || !__ratelimit(&rs))
> > >                             continue;
> > > -                   dev_info(smmu->dev, "event 0x%02x received:\n", id);
> > > -                   for (i = 0; i < ARRAY_SIZE(evt); ++i)
> > > +                   dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > > +                   for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > >                             dev_info(smmu->dev, "\t0x%016llx\n",
> > > -                                    (unsigned long long)evt[i]);
> > > +                                   (unsigned long long)evt.raw[i]);
> > >                     cond_resched();
> > >             }
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > index 1e9952ca989f..8a42d7b701fb 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > @@ -437,6 +437,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> > >   #define EVTQ_0_ID                 GENMASK_ULL(7, 0)
> > > +/* Events */

I'd remove this comment.

> > >   #define EVT_ID_TRANSLATION_FAULT  0x10
> > >   #define EVT_ID_ADDR_SIZE_FAULT            0x11
> > >   #define EVT_ID_ACCESS_FAULT               0x12
> > > @@ -452,6 +453,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> > >   #define EVTQ_1_RnW                        (1UL << 35)
> > >   #define EVTQ_1_S2                 (1UL << 39)
> > >   #define EVTQ_1_CLASS                      GENMASK_ULL(41, 40)
> > > +#define EVTQ_1_CLASS_TT                    0x1

Remove this if it's not used anywhere.

> > >   #define EVTQ_1_TT_READ                    (1UL << 44)
> > >   #define EVTQ_2_ADDR                       GENMASK_ULL(63, 0)
> > >   #define EVTQ_3_IPA                        GENMASK_ULL(51, 12)
> > > @@ -771,6 +773,24 @@ struct arm_smmu_stream {
> > >     struct rb_node                  node;
> > >   };
> > > +struct arm_smmu_event {
> > > +   struct arm_smmu_device          *smmu;
> > > +   u8                              id;
> > > +   u8                              class;
> > > +   u16                             stag;
> > > +   u32                             sid;
> > > +   u32                             ssid;
> > > +   u64                             iova;
> > > +   u64                             ipa;
> > > +   u64                             raw[EVTQ_ENT_DWORDS];
> > > +   bool                            stall;
> > > +   bool                            ssid_valid;

In arm_smmu_handle_ppr(), the name ssv is used instead of ssid_valid.
For consistent naming, consider ssv. I don't have a strong opinion on
this, though.

> > > +   bool                            privileged;
> > > +   bool                            instruction;
> > > +   bool                            s2;
> > > +   bool                            read;
> > > +};
> > > +
> > >   /* SMMU private data for each master */
> > >   struct arm_smmu_master {
> > >     struct arm_smmu_device          *smmu;
> >

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Daniel Mentz]

On Fri, Oct 18, 2024 at 11:01 AM Pranjal Shrivastava <praan@google.com> wrote:
>
> Currently, the driver dumps the raw hex for a received event record.
> Improve this by leveraging `struct arm_smmu_event` for event fields
> and log human-readable event records with meaningful information.
>
> Signed-off-by: Pranjal Shrivastava <praan@google.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 109 ++++++++++++++++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  13 +++
>  2 files changed, 113 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 2f1108e5de51..4477cf86cb8e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -83,6 +83,34 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>         { 0, NULL},
>  };
>
> +static const char * const event_str[] = {

I'd prefer the order in which they are specified in the Arm SMMUv3 arch spec.

> +       /* Bad config events */
> +       [EVT_ID_BAD_SID_CONFIG] = "C_BAD_STREAMID",
> +       [EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
> +       [EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
> +       [EVT_ID_BAD_SSID_CONFIG] = "C_BAD_SUBSTREAMID",
> +       [EVT_ID_STREAM_DISABLED] = "F_STREAM_DISABLED",
> +
> +       /* Bad translation events */
> +       [EVT_ID_TRANSLATION_FAULT] = "F_TRANSLATION",
> +       [EVT_ID_ADDR_SIZE_FAULT] = "F_ADDR_SIZE",
> +       [EVT_ID_ACCESS_FAULT] = "F_ACCESS",
> +       [EVT_ID_PERMISSION_FAULT] = "F_PERMISSION",
> +
> +       /* Bad fetch events */
> +       [EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
> +       [EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
> +       [EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FAULT",

F_VMS_FETCH not F_VMS_FAULT

> +       [EVT_ID_MAX] = NULL,

Have you considered checking if evt->id is smaller than
ARRAY_SIZE(event_str) rather than inflating even_str[] to contain 256
entries?

> +};
> +
> +static const char * const event_class_str[] = {
> +       [0] = "CD fetch",
> +       [1] = "Stage 1 translation table fetch",
> +       [2] = "Input address caused fault ",
> +       [3] = "Reserved",
> +};
> +
>  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>                                     struct arm_smmu_device *smmu, u32 flags);
>  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
> @@ -1756,6 +1784,60 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
>         return rb_entry(node, struct arm_smmu_stream, node)->master;
>  }
>
> +static void arm_smmu_dump_raw_event(struct arm_smmu_event *event)
> +{
> +       int i;
> +       struct arm_smmu_device *smmu = event->smmu;
> +
> +       dev_err(smmu->dev, "event 0x%02x received: master %s:\n",
> +               event->id, event->master_name);
> +
> +       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> +               dev_err(smmu->dev, "\t0x%016llx\n", event->raw[i]);
> +}
> +
> +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> +{
> +       struct arm_smmu_device *smmu = evt->smmu;
> +       char title[100] = {0};
> +       char mastr[100] = {0};
> +       char addrs[100] = {0};
> +       char flags[100] = {0};
> +       char other[50] = {0};
> +
> +       if (!__ratelimit(rs))
> +               return;
> +
> +       snprintf(title, 100, "Unexpected event received: %s\n", event_str[evt->id]);
> +       snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> +                evt->master_name, evt->sid, evt->ssid);
> +
> +       switch (evt->id) {
> +       case EVT_ID_TRANSLATION_FAULT:
> +       case EVT_ID_ADDR_SIZE_FAULT:
> +       case EVT_ID_ACCESS_FAULT:
> +       case EVT_ID_PERMISSION_FAULT:
> +               snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
> +               snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
> +               snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
> +                        evt->privileged ? "Priv | " : "Unpriv | ",
> +                        evt->instruction ? "Inst | " : "Data | ",
> +                        evt->read ? "Read | " : "Write | ",
> +                        evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
> +                        evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");
> +               break;
> +
> +       case EVT_ID_STE_FETCH_FAULT:
> +       case EVT_ID_CD_FETCH_FAULT:
> +       case EVT_ID_VMS_FETCH_FAULT:
> +               snprintf(addrs, 100, "\tFetch address: %#llx\n", evt->ipa);
> +               break;
> +       }
> +
> +       dev_err(smmu->dev, "%s%s%s%s%s", title, mastr, addrs, flags, other);
> +       arm_smmu_dump_raw_event(evt);
> +}
> +
>  /* IRQ and event handlers */
>  static int arm_smmu_handle_evt(struct arm_smmu_event *event)
>  {
> @@ -1819,6 +1901,8 @@ static int arm_smmu_handle_evt(struct arm_smmu_event *event)
>  static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
>                                         struct arm_smmu_event *event)
>  {
> +       struct arm_smmu_master *master;
> +
>         /* Pick out the good stuff */
>         event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
>         event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> @@ -1833,12 +1917,24 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
>         event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
>         event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
>         event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> +       event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, event->raw[1]);
> +       event->ttrnw_valid = false;
>         event->smmu = smmu;
> +       event->dev = NULL;
> +
> +       if (event->id == EVT_ID_PERMISSION_FAULT)
> +               event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
> +
> +       mutex_lock(&smmu->streams_mutex);
> +       master = arm_smmu_find_master(smmu, event->sid);
> +       if (master)
> +               event->dev = get_device(master->dev);
> +       mutex_unlock(&smmu->streams_mutex);
> +       event->master_name = event->dev ? dev_name(event->dev) : "(unassigned sid)";
>  }
>
>  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>  {
> -       int i, ret;
>         struct arm_smmu_event evt;
>         struct arm_smmu_device *smmu = dev;
>         struct arm_smmu_queue *q = &smmu->evtq.q;
> @@ -1850,15 +1946,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>                 while (!queue_remove_raw(q, evt.raw)) {
>
>                         arm_smmu_get_event_from_raw(smmu, &evt);
> -                       ret = arm_smmu_handle_evt(&evt);
> -                       if (!ret || !__ratelimit(&rs))
> -                               continue;
> -
> -                       dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> -                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> -                               dev_info(smmu->dev, "\t0x%016llx\n",
> -                                       (unsigned long long)evt.raw[i]);
> +                       if (arm_smmu_handle_evt(&evt))
> +                               arm_smmu_dump_event(&evt, &rs);
>
> +                       put_device(evt.dev);
>                         cond_resched();
>                 }
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 8a42d7b701fb..820b08f872af 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -438,10 +438,19 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
>  #define EVTQ_0_ID                      GENMASK_ULL(7, 0)
>
>  /* Events */
> +#define EVT_ID_BAD_SID_CONFIG          0x02

I find that EVT_ID_BAD_STREAMID_CONFIG would be more consistent with
the spec and what's already in this header file.

> +#define EVT_ID_STE_FETCH_FAULT         0x03
> +#define EVT_ID_BAD_STE_CONFIG          0x04
> +#define EVT_ID_STREAM_DISABLED         0x06

EVT_ID_STREAM_DISABLED_FAULT

> +#define EVT_ID_BAD_SSID_CONFIG         0x08

EVT_ID_BAD_SUBSTREAMID_CONFIG

> +#define EVT_ID_CD_FETCH_FAULT          0x09
> +#define EVT_ID_BAD_CD_CONFIG           0x0a
>  #define EVT_ID_TRANSLATION_FAULT       0x10
>  #define EVT_ID_ADDR_SIZE_FAULT         0x11
>  #define EVT_ID_ACCESS_FAULT            0x12
>  #define EVT_ID_PERMISSION_FAULT                0x13
> +#define EVT_ID_VMS_FETCH_FAULT         0x25
> +#define EVT_ID_MAX                     0xff
>
>  #define EVTQ_0_SSV                     (1UL << 11)
>  #define EVTQ_0_SSID                    GENMASK_ULL(31, 12)
> @@ -774,7 +783,9 @@ struct arm_smmu_stream {
>  };
>
>  struct arm_smmu_event {
> +       const char                      *master_name;
>         struct arm_smmu_device          *smmu;
> +       struct device                   *dev;
>         u8                              id;
>         u8                              class;
>         u16                             stag;
> @@ -789,6 +800,8 @@ struct arm_smmu_event {
>         bool                            instruction;
>         bool                            s2;
>         bool                            read;
> +       bool                            ttrnw;
> +       bool                            ttrnw_valid;
>  };
>
>  /* SMMU private data for each master */
> --
> 2.47.0.rc1.288.g06298d1525-goog
>
>

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Sun, Nov 03, 2024 at 09:25:52PM -0800, Daniel Mentz wrote:
> On Fri, Nov 1, 2024 at 8:08 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Fri, Nov 01, 2024 at 02:41:07PM +0000, Robin Murphy wrote:
> > > On 2024-10-18 7:00 pm, Pranjal Shrivastava wrote:
> > > > Introduce `struct arm_smmu_event` to represent event records.
> > > > Parse out relevant fields from raw event records for ease and
> > > > use the new `struct arm_smmu_event` instead.
> > > >
> > > > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > > ---
> > > >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 59 ++++++++++++++-------
> > > >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 20 +++++++
> > > >   2 files changed, 59 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > index 737c5b882355..2f1108e5de51 100644
> > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > @@ -1757,17 +1757,16 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> > > >   }
> > > >   /* IRQ and event handlers */
> > > > -static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > > > +static int arm_smmu_handle_evt(struct arm_smmu_event *event)
> > > >   {
> > > >     int ret = 0;
> > > >     u32 perm = 0;
> > > >     struct arm_smmu_master *master;
> > > > -   bool ssid_valid = evt[0] & EVTQ_0_SSV;
> > > > -   u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> > > >     struct iopf_fault fault_evt = { };
> > > > +   struct arm_smmu_device *smmu = event->smmu;
> > > >     struct iommu_fault *flt = &fault_evt.fault;
> > > > -   switch (FIELD_GET(EVTQ_0_ID, evt[0])) {
> > > > +   switch (event->id) {
> > > >     case EVT_ID_TRANSLATION_FAULT:
> > > >     case EVT_ID_ADDR_SIZE_FAULT:
> > > >     case EVT_ID_ACCESS_FAULT:
> > > > @@ -1777,35 +1776,35 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > > >             return -EOPNOTSUPP;
> > > >     }
> > > > -   if (!(evt[1] & EVTQ_1_STALL))
> > > > +   if (!event->stall)
> > > >             return -EOPNOTSUPP;
> > > > -   if (evt[1] & EVTQ_1_RnW)
> > > > +   if (event->read)
> > > >             perm |= IOMMU_FAULT_PERM_READ;
> > > >     else
> > > >             perm |= IOMMU_FAULT_PERM_WRITE;
> > > > -   if (evt[1] & EVTQ_1_InD)
> > > > +   if (event->instruction)
> > > >             perm |= IOMMU_FAULT_PERM_EXEC;
> > > > -   if (evt[1] & EVTQ_1_PnU)
> > > > +   if (event->privileged)
> > > >             perm |= IOMMU_FAULT_PERM_PRIV;
> > > >     flt->type = IOMMU_FAULT_PAGE_REQ;
> > > >     flt->prm = (struct iommu_fault_page_request) {
> > > >             .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> > > > -           .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> > > > +           .grpid = event->stag,
> > > >             .perm = perm,
> > > > -           .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> > > > +           .addr = event->iova,
> > > >     };
> > > > -   if (ssid_valid) {
> > > > +   if (event->ssid_valid) {
> > > >             flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> > > > -           flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
> > > > +           flt->prm.pasid = event->ssid;
> > > >     }
> > > >     mutex_lock(&smmu->streams_mutex);
> > > > -   master = arm_smmu_find_master(smmu, sid);
> > > > +   master = arm_smmu_find_master(smmu, event->sid);
> > > >     if (!master) {
> > > >             ret = -EINVAL;
> > > >             goto out_unlock;
> > > > @@ -1817,28 +1816,48 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > > >     return ret;
> > > >   }
> > > > +static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> > > > +                                   struct arm_smmu_event *event)
> > >
> > > One would kind of expect "get event from raw" to take a raw thing and return
> > > an event... personally I'd still just inline this in arm_smmu_handle_evt()
> >
> > Hmm.. I think we can do both of those things.
> >
> > We can kzalloc struct arm_smmu_event and return it, that should also
> > reduce the stack size. However, I'm unsure if that'd slow things down
> > because kzalloc may be a little more expensive than a local variable..
> 
> I don't think stack size is a concern here. Therefore, I would advise
> against kzalloc. Keep it simple by allocating on the stack.
> 

Ack. That's my preference as well, in that case I'll just rename it to
"arm_smmu_decode_event" as suggested by Robin.

> > For the inlining, just to ensure I understand, we're looking to keep the
> > old arm_smmu_handle_evt(smmu, raw_evt) as is and then decode the event
> > within arm_smmu_handle_evt before we start handling it, right?
> >
> > I'm fine with both of the suggestions above, let me know your vote?
> >
> > > itself, but otherwise something like arm_smmu_decode_evt() would seem a more
> > > logical and obvious name at this point.
> > >
> > > > +{
> > > > +   /* Pick out the good stuff */
> 
> I'd remove this comment.
> 

Ack.

> > > > +   event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> > > > +   event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > > > +   event->ssid_valid = event->raw[0] & EVTQ_0_SSV;
> > > > +   event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
> > > > +   event->privileged = FIELD_GET(EVTQ_1_PnU, event->raw[1]);
> > > > +   event->instruction = FIELD_GET(EVTQ_1_InD, event->raw[1]);
> > > > +   event->s2 = FIELD_GET(EVTQ_1_S2, event->raw[1]);
> > > > +   event->read = FIELD_GET(EVTQ_1_RnW, event->raw[1]);
> > > > +   event->stag = FIELD_GET(EVTQ_1_STAG, event->raw[1]);
> > > > +   event->stall = event->raw[1] & EVTQ_1_STALL;
> > > > +   event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
> > > > +   event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
> > > > +   event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> > > > +   event->smmu = smmu;
> > >
> > > The SMMU pointer isn't part of the raw event record... TBH I'd be inclined
> > > to leave it entirely separate, but if you really do want to hide it in the
> > > arm_smmu_event, at least keep things simple and initialise it outside the
> > > loop - it's not like it's ever going to change between events.
> >
> > Yea.. honestly, just init-ing one member at a different time makes it
> > look a little weird if we allocate `arm_smmu_event` dynamically.
> > If we go ahead with allocating the `arm_smmu_event` using k*alloc, I
> > think it's best to remove the `smmu` member from the struct and pass it
> > around in functions.
> 
> I am in favor of removing the smmu member from struct arm_smmu_event.
> That would be consistent with other structs like arm_smmu_cmdq and
> arm_smmu_queue_poll.
> 

Ack. In that case, let's just remove it. At this point, I don't see any
benefit in hiding it within `arm_smmu_event`. 

> >
> > >
> > > Thanks,
> > > Robin.
> > >
> >
> > Thanks,
> > Praan
> >
> > > > +}
> > > > +
> > > >   static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> > > >   {
> > > >     int i, ret;
> > > > +   struct arm_smmu_event evt;
> > > >     struct arm_smmu_device *smmu = dev;
> > > >     struct arm_smmu_queue *q = &smmu->evtq.q;
> > > >     struct arm_smmu_ll_queue *llq = &q->llq;
> > > >     static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> > > >                                   DEFAULT_RATELIMIT_BURST);
> > > > -   u64 evt[EVTQ_ENT_DWORDS];
> > > >     do {
> > > > -           while (!queue_remove_raw(q, evt)) {
> > > > -                   u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> > > > +           while (!queue_remove_raw(q, evt.raw)) {
> > > > -                   ret = arm_smmu_handle_evt(smmu, evt);
> > > > +                   arm_smmu_get_event_from_raw(smmu, &evt);
> > > > +                   ret = arm_smmu_handle_evt(&evt);
> > > >                     if (!ret || !__ratelimit(&rs))
> > > >                             continue;
> > > > -                   dev_info(smmu->dev, "event 0x%02x received:\n", id);
> > > > -                   for (i = 0; i < ARRAY_SIZE(evt); ++i)
> > > > +                   dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > > > +                   for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > > >                             dev_info(smmu->dev, "\t0x%016llx\n",
> > > > -                                    (unsigned long long)evt[i]);
> > > > +                                   (unsigned long long)evt.raw[i]);
> > > >                     cond_resched();
> > > >             }
> > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > > index 1e9952ca989f..8a42d7b701fb 100644
> > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > > @@ -437,6 +437,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> > > >   #define EVTQ_0_ID                 GENMASK_ULL(7, 0)
> > > > +/* Events */
> 
> I'd remove this comment.
> 

Ack.

> > > >   #define EVT_ID_TRANSLATION_FAULT  0x10
> > > >   #define EVT_ID_ADDR_SIZE_FAULT            0x11
> > > >   #define EVT_ID_ACCESS_FAULT               0x12
> > > > @@ -452,6 +453,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> > > >   #define EVTQ_1_RnW                        (1UL << 35)
> > > >   #define EVTQ_1_S2                 (1UL << 39)
> > > >   #define EVTQ_1_CLASS                      GENMASK_ULL(41, 40)
> > > > +#define EVTQ_1_CLASS_TT                    0x1
> 
> Remove this if it's not used anywhere.
> 

We're using it in `arm_smmu_get_event_from_raw` function for TTRnW:

+	if (event->id == EVT_ID_PERMISSION_FAULT)
+		event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);

> > > >   #define EVTQ_1_TT_READ                    (1UL << 44)
> > > >   #define EVTQ_2_ADDR                       GENMASK_ULL(63, 0)
> > > >   #define EVTQ_3_IPA                        GENMASK_ULL(51, 12)
> > > > @@ -771,6 +773,24 @@ struct arm_smmu_stream {
> > > >     struct rb_node                  node;
> > > >   };
> > > > +struct arm_smmu_event {
> > > > +   struct arm_smmu_device          *smmu;
> > > > +   u8                              id;
> > > > +   u8                              class;
> > > > +   u16                             stag;
> > > > +   u32                             sid;
> > > > +   u32                             ssid;
> > > > +   u64                             iova;
> > > > +   u64                             ipa;
> > > > +   u64                             raw[EVTQ_ENT_DWORDS];
> > > > +   bool                            stall;
> > > > +   bool                            ssid_valid;
> 
> In arm_smmu_handle_ppr(), the name ssv is used instead of ssid_valid.
> For consistent naming, consider ssv. I don't have a strong opinion on
> this, though.
> 

Hmm. I was referring to `arm_smmu_handle_evt` which used `ssid_valid`,
looks like there's some inconsistency across the file... Regardless, I
don't mind changing it to `ssv`, unless someone has an objection?

> > > > +   bool                            privileged;
> > > > +   bool                            instruction;
> > > > +   bool                            s2;
> > > > +   bool                            read;
> > > > +};
> > > > +
> > > >   /* SMMU private data for each master */
> > > >   struct arm_smmu_master {
> > > >     struct arm_smmu_device          *smmu;
> > >


Thanks,
Praan

Re: [PATCH v4 2/3] iommu/arm-smmu-v3: Log better event records

[Pranjal Shrivastava]

On Sun, Nov 03, 2024 at 10:36:36PM -0800, Daniel Mentz wrote:
> On Fri, Oct 18, 2024 at 11:01 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > Currently, the driver dumps the raw hex for a received event record.
> > Improve this by leveraging `struct arm_smmu_event` for event fields
> > and log human-readable event records with meaningful information.
> >
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 109 ++++++++++++++++++--
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  13 +++
> >  2 files changed, 113 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 2f1108e5de51..4477cf86cb8e 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -83,6 +83,34 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
> >         { 0, NULL},
> >  };
> >
> > +static const char * const event_str[] = {
> 
> I'd prefer the order in which they are specified in the Arm SMMUv3 arch spec.
> 

Ack. I had this initially as I grouped the event parsing in the
following types which has changed in the new versions. I'll re-order
them as they appear in the Arm SMMUv3 spec.

> > +       /* Bad config events */
> > +       [EVT_ID_BAD_SID_CONFIG] = "C_BAD_STREAMID",
> > +       [EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
> > +       [EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
> > +       [EVT_ID_BAD_SSID_CONFIG] = "C_BAD_SUBSTREAMID",
> > +       [EVT_ID_STREAM_DISABLED] = "F_STREAM_DISABLED",
> > +
> > +       /* Bad translation events */
> > +       [EVT_ID_TRANSLATION_FAULT] = "F_TRANSLATION",
> > +       [EVT_ID_ADDR_SIZE_FAULT] = "F_ADDR_SIZE",
> > +       [EVT_ID_ACCESS_FAULT] = "F_ACCESS",
> > +       [EVT_ID_PERMISSION_FAULT] = "F_PERMISSION",
> > +
> > +       /* Bad fetch events */
> > +       [EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
> > +       [EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
> > +       [EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FAULT",
> 
> F_VMS_FETCH not F_VMS_FAULT
> 

Ahh, yes! Thanks! I'll fix this.

> > +       [EVT_ID_MAX] = NULL,
> 
> Have you considered checking if evt->id is smaller than
> ARRAY_SIZE(event_str) rather than inflating even_str[] to contain 256
> entries?
> 

Yea.. a comparison of evt_id with array size wouldn't be correct as we
aren't listing all the events. Let's take the following example:

The `F_BAD_ATS_TREQ` has an event id of 0x05 but isn't defined above. In
this case, evt->id < ARRAY_SIZE(event_str) will be true.

The spec defines event IDs with inconsistent holes, for e.g. after 0x0b
we have 0x10, after 0x13 we have 0x20, after 0x21 we have 0x24.

Although, I agree that inflating the event_str[] to contain 256 entries
while most of them being NULL isn't good.

I think we can set the EVT_ID_MAX to 0x26 (the last spec-defined event
has an ID of 0x25). That way, we'll have 0x27 entries in the event_str
with a few holes. 

That way, if ((event_str[id] == NULL) || (id > ARRAY_SIZE(event_str))
we can print "Unknown Event". I believe that'd be better.

I'm assuming since we don't have anything in upstream for 
"Implementation-defined" events we can mark them as "UNKNOWN EVENTS"

> > +};
> > +
> > +static const char * const event_class_str[] = {
> > +       [0] = "CD fetch",
> > +       [1] = "Stage 1 translation table fetch",
> > +       [2] = "Input address caused fault ",
> > +       [3] = "Reserved",
> > +};
> > +
> >  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> >                                     struct arm_smmu_device *smmu, u32 flags);
> >  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
> > @@ -1756,6 +1784,60 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> >         return rb_entry(node, struct arm_smmu_stream, node)->master;
> >  }
> >
> > +static void arm_smmu_dump_raw_event(struct arm_smmu_event *event)
> > +{
> > +       int i;
> > +       struct arm_smmu_device *smmu = event->smmu;
> > +
> > +       dev_err(smmu->dev, "event 0x%02x received: master %s:\n",
> > +               event->id, event->master_name);
> > +
> > +       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > +               dev_err(smmu->dev, "\t0x%016llx\n", event->raw[i]);
> > +}
> > +
> > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > +{
> > +       struct arm_smmu_device *smmu = evt->smmu;
> > +       char title[100] = {0};
> > +       char mastr[100] = {0};
> > +       char addrs[100] = {0};
> > +       char flags[100] = {0};
> > +       char other[50] = {0};
> > +
> > +       if (!__ratelimit(rs))
> > +               return;
> > +
> > +       snprintf(title, 100, "Unexpected event received: %s\n", event_str[evt->id]);
> > +       snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> > +                evt->master_name, evt->sid, evt->ssid);
> > +
> > +       switch (evt->id) {
> > +       case EVT_ID_TRANSLATION_FAULT:
> > +       case EVT_ID_ADDR_SIZE_FAULT:
> > +       case EVT_ID_ACCESS_FAULT:
> > +       case EVT_ID_PERMISSION_FAULT:
> > +               snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
> > +               snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
> > +               snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
> > +                        evt->privileged ? "Priv | " : "Unpriv | ",
> > +                        evt->instruction ? "Inst | " : "Data | ",
> > +                        evt->read ? "Read | " : "Write | ",
> > +                        evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
> > +                        evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");
> > +               break;
> > +
> > +       case EVT_ID_STE_FETCH_FAULT:
> > +       case EVT_ID_CD_FETCH_FAULT:
> > +       case EVT_ID_VMS_FETCH_FAULT:
> > +               snprintf(addrs, 100, "\tFetch address: %#llx\n", evt->ipa);
> > +               break;
> > +       }
> > +
> > +       dev_err(smmu->dev, "%s%s%s%s%s", title, mastr, addrs, flags, other);
> > +       arm_smmu_dump_raw_event(evt);
> > +}
> > +
> >  /* IRQ and event handlers */
> >  static int arm_smmu_handle_evt(struct arm_smmu_event *event)
> >  {
> > @@ -1819,6 +1901,8 @@ static int arm_smmu_handle_evt(struct arm_smmu_event *event)
> >  static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> >                                         struct arm_smmu_event *event)
> >  {
> > +       struct arm_smmu_master *master;
> > +
> >         /* Pick out the good stuff */
> >         event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> >         event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > @@ -1833,12 +1917,24 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> >         event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
> >         event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
> >         event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> > +       event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, event->raw[1]);
> > +       event->ttrnw_valid = false;
> >         event->smmu = smmu;
> > +       event->dev = NULL;
> > +
> > +       if (event->id == EVT_ID_PERMISSION_FAULT)
> > +               event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
> > +
> > +       mutex_lock(&smmu->streams_mutex);
> > +       master = arm_smmu_find_master(smmu, event->sid);
> > +       if (master)
> > +               event->dev = get_device(master->dev);
> > +       mutex_unlock(&smmu->streams_mutex);
> > +       event->master_name = event->dev ? dev_name(event->dev) : "(unassigned sid)";
> >  }
> >
> >  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >  {
> > -       int i, ret;
> >         struct arm_smmu_event evt;
> >         struct arm_smmu_device *smmu = dev;
> >         struct arm_smmu_queue *q = &smmu->evtq.q;
> > @@ -1850,15 +1946,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >                 while (!queue_remove_raw(q, evt.raw)) {
> >
> >                         arm_smmu_get_event_from_raw(smmu, &evt);
> > -                       ret = arm_smmu_handle_evt(&evt);
> > -                       if (!ret || !__ratelimit(&rs))
> > -                               continue;
> > -
> > -                       dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > -                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> > -                               dev_info(smmu->dev, "\t0x%016llx\n",
> > -                                       (unsigned long long)evt.raw[i]);
> > +                       if (arm_smmu_handle_evt(&evt))
> > +                               arm_smmu_dump_event(&evt, &rs);
> >
> > +                       put_device(evt.dev);
> >                         cond_resched();
> >                 }
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > index 8a42d7b701fb..820b08f872af 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -438,10 +438,19 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> >  #define EVTQ_0_ID                      GENMASK_ULL(7, 0)
> >
> >  /* Events */
> > +#define EVT_ID_BAD_SID_CONFIG          0x02
> 
> I find that EVT_ID_BAD_STREAMID_CONFIG would be more consistent with
> the spec and what's already in this header file.
> 

Ack.

> > +#define EVT_ID_STE_FETCH_FAULT         0x03
> > +#define EVT_ID_BAD_STE_CONFIG          0x04
> > +#define EVT_ID_STREAM_DISABLED         0x06
> 
> EVT_ID_STREAM_DISABLED_FAULT
> 
> > +#define EVT_ID_BAD_SSID_CONFIG         0x08
> 
> EVT_ID_BAD_SUBSTREAMID_CONFIG
> 

Ack. Will update the names.

> > +#define EVT_ID_CD_FETCH_FAULT          0x09
> > +#define EVT_ID_BAD_CD_CONFIG           0x0a
> >  #define EVT_ID_TRANSLATION_FAULT       0x10
> >  #define EVT_ID_ADDR_SIZE_FAULT         0x11
> >  #define EVT_ID_ACCESS_FAULT            0x12
> >  #define EVT_ID_PERMISSION_FAULT                0x13
> > +#define EVT_ID_VMS_FETCH_FAULT         0x25
> > +#define EVT_ID_MAX                     0xff
> >
> >  #define EVTQ_0_SSV                     (1UL << 11)
> >  #define EVTQ_0_SSID                    GENMASK_ULL(31, 12)
> > @@ -774,7 +783,9 @@ struct arm_smmu_stream {
> >  };
> >
> >  struct arm_smmu_event {
> > +       const char                      *master_name;
> >         struct arm_smmu_device          *smmu;
> > +       struct device                   *dev;
> >         u8                              id;
> >         u8                              class;
> >         u16                             stag;
> > @@ -789,6 +800,8 @@ struct arm_smmu_event {
> >         bool                            instruction;
> >         bool                            s2;
> >         bool                            read;
> > +       bool                            ttrnw;
> > +       bool                            ttrnw_valid;
> >  };
> >
> >  /* SMMU private data for each master */
> > --
> > 2.47.0.rc1.288.g06298d1525-goog
> >
> >

Thanks,
Praan

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Daniel Mentz]

On Thu, Oct 24, 2024 at 10:02 AM Pranjal Shrivastava <praan@google.com> wrote:
>
> On Thu, Oct 24, 2024 at 02:11:48PM +0100, Will Deacon wrote:
> > On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> > > +struct arm_smmu_event {
> > > +   struct arm_smmu_device          *smmu;
> > > +   u8                              id;
> > > +   u8                              class;
> > > +   u16                             stag;
> > > +   u32                             sid;
> > > +   u32                             ssid;
> > > +   u64                             iova;
> > > +   u64                             ipa;
> > > +   u64                             raw[EVTQ_ENT_DWORDS];
> > > +   bool                            stall;
> > > +   bool                            ssid_valid;
> > > +   bool                            privileged;
> > > +   bool                            instruction;
> > > +   bool                            s2;
> > > +   bool                            read;
> > > +};
> >
> > minor nit, but it might be worth seeing what pahole says about the
> > layout of this structure in case you've got a bunch of wasted padding
> > thanks to the mixed-size fields.
>
> I ran pahole with this, looks like there's only one 4-byte hole but the
> cacheline aligment is bad (for a 64-byte cacheline):
>
> struct arm_smmu_event {
>         const char  *              master_name;          /*     0     8 */
>         struct arm_smmu_device *   smmu;                 /*     8     8 */
>         struct device *            dev;                  /*    16     8 */
>         u8                         id;                   /*    24     1 */
>         u8                         class;                /*    25     1 */
>         u16                        stag;                 /*    26     2 */
>         u32                        sid;                  /*    28     4 */
>         u32                        ssid;                 /*    32     4 */
>
>         /* XXX 4 bytes hole, try to pack */
>
>         u64                        iova;                 /*    40     8 */
>         u64                        ipa;                  /*    48     8 */
>         u64                        raw[4];               /*    56    32 */
>         /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
>         bool                       stall;                /*    88     1 */
>         bool                       ssid_valid;           /*    89     1 */
>         bool                       privileged;           /*    90     1 */
>         bool                       instruction;          /*    91     1 */
>         bool                       s2;                   /*    92     1 */
>         bool                       read;                 /*    93     1 */
>         bool                       ttrnw;                /*    94     1 */
>         bool                       ttrnw_valid;          /*    95     1 */
>
>         /* size: 96, cachelines: 2, members: 19 */
>         /* sum members: 92, holes: 1, sum holes: 4 */
>         /* last cacheline: 32 bytes */
> };
>
> I don't think we can do much about the 4-byte hole as the members occupy
> 92 bytes only. I assume a single 4-byte hole shall be fine?
>
> However, for cacheline aligment we can move the 3 top pointer-members,
> `master_name`,`smmu` & `dev` which improves the cacheline aligment:

Can you be more explicit about what is a good cacheline alignment? I'm
wondering if you're trying to ensure that raw[4] is contained within a
single cacheline as opposed to spanning two adjacent cachelines. I
doubt that this is worth optimizing for. Also, I'm wondering if this
analysis assumes that the base address of a struct arm_smmu_event
instance is cacheline aligned, which I am not sure is the case. I
would solely optimize for size.

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Mon, Nov 04, 2024 at 09:23:31AM -0800, Daniel Mentz wrote:
> On Thu, Oct 24, 2024 at 10:02 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Thu, Oct 24, 2024 at 02:11:48PM +0100, Will Deacon wrote:
> > > On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> > > > +struct arm_smmu_event {
> > > > +   struct arm_smmu_device          *smmu;
> > > > +   u8                              id;
> > > > +   u8                              class;
> > > > +   u16                             stag;
> > > > +   u32                             sid;
> > > > +   u32                             ssid;
> > > > +   u64                             iova;
> > > > +   u64                             ipa;
> > > > +   u64                             raw[EVTQ_ENT_DWORDS];
> > > > +   bool                            stall;
> > > > +   bool                            ssid_valid;
> > > > +   bool                            privileged;
> > > > +   bool                            instruction;
> > > > +   bool                            s2;
> > > > +   bool                            read;
> > > > +};
> > >
> > > minor nit, but it might be worth seeing what pahole says about the
> > > layout of this structure in case you've got a bunch of wasted padding
> > > thanks to the mixed-size fields.
> >
> > I ran pahole with this, looks like there's only one 4-byte hole but the
> > cacheline aligment is bad (for a 64-byte cacheline):
> >
> > struct arm_smmu_event {
> >         const char  *              master_name;          /*     0     8 */
> >         struct arm_smmu_device *   smmu;                 /*     8     8 */
> >         struct device *            dev;                  /*    16     8 */
> >         u8                         id;                   /*    24     1 */
> >         u8                         class;                /*    25     1 */
> >         u16                        stag;                 /*    26     2 */
> >         u32                        sid;                  /*    28     4 */
> >         u32                        ssid;                 /*    32     4 */
> >
> >         /* XXX 4 bytes hole, try to pack */
> >
> >         u64                        iova;                 /*    40     8 */
> >         u64                        ipa;                  /*    48     8 */
> >         u64                        raw[4];               /*    56    32 */
> >         /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> >         bool                       stall;                /*    88     1 */
> >         bool                       ssid_valid;           /*    89     1 */
> >         bool                       privileged;           /*    90     1 */
> >         bool                       instruction;          /*    91     1 */
> >         bool                       s2;                   /*    92     1 */
> >         bool                       read;                 /*    93     1 */
> >         bool                       ttrnw;                /*    94     1 */
> >         bool                       ttrnw_valid;          /*    95     1 */
> >
> >         /* size: 96, cachelines: 2, members: 19 */
> >         /* sum members: 92, holes: 1, sum holes: 4 */
> >         /* last cacheline: 32 bytes */
> > };
> >
> > I don't think we can do much about the 4-byte hole as the members occupy
> > 92 bytes only. I assume a single 4-byte hole shall be fine?
> >
> > However, for cacheline aligment we can move the 3 top pointer-members,
> > `master_name`,`smmu` & `dev` which improves the cacheline aligment:
> 
> Can you be more explicit about what is a good cacheline alignment? I'm
> wondering if you're trying to ensure that raw[4] is contained within a
> single cacheline as opposed to spanning two adjacent cachelines. I

I'm using a tool `pahole` [1] as suggested by Will earlier.
The tool prints information about the layout of structures, checking for
memory wastage due to padding and if the struct layout causes
mis-alignment with the caches.

The tool printed, "cacheline 1 boundary (64 bytes) was 24 bytes ago"
hinting that with the current layout, we are wasting 24-bytes of a
cacheline.

> doubt that this is worth optimizing for. Also, I'm wondering if this
> analysis assumes that the base address of a struct arm_smmu_event
> instance is cacheline aligned, which I am not sure is the case. I
> would solely optimize for size.

You're right, the tool does assume that the struct begins at a cacheline
I'm just sharing the analysis done by the tool to be able to finalize
the layout of `struct arm_smmu_event`.

IMO, if we don't have a strong opinion about this, there's no harm in
trying to layout the struct better even if it is a micro-optimization.

Although, I agree that size was the main concern here but if we have a
92-byte structure with 64-bit fields, we'll always have a 4-byte hole.

If we remove the `smmu` & `master_name` fields and pack all bools in
bitfields as discussed earlier, we'll have a size of 69 bytes,
(92 - 16 - 8 + 1), thus, we'd need a padding of 3 more bytes. Hence, the
size would be reduced to 72-bytes from 92-bytes. I guess, that's fine?

Thanks,
Praan

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Mon, Nov 04, 2024 at 06:16:48PM +0000, Pranjal Shrivastava wrote:
> On Mon, Nov 04, 2024 at 09:23:31AM -0800, Daniel Mentz wrote:
> > On Thu, Oct 24, 2024 at 10:02 AM Pranjal Shrivastava <praan@google.com> wrote:
> > >
> > > On Thu, Oct 24, 2024 at 02:11:48PM +0100, Will Deacon wrote:
> > > > On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> > > > > +struct arm_smmu_event {
> > > > > +   struct arm_smmu_device          *smmu;
> > > > > +   u8                              id;
> > > > > +   u8                              class;
> > > > > +   u16                             stag;
> > > > > +   u32                             sid;
> > > > > +   u32                             ssid;
> > > > > +   u64                             iova;
> > > > > +   u64                             ipa;
> > > > > +   u64                             raw[EVTQ_ENT_DWORDS];
> > > > > +   bool                            stall;
> > > > > +   bool                            ssid_valid;
> > > > > +   bool                            privileged;
> > > > > +   bool                            instruction;
> > > > > +   bool                            s2;
> > > > > +   bool                            read;
> > > > > +};
> > > >
> > > > minor nit, but it might be worth seeing what pahole says about the
> > > > layout of this structure in case you've got a bunch of wasted padding
> > > > thanks to the mixed-size fields.
> > >
> > > I ran pahole with this, looks like there's only one 4-byte hole but the
> > > cacheline aligment is bad (for a 64-byte cacheline):
> > >
> > > struct arm_smmu_event {
> > >         const char  *              master_name;          /*     0     8 */
> > >         struct arm_smmu_device *   smmu;                 /*     8     8 */
> > >         struct device *            dev;                  /*    16     8 */
> > >         u8                         id;                   /*    24     1 */
> > >         u8                         class;                /*    25     1 */
> > >         u16                        stag;                 /*    26     2 */
> > >         u32                        sid;                  /*    28     4 */
> > >         u32                        ssid;                 /*    32     4 */
> > >
> > >         /* XXX 4 bytes hole, try to pack */
> > >
> > >         u64                        iova;                 /*    40     8 */
> > >         u64                        ipa;                  /*    48     8 */
> > >         u64                        raw[4];               /*    56    32 */
> > >         /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> > >         bool                       stall;                /*    88     1 */
> > >         bool                       ssid_valid;           /*    89     1 */
> > >         bool                       privileged;           /*    90     1 */
> > >         bool                       instruction;          /*    91     1 */
> > >         bool                       s2;                   /*    92     1 */
> > >         bool                       read;                 /*    93     1 */
> > >         bool                       ttrnw;                /*    94     1 */
> > >         bool                       ttrnw_valid;          /*    95     1 */
> > >
> > >         /* size: 96, cachelines: 2, members: 19 */
> > >         /* sum members: 92, holes: 1, sum holes: 4 */
> > >         /* last cacheline: 32 bytes */
> > > };
> > >
> > > I don't think we can do much about the 4-byte hole as the members occupy
> > > 92 bytes only. I assume a single 4-byte hole shall be fine?
> > >
> > > However, for cacheline aligment we can move the 3 top pointer-members,
> > > `master_name`,`smmu` & `dev` which improves the cacheline aligment:
> > 
> > Can you be more explicit about what is a good cacheline alignment? I'm
> > wondering if you're trying to ensure that raw[4] is contained within a
> > single cacheline as opposed to spanning two adjacent cachelines. I
> 
> I'm using a tool `pahole` [1] as suggested by Will earlier.
> The tool prints information about the layout of structures, checking for
> memory wastage due to padding and if the struct layout causes
> mis-alignment with the caches.
> 
> The tool printed, "cacheline 1 boundary (64 bytes) was 24 bytes ago"
> hinting that with the current layout, we are wasting 24-bytes of a
> cacheline.
> 
> > doubt that this is worth optimizing for. Also, I'm wondering if this
> > analysis assumes that the base address of a struct arm_smmu_event
> > instance is cacheline aligned, which I am not sure is the case. I
> > would solely optimize for size.
> 
> You're right, the tool does assume that the struct begins at a cacheline
> I'm just sharing the analysis done by the tool to be able to finalize
> the layout of `struct arm_smmu_event`.
> 
> IMO, if we don't have a strong opinion about this, there's no harm in
> trying to layout the struct better even if it is a micro-optimization.
> 
> Although, I agree that size was the main concern here but if we have a
> 92-byte structure with 64-bit fields, we'll always have a 4-byte hole.
> 
> If we remove the `smmu` & `master_name` fields and pack all bools in
> bitfields as discussed earlier, we'll have a size of 69 bytes,
> (92 - 16 - 8 + 1), thus, we'd need a padding of 3 more bytes. Hence, the
> size would be reduced to 72-bytes from 92-bytes. I guess, that's fine?
> 
> Thanks,
> Praan

Missed the link tag! Apologies.

[1] https://linux.die.net/man/1/pahole

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Daniel Mentz]

On Mon, Nov 4, 2024 at 12:31 AM Pranjal Shrivastava <praan@google.com> wrote:
>
> On Sun, Nov 03, 2024 at 09:25:52PM -0800, Daniel Mentz wrote:
> > > > >   #define EVT_ID_TRANSLATION_FAULT  0x10
> > > > >   #define EVT_ID_ADDR_SIZE_FAULT            0x11
> > > > >   #define EVT_ID_ACCESS_FAULT               0x12
> > > > > @@ -452,6 +453,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> > > > >   #define EVTQ_1_RnW                        (1UL << 35)
> > > > >   #define EVTQ_1_S2                 (1UL << 39)
> > > > >   #define EVTQ_1_CLASS                      GENMASK_ULL(41, 40)
> > > > > +#define EVTQ_1_CLASS_TT                    0x1
> >
> > Remove this if it's not used anywhere.
> >
>
> We're using it in `arm_smmu_get_event_from_raw` function for TTRnW:
>
> +       if (event->id == EVT_ID_PERMISSION_FAULT)
> +               event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
>

The #define for EVTQ_1_CLASS_TT should be added in your second patch
with title "iommu/arm-smmu-v3: Log better event records" where it's
used for the first time.

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Daniel Mentz]

On Fri, Oct 18, 2024 at 11:00 AM Pranjal Shrivastava <praan@google.com> wrote:
>
> Introduce `struct arm_smmu_event` to represent event records.
> Parse out relevant fields from raw event records for ease and
> use the new `struct arm_smmu_event` instead.
>
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> Signed-off-by: Pranjal Shrivastava <praan@google.com>
> ---

> +struct arm_smmu_event {
> +       struct arm_smmu_device          *smmu;
> +       u8                              id;
> +       u8                              class;
> +       u16                             stag;
> +       u32                             sid;
> +       u32                             ssid;
> +       u64                             iova;
> +       u64                             ipa;
> +       u64                             raw[EVTQ_ENT_DWORDS];

Consider removing the member named raw from struct arm_smmu_event.
Compare this with struct arm_smmu_cmdq_ent and
arm_smmu_cmdq_build_cmd() which keep the encoded and decoded versions
separate.

> +       bool                            stall;
> +       bool                            ssid_valid;
> +       bool                            privileged;
> +       bool                            instruction;
> +       bool                            s2;
> +       bool                            read;
> +};

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Wed, Nov 06, 2024 at 04:10:01PM -0800, Daniel Mentz wrote:
> On Mon, Nov 4, 2024 at 12:31 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Sun, Nov 03, 2024 at 09:25:52PM -0800, Daniel Mentz wrote:
> > > > > >   #define EVT_ID_TRANSLATION_FAULT  0x10
> > > > > >   #define EVT_ID_ADDR_SIZE_FAULT            0x11
> > > > > >   #define EVT_ID_ACCESS_FAULT               0x12
> > > > > > @@ -452,6 +453,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> > > > > >   #define EVTQ_1_RnW                        (1UL << 35)
> > > > > >   #define EVTQ_1_S2                 (1UL << 39)
> > > > > >   #define EVTQ_1_CLASS                      GENMASK_ULL(41, 40)
> > > > > > +#define EVTQ_1_CLASS_TT                    0x1
> > >
> > > Remove this if it's not used anywhere.
> > >
> >
> > We're using it in `arm_smmu_get_event_from_raw` function for TTRnW:
> >
> > +       if (event->id == EVT_ID_PERMISSION_FAULT)
> > +               event->ttrnw_valid = (event->class == EVTQ_1_CLASS_TT);
> >
> 
> The #define for EVTQ_1_CLASS_TT should be added in your second patch
> with title "iommu/arm-smmu-v3: Log better event records" where it's
> used for the first time.

Ahh, yes! Apologies. Will move it to the right patch.

Thanks,
Praan

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Wed, Nov 06, 2024 at 04:16:19PM -0800, Daniel Mentz wrote:
> On Fri, Oct 18, 2024 at 11:00 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > Introduce `struct arm_smmu_event` to represent event records.
> > Parse out relevant fields from raw event records for ease and
> > use the new `struct arm_smmu_event` instead.
> >
> > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> 
> > +struct arm_smmu_event {
> > +       struct arm_smmu_device          *smmu;
> > +       u8                              id;
> > +       u8                              class;
> > +       u16                             stag;
> > +       u32                             sid;
> > +       u32                             ssid;
> > +       u64                             iova;
> > +       u64                             ipa;
> > +       u64                             raw[EVTQ_ENT_DWORDS];
> 
> Consider removing the member named raw from struct arm_smmu_event.
> Compare this with struct arm_smmu_cmdq_ent and
> arm_smmu_cmdq_build_cmd() which keep the encoded and decoded versions
> separate.

I had a similar implemntation in v3 [1] but it was decided [2]
to keep the "raw" event array within arm_smmu_event itself. Since
otherwise we'd have two variables, one pointing to the other when they
have the exact same scope and lifetime anyway.

Do we have a strong preference here?

> 
> > +       bool                            stall;
> > +       bool                            ssid_valid;
> > +       bool                            privileged;
> > +       bool                            instruction;
> > +       bool                            s2;
> > +       bool                            read;
> > +};

Thanks,
Praan

[1] https://lore.kernel.org/all/20240928005143.2378938-2-praan@google.com/
[2] https://lore.kernel.org/all/5eda3ba6-c35a-432b-be87-48bd8a0a3bf1@arm.com/

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Daniel Mentz]

On Thu, Nov 7, 2024 at 6:57 AM Pranjal Shrivastava <praan@google.com> wrote:
>
> On Wed, Nov 06, 2024 at 04:16:19PM -0800, Daniel Mentz wrote:
> > On Fri, Oct 18, 2024 at 11:00 AM Pranjal Shrivastava <praan@google.com> wrote:
> > >
> > > Introduce `struct arm_smmu_event` to represent event records.
> > > Parse out relevant fields from raw event records for ease and
> > > use the new `struct arm_smmu_event` instead.
> > >
> > > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > ---
> >
> > > +struct arm_smmu_event {
> > > +       struct arm_smmu_device          *smmu;
> > > +       u8                              id;
> > > +       u8                              class;
> > > +       u16                             stag;
> > > +       u32                             sid;
> > > +       u32                             ssid;
> > > +       u64                             iova;
> > > +       u64                             ipa;
> > > +       u64                             raw[EVTQ_ENT_DWORDS];
> >
> > Consider removing the member named raw from struct arm_smmu_event.
> > Compare this with struct arm_smmu_cmdq_ent and
> > arm_smmu_cmdq_build_cmd() which keep the encoded and decoded versions
> > separate.
>
> I had a similar implemntation in v3 [1] but it was decided [2]
> to keep the "raw" event array within arm_smmu_event itself. Since
> otherwise we'd have two variables, one pointing to the other when they
> have the exact same scope and lifetime anyway.

I understand that the concern in [2] was that "one [is] pointing to
the other". At the time, I think you had a pointer in struct
arm_smmu_event named raw, and the feedback was to embed the raw event
data in the structure instead of having a pointer. What I'm proposing
is to neither have a pointer nor embed it in the struct.

>
> Do we have a strong preference here?

Not a strong preference, but I'd prefer to have the raw event and the
decoded event separate.

>
> >
> > > +       bool                            stall;
> > > +       bool                            ssid_valid;
> > > +       bool                            privileged;
> > > +       bool                            instruction;
> > > +       bool                            s2;
> > > +       bool                            read;
> > > +};
>
> Thanks,
> Praan
>
> [1] https://lore.kernel.org/all/20240928005143.2378938-2-praan@google.com/
> [2] https://lore.kernel.org/all/5eda3ba6-c35a-432b-be87-48bd8a0a3bf1@arm.com/

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Mon, Nov 11, 2024 at 02:20:46PM -0800, Daniel Mentz wrote:
> On Thu, Nov 7, 2024 at 6:57 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Wed, Nov 06, 2024 at 04:16:19PM -0800, Daniel Mentz wrote:
> > > On Fri, Oct 18, 2024 at 11:00 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > >
> > > > Introduce `struct arm_smmu_event` to represent event records.
> > > > Parse out relevant fields from raw event records for ease and
> > > > use the new `struct arm_smmu_event` instead.
> > > >
> > > > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > > ---
> > >
> > > > +struct arm_smmu_event {
> > > > +       struct arm_smmu_device          *smmu;
> > > > +       u8                              id;
> > > > +       u8                              class;
> > > > +       u16                             stag;
> > > > +       u32                             sid;
> > > > +       u32                             ssid;
> > > > +       u64                             iova;
> > > > +       u64                             ipa;
> > > > +       u64                             raw[EVTQ_ENT_DWORDS];
> > >
> > > Consider removing the member named raw from struct arm_smmu_event.
> > > Compare this with struct arm_smmu_cmdq_ent and
> > > arm_smmu_cmdq_build_cmd() which keep the encoded and decoded versions
> > > separate.
> >
> > I had a similar implemntation in v3 [1] but it was decided [2]
> > to keep the "raw" event array within arm_smmu_event itself. Since
> > otherwise we'd have two variables, one pointing to the other when they
> > have the exact same scope and lifetime anyway.
> 
> I understand that the concern in [2] was that "one [is] pointing to
> the other". At the time, I think you had a pointer in struct
> arm_smmu_event named raw, and the feedback was to embed the raw event
> data in the structure instead of having a pointer. What I'm proposing
> is to neither have a pointer nor embed it in the struct.
> 
> >
> > Do we have a strong preference here?
> 
> Not a strong preference, but I'd prefer to have the raw event and the
> decoded event separate.
> 

I see. So, do you suggest that we should have the raw print as it is and
then the pretty print separately like the following:
---------------------------------------->8------------------------------------

	while (!queue_remove_raw(q, raw_evt)) {

		arm_smmu_decode_event(raw_evt, &evt);

		if (arm_smmu_handle_evt(smmu, &evt)) {
			dev_err(smmu->dev, "event 0x%02x received:\n", event->id);
			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
				dev_err(smmu->dev, "\t0x%016llx\n", raw_evt[i]);
			arm_smmu_dump_event(smmu, &evt, &rs);
		}

		put_device(evt.dev);
		cond_resched();
	}

---------------------------------------->8------------------------------------

OR should we pass the raw event to the dump_event function which, I
believe, is a little duplicative:

---------------------------------->8-----------------------------

	while (!queue_remove_raw(q, raw_evt)) {
		arm_smmu_decode_event(raw_evt, &evt);
		if (arm_smmu_handle_evt(smmu, &evt))
			arm_smmu_dump_event(smmu, &evt, raw_evt, &rs);
		put_device(evt.dev);
		cond_resched();
	}

---------------------------------->8-----------------------------

> >
> > >
> > > > +       bool                            stall;
> > > > +       bool                            ssid_valid;
> > > > +       bool                            privileged;
> > > > +       bool                            instruction;
> > > > +       bool                            s2;
> > > > +       bool                            read;
> > > > +};
> >
> > Thanks,
> > Praan
> >
> > [1] https://lore.kernel.org/all/20240928005143.2378938-2-praan@google.com/
> > [2] https://lore.kernel.org/all/5eda3ba6-c35a-432b-be87-48bd8a0a3bf1@arm.com/

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Daniel Mentz]

On Mon, Nov 11, 2024 at 4:52 PM Pranjal Shrivastava <praan@google.com> wrote:
>
> On Mon, Nov 11, 2024 at 02:20:46PM -0800, Daniel Mentz wrote:
> > On Thu, Nov 7, 2024 at 6:57 AM Pranjal Shrivastava <praan@google.com> wrote:
> > >
> > > On Wed, Nov 06, 2024 at 04:16:19PM -0800, Daniel Mentz wrote:
> > > > On Fri, Oct 18, 2024 at 11:00 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > > >
> > > > > Introduce `struct arm_smmu_event` to represent event records.
> > > > > Parse out relevant fields from raw event records for ease and
> > > > > use the new `struct arm_smmu_event` instead.
> > > > >
> > > > > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > > > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > > > ---
> > > >
> > > > > +struct arm_smmu_event {
> > > > > +       struct arm_smmu_device          *smmu;
> > > > > +       u8                              id;
> > > > > +       u8                              class;
> > > > > +       u16                             stag;
> > > > > +       u32                             sid;
> > > > > +       u32                             ssid;
> > > > > +       u64                             iova;
> > > > > +       u64                             ipa;
> > > > > +       u64                             raw[EVTQ_ENT_DWORDS];
> > > >
> > > > Consider removing the member named raw from struct arm_smmu_event.
> > > > Compare this with struct arm_smmu_cmdq_ent and
> > > > arm_smmu_cmdq_build_cmd() which keep the encoded and decoded versions
> > > > separate.
> > >
> > > I had a similar implemntation in v3 [1] but it was decided [2]
> > > to keep the "raw" event array within arm_smmu_event itself. Since
> > > otherwise we'd have two variables, one pointing to the other when they
> > > have the exact same scope and lifetime anyway.
> >
> > I understand that the concern in [2] was that "one [is] pointing to
> > the other". At the time, I think you had a pointer in struct
> > arm_smmu_event named raw, and the feedback was to embed the raw event
> > data in the structure instead of having a pointer. What I'm proposing
> > is to neither have a pointer nor embed it in the struct.
> >
> > >
> > > Do we have a strong preference here?
> >
> > Not a strong preference, but I'd prefer to have the raw event and the
> > decoded event separate.
> >
>
> I see. So, do you suggest that we should have the raw print as it is and
> then the pretty print separately like the following:
> ---------------------------------------->8------------------------------------
>
>         while (!queue_remove_raw(q, raw_evt)) {
>
>                 arm_smmu_decode_event(raw_evt, &evt);
>
>                 if (arm_smmu_handle_evt(smmu, &evt)) {
>                         dev_err(smmu->dev, "event 0x%02x received:\n", event->id);
>                         for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
>                                 dev_err(smmu->dev, "\t0x%016llx\n", raw_evt[i]);
>                         arm_smmu_dump_event(smmu, &evt, &rs);
>                 }
>
>                 put_device(evt.dev);
>                 cond_resched();
>         }
>
> ---------------------------------------->8------------------------------------
>
> OR should we pass the raw event to the dump_event function which, I
> believe, is a little duplicative:
>
> ---------------------------------->8-----------------------------
>
>         while (!queue_remove_raw(q, raw_evt)) {
>                 arm_smmu_decode_event(raw_evt, &evt);
>                 if (arm_smmu_handle_evt(smmu, &evt))
>                         arm_smmu_dump_event(smmu, &evt, raw_evt, &rs);
>                 put_device(evt.dev);
>                 cond_resched();
>         }
>
> ---------------------------------->8-----------------------------

Your second option would be my preference despite the fact that you
have to pass in an extra parameter. It makes it clear that
arm_smmu_dump_event prints both. I would make sure that the order of
&evt and raw_evt in the argument list matches the order in which
arm_smmu_dump_event is printing them.

>
> > >
> > > >
> > > > > +       bool                            stall;
> > > > > +       bool                            ssid_valid;
> > > > > +       bool                            privileged;
> > > > > +       bool                            instruction;
> > > > > +       bool                            s2;
> > > > > +       bool                            read;
> > > > > +};
> > >
> > > Thanks,
> > > Praan
> > >
> > > [1] https://lore.kernel.org/all/20240928005143.2378938-2-praan@google.com/
> > > [2] https://lore.kernel.org/all/5eda3ba6-c35a-432b-be87-48bd8a0a3bf1@arm.com/

Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava]

On Mon, Nov 11, 2024 at 08:01:58PM -0800, Daniel Mentz wrote:
> On Mon, Nov 11, 2024 at 4:52 PM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Mon, Nov 11, 2024 at 02:20:46PM -0800, Daniel Mentz wrote:
> > > On Thu, Nov 7, 2024 at 6:57 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > >
> > > > On Wed, Nov 06, 2024 at 04:16:19PM -0800, Daniel Mentz wrote:
> > > > > On Fri, Oct 18, 2024 at 11:00 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > > > >
> > > > > > Introduce `struct arm_smmu_event` to represent event records.
> > > > > > Parse out relevant fields from raw event records for ease and
> > > > > > use the new `struct arm_smmu_event` instead.
> > > > > >
> > > > > > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > > > > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > > > > ---
> > > > >
> > > > > > +struct arm_smmu_event {
> > > > > > +       struct arm_smmu_device          *smmu;
> > > > > > +       u8                              id;
> > > > > > +       u8                              class;
> > > > > > +       u16                             stag;
> > > > > > +       u32                             sid;
> > > > > > +       u32                             ssid;
> > > > > > +       u64                             iova;
> > > > > > +       u64                             ipa;
> > > > > > +       u64                             raw[EVTQ_ENT_DWORDS];
> > > > >
> > > > > Consider removing the member named raw from struct arm_smmu_event.
> > > > > Compare this with struct arm_smmu_cmdq_ent and
> > > > > arm_smmu_cmdq_build_cmd() which keep the encoded and decoded versions
> > > > > separate.
> > > >
> > > > I had a similar implemntation in v3 [1] but it was decided [2]
> > > > to keep the "raw" event array within arm_smmu_event itself. Since
> > > > otherwise we'd have two variables, one pointing to the other when they
> > > > have the exact same scope and lifetime anyway.
> > >
> > > I understand that the concern in [2] was that "one [is] pointing to
> > > the other". At the time, I think you had a pointer in struct
> > > arm_smmu_event named raw, and the feedback was to embed the raw event
> > > data in the structure instead of having a pointer. What I'm proposing
> > > is to neither have a pointer nor embed it in the struct.
> > >
> > > >
> > > > Do we have a strong preference here?
> > >
> > > Not a strong preference, but I'd prefer to have the raw event and the
> > > decoded event separate.
> > >
> >
> > I see. So, do you suggest that we should have the raw print as it is and
> > then the pretty print separately like the following:
> > ---------------------------------------->8------------------------------------
> >
> >         while (!queue_remove_raw(q, raw_evt)) {
> >
> >                 arm_smmu_decode_event(raw_evt, &evt);
> >
> >                 if (arm_smmu_handle_evt(smmu, &evt)) {
> >                         dev_err(smmu->dev, "event 0x%02x received:\n", event->id);
> >                         for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> >                                 dev_err(smmu->dev, "\t0x%016llx\n", raw_evt[i]);
> >                         arm_smmu_dump_event(smmu, &evt, &rs);
> >                 }
> >
> >                 put_device(evt.dev);
> >                 cond_resched();
> >         }
> >
> > ---------------------------------------->8------------------------------------
> >
> > OR should we pass the raw event to the dump_event function which, I
> > believe, is a little duplicative:
> >
> > ---------------------------------->8-----------------------------
> >
> >         while (!queue_remove_raw(q, raw_evt)) {
> >                 arm_smmu_decode_event(raw_evt, &evt);
> >                 if (arm_smmu_handle_evt(smmu, &evt))
> >                         arm_smmu_dump_event(smmu, &evt, raw_evt, &rs);
> >                 put_device(evt.dev);
> >                 cond_resched();
> >         }
> >
> > ---------------------------------->8-----------------------------
> 
> Your second option would be my preference despite the fact that you
> have to pass in an extra parameter. It makes it clear that
> arm_smmu_dump_event prints both. I would make sure that the order of
> &evt and raw_evt in the argument list matches the order in which
> arm_smmu_dump_event is printing them.
> 

Ack. I'll post a v5 soon and accommodate this with it! Thanks!

> >
> > > >
> > > > >
> > > > > > +       bool                            stall;
> > > > > > +       bool                            ssid_valid;
> > > > > > +       bool                            privileged;
> > > > > > +       bool                            instruction;
> > > > > > +       bool                            s2;
> > > > > > +       bool                            read;
> > > > > > +};
> > > >
> > > > Thanks,
> > > > Praan
> > > >
> > > > [1] https://lore.kernel.org/all/20240928005143.2378938-2-praan@google.com/
> > > > [2] https://lore.kernel.org/all/5eda3ba6-c35a-432b-be87-48bd8a0a3bf1@arm.com/