[PATCH v3 0/2] Parse out event records

[PATCH v3 0/2] 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.

v3
 * 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 (2):
  iommu/arm-smmu-v3: Print better events records
  iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 161 +++++++++++++++++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  30 ++++
 2 files changed, 167 insertions(+), 24 deletions(-)

-- 
2.46.1.824.gd892dcdcdd-goog

[PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Pranjal Shrivastava]

Currently, the driver dumps the hex for a received event. Improve this
by parsing out fields from an event for easier-to-read event records.
Introduce a `struct arm_smmu_event` to hold the parsed out information.

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 | 128 ++++++++++++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  30 +++++
 2 files changed, 151 insertions(+), 7 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..2b2949bd2d26 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 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,66 @@ 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_device *smmu,
+				    struct arm_smmu_event *event)
+{
+	int i;
+
+	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",
+			 (unsigned long long)event->raw[i]);
+}
+
+static void arm_smmu_dump_event(struct arm_smmu_device *smmu,
+				struct arm_smmu_event *event)
+{
+	char title[256] = {0};
+	char mastr[256] = {0};
+	char addrs[256] = {0};
+	char flags[256] = {0};
+	char other[256] = {0};
+
+	snprintf(title, 256, "Unexpected event received: %s\n",	event_str[event->id]);
+	snprintf(mastr, 256, "\tmaster: %s sid: 0x%08x.0x%05x\n",
+		 event->master_name, event->sid, event->ssid);
+
+	switch (event->id) {
+	case EVT_ID_TRANSLATION_FAULT:
+	case EVT_ID_ADDR_SIZE_FAULT:
+	case EVT_ID_ACCESS_FAULT:
+	case EVT_ID_PERMISSION_FAULT:
+		snprintf(addrs, 256, "\tiova = %#llx ipa = %#llx\n", event->iova, event->ipa);
+		snprintf(flags, 256, "\t%s%s%s%s%s%s\n",
+				event->privileged ? "Priv |" : "Unpriv |",
+				event->instruction ? " Inst |" : " Data |",
+				event->read ? " Read |" : " Write |",
+				event->s2 ? "S2|" : "S1|", class_str[event->class],
+				((event->id == EVT_ID_PERMISSION_FAULT) &&
+				 (event->class == EVTQ_1_CLASS_TT)) ?
+				  (FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ?
+				  "|TTD Read" : "|TTD Write") : "");
+		snprintf(other, 256, "\tSTAG = %#x Stall = %#x\n",
+				event->stag, event->stall);
+		break;
+
+	case EVT_ID_STE_FETCH_FAULT:
+	case EVT_ID_CD_FETCH_FAULT:
+	case EVT_ID_VMS_FETCH_FAULT:
+		snprintf(addrs, 256, "\tFetch address: %#llx\n", event->ipa);
+		break;
+	}
+
+	dev_err(smmu->dev, "%s%s%s%s%s", title, mastr,
+		strlen(addrs) ? addrs : "",
+		strlen(flags) ? flags : "",
+		strlen(other) ? other : "");
+
+	arm_smmu_dump_raw_event(smmu, event);
+}
+
 /* IRQ and event handlers */
 static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
 {
@@ -1817,9 +1905,39 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
 	return ret;
 }
 
+static void arm_smmu_get_evt_info(struct arm_smmu_device *smmu, u64 *evt,
+				  struct arm_smmu_event *event)
+{
+	/* Pick out the good stuff */
+	event->id = FIELD_GET(EVTQ_0_ID, evt[0]);
+	event->sid = FIELD_GET(EVTQ_0_SID, evt[0]);
+	event->ssid_valid = evt[0] & EVTQ_0_SSV;
+	event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, evt[0]) : IOMMU_NO_PASID;
+	event->class = FIELD_GET(EVTQ_1_CLASS, evt[1]);
+	event->iova = FIELD_GET(EVTQ_2_ADDR, evt[2]);
+	event->ipa = FIELD_GET(EVTQ_3_IPA, evt[3]);
+	event->privileged = FIELD_GET(EVTQ_1_PnU, evt[1]);
+	event->instruction = FIELD_GET(EVTQ_1_InD, evt[1]);
+	event->s2 = FIELD_GET(EVTQ_1_S2, evt[1]);
+	event->read = FIELD_GET(EVTQ_1_RnW, evt[1]);
+	event->stag = FIELD_GET(EVTQ_1_STAG, evt[1]);
+	event->stall = evt[1] & EVTQ_1_STALL;
+	event->raw = evt;
+
+	mutex_lock(&smmu->streams_mutex);
+	event->master = arm_smmu_find_master(smmu, event->sid);
+	mutex_unlock(&smmu->streams_mutex);
+
+	if (event->master)
+		event->master_name = dev_name(event->master->dev);
+	else
+		event->master_name = "(unassigned sid)";
+}
+
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
-	int i, ret;
+	int ret;
+	struct arm_smmu_event event;
 	struct arm_smmu_device *smmu = dev;
 	struct arm_smmu_queue *q = &smmu->evtq.q;
 	struct arm_smmu_ll_queue *llq = &q->llq;
@@ -1829,17 +1947,13 @@ 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]);
 
+			arm_smmu_get_evt_info(smmu, evt, &event);
 			ret = arm_smmu_handle_evt(smmu, 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, "\t0x%016llx\n",
-					 (unsigned long long)evt[i]);
-
+			arm_smmu_dump_event(smmu, &event);
 			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..743a544a50e9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -437,10 +437,20 @@ 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)
@@ -452,6 +462,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 +782,25 @@ struct arm_smmu_stream {
 	struct rb_node			node;
 };
 
+struct arm_smmu_event {
+	struct arm_smmu_master		*master;
+	const char			*master_name;
+	u8				id;
+	u8				class;
+	u16				stag;
+	u32				sid;
+	u32				ssid;
+	u64				iova;
+	u64				ipa;
+	u64				*raw;
+	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.46.1.824.gd892dcdcdd-goog

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Nicolin Chen]

On Sat, Sep 28, 2024 at 12:51:42AM +0000, Pranjal Shrivastava wrote:

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

s/class_str/event_class_str

> @@ -1756,6 +1784,66 @@ 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_device *smmu,
> +                                   struct arm_smmu_event *event)
> +{
> +       int i;
> +
> +       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",
> +                        (unsigned long long)event->raw[i]);

Though I know that is what we have been doing in the driver, I
wonder if we have to cast from u64 to ull?

> +static void arm_smmu_dump_event(struct arm_smmu_device *smmu,
> +                               struct arm_smmu_event *event)
> +{
> +       char title[256] = {0};
> +       char mastr[256] = {0};
> +       char addrs[256] = {0};
> +       char flags[256] = {0};
> +       char other[256] = {0};
> +
> +       snprintf(title, 256, "Unexpected event received: %s\n", event_str[event->id]);
> +       snprintf(mastr, 256, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> +                event->master_name, event->sid, event->ssid);
> +
> +       switch (event->id) {
> +       case EVT_ID_TRANSLATION_FAULT:
> +       case EVT_ID_ADDR_SIZE_FAULT:
> +       case EVT_ID_ACCESS_FAULT:
> +       case EVT_ID_PERMISSION_FAULT:
> +               snprintf(addrs, 256, "\tiova = %#llx ipa = %#llx\n", event->iova, event->ipa);
> +               snprintf(flags, 256, "\t%s%s%s%s%s%s\n",
> +                               event->privileged ? "Priv |" : "Unpriv |",
> +                               event->instruction ? " Inst |" : " Data |",
> +                               event->read ? " Read |" : " Write |",

Indentations are not matched with the snprintf(mastr line:
		snprintf(flags, 256, "\t%s%s%s%s%s%s\n",
			 event->privileged ? "Priv |" : "Unpriv |",
			 ...

> +                               event->s2 ? "S2|" : "S1|", class_str[event->class],

The previous flags have spaces: " Inst |", " Read |". So, maybe:
			 " S2 |" and " S1 |" here?

> +                               ((event->id == EVT_ID_PERMISSION_FAULT) &&
> +                                (event->class == EVTQ_1_CLASS_TT)) ?
> +                                 (FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ?
> +                                 "|TTD Read" : "|TTD Write") : "");

I'd try to avoid that nested ternary. Perhaps:
	char ttrnd[32] = {0};
	...
	case EVT_ID_PERMISSION_FAULT:
		if (event->class == EVTQ_1_CLASS_TT) {
			snprintf(ttrnd, 256, "%s",
				 FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ?
				 " | TTD Read" : " | TTD Write");
		}
		fallthrough;
	case EVT_ID_TRANSLATION_FAULT:
	case EVT_ID_ADDR_SIZE_FAULT:
	case EVT_ID_ACCESS_FAULT:
		snprintf(flags, 256, "\t%s%s%s%s%s%s\n",
			 event->privileged ? "Priv |" : "Unpriv |",
			 event->instruction ? " Inst |" : " Data |",
			 event->read ? " Read |" : " Write |",
			 event->s2 ? " S2 |" : " S1 |",
			 event_class_str[event->class], ttrnd);

> +               snprintf(other, 256, "\tSTAG = %#x Stall = %#x\n",
> +                               event->stag, event->stall);
> +               break;
> +
> +       case EVT_ID_STE_FETCH_FAULT:
> +       case EVT_ID_CD_FETCH_FAULT:
> +       case EVT_ID_VMS_FETCH_FAULT:
> +               snprintf(addrs, 256, "\tFetch address: %#llx\n", event->ipa);
> +               break;
> +       }
> +
> +       dev_err(smmu->dev, "%s%s%s%s%s", title, mastr,
> +               strlen(addrs) ? addrs : "",
> +               strlen(flags) ? flags : "",
> +               strlen(other) ? other : "");

It looks to me that you init-ed them well, so maybe strlen isn't
necessary?

> +
> +       arm_smmu_dump_raw_event(smmu, event);
> +}

I assume these prints would look good, yet, would you please try
triggering a FAULT to give us a showcase in the commit message?

> +static void arm_smmu_get_evt_info(struct arm_smmu_device *smmu, u64 *evt,
> +                                 struct arm_smmu_event *event)
> +{
> +       mutex_lock(&smmu->streams_mutex);
> +       event->master = arm_smmu_find_master(smmu, event->sid);
> +       mutex_unlock(&smmu->streams_mutex);
> +
> +       if (event->master)
> +               event->master_name = dev_name(event->master->dev);
> +       else
> +               event->master_name = "(unassigned sid)";

Likely safer for dev_name to be in the mutex too.

Thanks
Nicolin

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Pranjal Shrivastava]

On Mon, Sep 30, 2024 at 12:15:10PM -0700, Nicolin Chen wrote:
> On Sat, Sep 28, 2024 at 12:51:42AM +0000, Pranjal Shrivastava wrote:
> 
> > +static const char * const class_str[] = {
> > +       [0] = "CD fetch",
> > +       [1] = "Stage 1 translation table fetch",
> > +       [2] = "Input address caused fault ",
> > +       [3] = "Reserved",
> > +};
> 
> s/class_str/event_class_str
> 

Ack.

> > @@ -1756,6 +1784,66 @@ 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_device *smmu,
> > +                                   struct arm_smmu_event *event)
> > +{
> > +       int i;
> > +
> > +       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",
> > +                        (unsigned long long)event->raw[i]);
> 
> Though I know that is what we have been doing in the driver, I
> wonder if we have to cast from u64 to ull?
> 

I think we can do that? Are we avoiding potential compiler troubles?

> > +static void arm_smmu_dump_event(struct arm_smmu_device *smmu,
> > +                               struct arm_smmu_event *event)
> > +{
> > +       char title[256] = {0};
> > +       char mastr[256] = {0};
> > +       char addrs[256] = {0};
> > +       char flags[256] = {0};
> > +       char other[256] = {0};
> > +
> > +       snprintf(title, 256, "Unexpected event received: %s\n", event_str[event->id]);
> > +       snprintf(mastr, 256, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> > +                event->master_name, event->sid, event->ssid);
> > +
> > +       switch (event->id) {
> > +       case EVT_ID_TRANSLATION_FAULT:
> > +       case EVT_ID_ADDR_SIZE_FAULT:
> > +       case EVT_ID_ACCESS_FAULT:
> > +       case EVT_ID_PERMISSION_FAULT:
> > +               snprintf(addrs, 256, "\tiova = %#llx ipa = %#llx\n", event->iova, event->ipa);
> > +               snprintf(flags, 256, "\t%s%s%s%s%s%s\n",
> > +                               event->privileged ? "Priv |" : "Unpriv |",
> > +                               event->instruction ? " Inst |" : " Data |",
> > +                               event->read ? " Read |" : " Write |",
> 
> Indentations are not matched with the snprintf(mastr line:
> 		snprintf(flags, 256, "\t%s%s%s%s%s%s\n",
> 			 event->privileged ? "Priv |" : "Unpriv |",
> 			 ...
> 

Ack, will fix this in the next version..

> > +                               event->s2 ? "S2|" : "S1|", class_str[event->class],
> 
> The previous flags have spaces: " Inst |", " Read |". So, maybe:
> 			 " S2 |" and " S1 |" here?
> 

Ack. I'm guessing even in the worst case we'd get to 78 chars so adding
a space should be fine?

> > +                               ((event->id == EVT_ID_PERMISSION_FAULT) &&
> > +                                (event->class == EVTQ_1_CLASS_TT)) ?
> > +                                 (FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ?
> > +                                 "|TTD Read" : "|TTD Write") : "");
> 
> I'd try to avoid that nested ternary. Perhaps:
> 	char ttrnd[32] = {0};
> 	...
> 	case EVT_ID_PERMISSION_FAULT:
> 		if (event->class == EVTQ_1_CLASS_TT) {
> 			snprintf(ttrnd, 256, "%s",
> 				 FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ?
> 				 " | TTD Read" : " | TTD Write");
> 		}
> 		fallthrough;
> 	case EVT_ID_TRANSLATION_FAULT:
> 	case EVT_ID_ADDR_SIZE_FAULT:
> 	case EVT_ID_ACCESS_FAULT:
> 		snprintf(flags, 256, "\t%s%s%s%s%s%s\n",
> 			 event->privileged ? "Priv |" : "Unpriv |",
> 			 event->instruction ? " Inst |" : " Data |",
> 			 event->read ? " Read |" : " Write |",
> 			 event->s2 ? " S2 |" : " S1 |",
> 			 event_class_str[event->class], ttrnd);
> 

Hmm, I like that idea. I'll add something like this!

> > +               snprintf(other, 256, "\tSTAG = %#x Stall = %#x\n",
> > +                               event->stag, event->stall);
> > +               break;
> > +
> > +       case EVT_ID_STE_FETCH_FAULT:
> > +       case EVT_ID_CD_FETCH_FAULT:
> > +       case EVT_ID_VMS_FETCH_FAULT:
> > +               snprintf(addrs, 256, "\tFetch address: %#llx\n", event->ipa);
> > +               break;
> > +       }
> > +
> > +       dev_err(smmu->dev, "%s%s%s%s%s", title, mastr,
> > +               strlen(addrs) ? addrs : "",
> > +               strlen(flags) ? flags : "",
> > +               strlen(other) ? other : "");
> 
> It looks to me that you init-ed them well, so maybe strlen isn't
> necessary?
> 

True, missed that, will remove this.

> > +
> > +       arm_smmu_dump_raw_event(smmu, event);
> > +}
> 
> I assume these prints would look good, yet, would you please try
> triggering a FAULT to give us a showcase in the commit message?
> 

Sure, let me arrange for a sample FAULT log in the next version!

> > +static void arm_smmu_get_evt_info(struct arm_smmu_device *smmu, u64 *evt,
> > +                                 struct arm_smmu_event *event)
> > +{
> > +       mutex_lock(&smmu->streams_mutex);
> > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > +       mutex_unlock(&smmu->streams_mutex);
> > +
> > +       if (event->master)
> > +               event->master_name = dev_name(event->master->dev);
> > +       else
> > +               event->master_name = "(unassigned sid)";
> 
> Likely safer for dev_name to be in the mutex too.
> 

Ack.

> Thanks
> Nicolin

Thanks,
Pranjal

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Nicolin Chen]

On Tue, Oct 01, 2024 at 08:52:57PM +0000, Pranjal Shrivastava wrote:
> > > +static void arm_smmu_dump_raw_event(struct arm_smmu_device *smmu,
> > > +                                   struct arm_smmu_event *event)
> > > +{
> > > +       int i;
> > > +
> > > +       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",
> > > +                        (unsigned long long)event->raw[i]);
> >
> > Though I know that is what we have been doing in the driver, I
> > wonder if we have to cast from u64 to ull?
> >
> 
> I think we can do that?

I think so.

> Are we avoiding potential compiler troubles?

I assume so. But I am not sure what's the back story.

> > > +                               event->s2 ? "S2|" : "S1|", class_str[event->class],
> >
> > The previous flags have spaces: " Inst |", " Read |". So, maybe:
> >                        " S2 |" and " S1 |" here?
> >
> 
> Ack. I'm guessing even in the worst case we'd get to 78 chars so adding
> a space should be fine?

Well, if we want to manage the length. Perhaps printing them with
no space at all could be a choice too. Just, if we ever add space,
we'd better keep the consistency.

Thanks
Nicolin

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Jason Gunthorpe]

On Mon, Sep 30, 2024 at 12:15:10PM -0700, Nicolin Chen wrote:
> > +static void arm_smmu_get_evt_info(struct arm_smmu_device *smmu, u64 *evt,
> > +                                 struct arm_smmu_event *event)
> > +{
> > +       mutex_lock(&smmu->streams_mutex);
> > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > +       mutex_unlock(&smmu->streams_mutex);
> > +
> > +       if (event->master)
> > +               event->master_name = dev_name(event->master->dev);
> > +       else
> > +               event->master_name = "(unassigned sid)";
> 
> Likely safer for dev_name to be in the mutex too.

The result of arm_smmu_find_master() should not leave the lock, and
dev_name()'s result can't leave the lock either, at least not with out
some refcounting on the struct device.

Jason

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Nicolin Chen]

On Wed, Oct 02, 2024 at 10:57:28AM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 30, 2024 at 12:15:10PM -0700, Nicolin Chen wrote:
> > > +static void arm_smmu_get_evt_info(struct arm_smmu_device *smmu, u64 *evt,
> > > +                                 struct arm_smmu_event *event)
> > > +{
> > > +       mutex_lock(&smmu->streams_mutex);
> > > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > > +       mutex_unlock(&smmu->streams_mutex);
> > > +
> > > +       if (event->master)
> > > +               event->master_name = dev_name(event->master->dev);
> > > +       else
> > > +               event->master_name = "(unassigned sid)";
> >
> > Likely safer for dev_name to be in the mutex too.
> 
> The result of arm_smmu_find_master() should not leave the lock, and

Yea, "master_name = dev_name()" must be locked. Yet..

> dev_name()'s result can't leave the lock either, at least not with out
> some refcounting on the struct device.

I found dev_name returns "const char *" so the string source is
likely in the data section? If so, could the result of a locked
"master_name = dev_name()" get away from the lock?

Thanks
Nicolin

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Jason Gunthorpe]

On Wed, Oct 02, 2024 at 09:55:14AM -0700, Nicolin Chen wrote:
> > dev_name()'s result can't leave the lock either, at least not with out
> > some refcounting on the struct device.
> 
> I found dev_name returns "const char *" so the string source is
> likely in the data section? If so, could the result of a locked
> "master_name = dev_name()" get away from the lock?

The name is allocated memory freed when the struct device is freed.

Jason

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Nicolin Chen]

On Wed, Oct 02, 2024 at 02:10:52PM -0300, Jason Gunthorpe wrote:
> On Wed, Oct 02, 2024 at 09:55:14AM -0700, Nicolin Chen wrote:
> > > dev_name()'s result can't leave the lock either, at least not with out
> > > some refcounting on the struct device.
> >
> > I found dev_name returns "const char *" so the string source is
> > likely in the data section? If so, could the result of a locked
> > "master_name = dev_name()" get away from the lock?
> 
> The name is allocated memory freed when the struct device is freed.

Oh, reading it a bit further, I found kfree_const() at kobj->name
and some of the dev->init_names aren't coming from static strings.

We could kstrdup alternatively, but keeping the dump() inside the
lock is just simpler.

Thanks!
Nicolin

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Pranjal Shrivastava]

On Wed, Oct 02, 2024 at 10:22:13AM -0700, Nicolin Chen wrote:
> On Wed, Oct 02, 2024 at 02:10:52PM -0300, Jason Gunthorpe wrote:
> > On Wed, Oct 02, 2024 at 09:55:14AM -0700, Nicolin Chen wrote:
> > > > dev_name()'s result can't leave the lock either, at least not with out
> > > > some refcounting on the struct device.

Agreed, I missed that! Maybe we can avoid reading the master name in the
`arm_smmu_get_evt_info` and move it within the `arm_smmu_handle_evt`
under proper locking? Since the event won't be dumped before it's
handled, we can avoid locking at two places for doing the same thing?

> > >
> > > I found dev_name returns "const char *" so the string source is
> > > likely in the data section? If so, could the result of a locked
> > > "master_name = dev_name()" get away from the lock?
> > 
> > The name is allocated memory freed when the struct device is freed.
> 
> Oh, reading it a bit further, I found kfree_const() at kobj->name
> and some of the dev->init_names aren't coming from static strings.
> 
> We could kstrdup alternatively, but keeping the dump() inside the
> lock is just simpler.

kstrdup is a good idea too.. overall, I wouldn't wanna *accidentally*
mess up locking for logging (no rhymes intended).

Hence, we have 2 options:

1. Read the master_name within `arm_smmu_handle_evt` (at one place only)
2. Use kstrdup to copy the master_name elsewhere and dereference it.

Personally, I won't prefer 2 as we'd also have to free the memory
allocated for the dup'ed string after logging everything in
`arm_smmu_handle_evt` which is a bottom-half. LMK what y'all think?

> 
> Thanks!
> Nicolin

Thanks,
Pranjal

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Nicolin Chen]

On Thu, Oct 03, 2024 at 09:26:36PM +0000, Pranjal Shrivastava wrote:
> On Wed, Oct 02, 2024 at 10:22:13AM -0700, Nicolin Chen wrote:
> > On Wed, Oct 02, 2024 at 02:10:52PM -0300, Jason Gunthorpe wrote:
> > > On Wed, Oct 02, 2024 at 09:55:14AM -0700, Nicolin Chen wrote:
> > > > > dev_name()'s result can't leave the lock either, at least not with out
> > > > > some refcounting on the struct device.
> 
> Agreed, I missed that! Maybe we can avoid reading the master name in the
> `arm_smmu_get_evt_info` and move it within the `arm_smmu_handle_evt`
> under proper locking? Since the event won't be dumped before it's
> handled, we can avoid locking at two places for doing the same thing?

Yea, just move the master_name closer to the locker, and put the
dump() inside the locker too, either of which happens in handle().

Thanks
Nicolin

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Pranjal Shrivastava]

On Thu, Oct 03, 2024 at 03:50:09PM -0700, Nicolin Chen wrote:
> On Thu, Oct 03, 2024 at 09:26:36PM +0000, Pranjal Shrivastava wrote:
> > On Wed, Oct 02, 2024 at 10:22:13AM -0700, Nicolin Chen wrote:
> > > On Wed, Oct 02, 2024 at 02:10:52PM -0300, Jason Gunthorpe wrote:
> > > > On Wed, Oct 02, 2024 at 09:55:14AM -0700, Nicolin Chen wrote:
> > > > > > dev_name()'s result can't leave the lock either, at least not with out
> > > > > > some refcounting on the struct device.
> > 
> > Agreed, I missed that! Maybe we can avoid reading the master name in the
> > `arm_smmu_get_evt_info` and move it within the `arm_smmu_handle_evt`
> > under proper locking? Since the event won't be dumped before it's
> > handled, we can avoid locking at two places for doing the same thing?
> 
> Yea, just move the master_name closer to the locker, and put the
> dump() inside the locker too, either of which happens in handle().
> 

Ack, I'll read and dump the event within the lock acquired in
`arm_smmu_handle_evt` in the next version, while keeping `master_name` a
local variable.

> Thanks
> Nicolin

Thanks,
Pranjal

Re: [PATCH v3 1/2] iommu/arm-smmu-v3: Print better events records

[Pranjal Shrivastava]

On Fri, Oct 11, 2024 at 07:53:50AM +0000, Pranjal Shrivastava wrote:
> On Thu, Oct 03, 2024 at 03:50:09PM -0700, Nicolin Chen wrote:
> > On Thu, Oct 03, 2024 at 09:26:36PM +0000, Pranjal Shrivastava wrote:
> > > On Wed, Oct 02, 2024 at 10:22:13AM -0700, Nicolin Chen wrote:
> > > > On Wed, Oct 02, 2024 at 02:10:52PM -0300, Jason Gunthorpe wrote:
> > > > > On Wed, Oct 02, 2024 at 09:55:14AM -0700, Nicolin Chen wrote:
> > > > > > > dev_name()'s result can't leave the lock either, at least not with out
> > > > > > > some refcounting on the struct device.
> > > 
> > > Agreed, I missed that! Maybe we can avoid reading the master name in the
> > > `arm_smmu_get_evt_info` and move it within the `arm_smmu_handle_evt`
> > > under proper locking? Since the event won't be dumped before it's
> > > handled, we can avoid locking at two places for doing the same thing?
> > 
> > Yea, just move the master_name closer to the locker, and put the
> > dump() inside the locker too, either of which happens in handle().
> > 
> 
> Ack, I'll read and dump the event within the lock acquired in
> `arm_smmu_handle_evt` in the next version, while keeping `master_name` a
> local variable.

On a second thought, it feels weird to pass a separate "master_name" arg
to the `arm_smmu_dump_event`. Let's keep the master_name within
`arm_smmu_event` and read it within the locks.

> 
> > Thanks
> > Nicolin
> 

Thanks,
Pranjal

[PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava]

Modify the event handler routines to leverage `struct arm_smmu_event`.
Log the event within the `arm_smmu_handle_evt` routine.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 35 ++++++++++-----------
 1 file changed, 17 insertions(+), 18 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 2b2949bd2d26..3c326f3a038a 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1845,17 +1845,14 @@ static void arm_smmu_dump_event(struct arm_smmu_device *smmu,
 }
 
 /* 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_device *smmu, 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 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:
@@ -1865,42 +1862,45 @@ 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 = FIELD_GET(EVTQ_1_STAG, event->raw[1]),
 		.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);
-	if (!master) {
+	event->master = arm_smmu_find_master(smmu, event->sid);
+	if (!event->master) {
 		ret = -EINVAL;
+		event->master_name = "(unassigned sid)";
 		goto out_unlock;
 	}
 
-	ret = iommu_report_device_fault(master->dev, &fault_evt);
+	ret = iommu_report_device_fault(event->master->dev, &fault_evt);
 out_unlock:
+	if (ret)
+		arm_smmu_dump_event(smmu, event);
 	mutex_unlock(&smmu->streams_mutex);
 	return ret;
 }
@@ -1949,11 +1949,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 		while (!queue_remove_raw(q, evt)) {
 
 			arm_smmu_get_evt_info(smmu, evt, &event);
-			ret = arm_smmu_handle_evt(smmu, evt);
+			ret = arm_smmu_handle_evt(smmu, &event);
 			if (!ret || !__ratelimit(&rs))
 				continue;
 
-			arm_smmu_dump_event(smmu, &event);
 			cond_resched();
 		}
 
-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Nicolin Chen]

On Sat, Sep 28, 2024 at 12:51:43AM +0000, Pranjal Shrivastava wrote:
>  /* 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_device *smmu, struct arm_smmu_event *event)

Looks like we could stuff an smmu pointer in the event:

struct arm_smmu_event {
+	struct arm_smmu_device *smmu;
	struct arm_smmu_master		*master;
	const char			*master_name;

Then both this arm_smmu_handle_evt and arm_smmu_dump_event can be
slightly cleaner.

> 
>         mutex_lock(&smmu->streams_mutex);
> -       master = arm_smmu_find_master(smmu, sid);
> -       if (!master) {
> +       event->master = arm_smmu_find_master(smmu, event->sid);
> +       if (!event->master) {
>                 ret = -EINVAL;
> +               event->master_name = "(unassigned sid)";
>                 goto out_unlock;
>         }

The PATCH-1 already did arm_smmu_find_master() to event->master in
arm_smmu_get_evt_info()?

Maybe we still need a wider mutex to lock arm_smmu_get_evt_info and
arm_smmu_handle_evt.

Thanks
Nicolin

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava]

On Mon, Sep 30, 2024 at 12:32:21PM -0700, Nicolin Chen wrote:
> On Sat, Sep 28, 2024 at 12:51:43AM +0000, Pranjal Shrivastava wrote:
> >  /* 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_device *smmu, struct arm_smmu_event *event)
> 
> Looks like we could stuff an smmu pointer in the event:
> 
> struct arm_smmu_event {
> +	struct arm_smmu_device *smmu;
> 	struct arm_smmu_master		*master;
> 	const char			*master_name;
> 
> Then both this arm_smmu_handle_evt and arm_smmu_dump_event can be
> slightly cleaner.
> 

Makes sense, we can read that in the get_evt_info.

> > 
> >         mutex_lock(&smmu->streams_mutex);
> > -       master = arm_smmu_find_master(smmu, sid);
> > -       if (!master) {
> > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > +       if (!event->master) {
> >                 ret = -EINVAL;
> > +               event->master_name = "(unassigned sid)";
> >                 goto out_unlock;
> >         }
> 
> The PATCH-1 already did arm_smmu_find_master() to event->master in
> arm_smmu_get_evt_info()?
> 
> Maybe we still need a wider mutex to lock arm_smmu_get_evt_info and
> arm_smmu_handle_evt.
> 

Yea, we did, I'm wondering if we really need to read the master_name in
arm_smmu_get_evt_info ? I mean, we can find and populate the name here
itself under the safety of this mutex and entirely remove that lock from
arm_smmu_get_evt_info as we anyway will dump the event after this line.

I'm not too sure if we should hold this lock for the entire duration of
get_evt_info and handle_evt ?

Also, shall we rename it to `arm_smmu_read_evt_info` ?

> Thanks
> Nicolin

Thanks,
Pranjal

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Nicolin Chen]

On Tue, Oct 01, 2024 at 09:02:42PM +0000, Pranjal Shrivastava wrote:
> On Mon, Sep 30, 2024 at 12:32:21PM -0700, Nicolin Chen wrote:
> > On Sat, Sep 28, 2024 at 12:51:43AM +0000, Pranjal Shrivastava wrote:
> > >         mutex_lock(&smmu->streams_mutex);
> > > -       master = arm_smmu_find_master(smmu, sid);
> > > -       if (!master) {
> > > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > > +       if (!event->master) {
> > >                 ret = -EINVAL;
> > > +               event->master_name = "(unassigned sid)";
> > >                 goto out_unlock;
> > >         }
> >
> > The PATCH-1 already did arm_smmu_find_master() to event->master in
> > arm_smmu_get_evt_info()?
> >
> > Maybe we still need a wider mutex to lock arm_smmu_get_evt_info and
> > arm_smmu_handle_evt.
> >
> 
> Yea, we did, I'm wondering if we really need to read the master_name in
> arm_smmu_get_evt_info ? I mean, we can find and populate the name here
> itself under the safety of this mutex and entirely remove that lock from
> arm_smmu_get_evt_info as we anyway will dump the event after this line.

We could. I guess the two patches organized in the slightly odd
way that the arm_smmu_dump_event() gets moved around in the 2nd
patch, so things look a bit redundant after all.

I think we could do two patches like:
PATCH-1: Add struct arm_smmu_event and swap all FIELD_GETs with
         the structure members.
PATCH-2: Add arm_smmu_dump_event() and put in arm_smmu_handle_evt
         directly.

The master and master_name are not that necessary to be in the
arm_smmu_event. I would keep both of them as local variables in
arm_smmu_handle_evt and pass master_name in to dump().

The dev_name() returns "const char *init_name", so it'd likely
be safe to put the arm_smmu_dump_event(&event, master_name) call
outside the mutex.

> I'm not too sure if we should hold this lock for the entire duration of
> get_evt_info and handle_evt ?

It should be fine for being a mutex. Yet, shrinking its scope is
always optimal.

> Also, shall we rename it to `arm_smmu_read_evt_info` ?

I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
no strong feeling about that.

Thanks
Nicolin

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava]

On Tue, Oct 01, 2024 at 03:59:54PM -0700, Nicolin Chen wrote:
> On Tue, Oct 01, 2024 at 09:02:42PM +0000, Pranjal Shrivastava wrote:
> > On Mon, Sep 30, 2024 at 12:32:21PM -0700, Nicolin Chen wrote:
> > > On Sat, Sep 28, 2024 at 12:51:43AM +0000, Pranjal Shrivastava wrote:
> > > >         mutex_lock(&smmu->streams_mutex);
> > > > -       master = arm_smmu_find_master(smmu, sid);
> > > > -       if (!master) {
> > > > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > > > +       if (!event->master) {
> > > >                 ret = -EINVAL;
> > > > +               event->master_name = "(unassigned sid)";
> > > >                 goto out_unlock;
> > > >         }
> > >
> > > The PATCH-1 already did arm_smmu_find_master() to event->master in
> > > arm_smmu_get_evt_info()?
> > >
> > > Maybe we still need a wider mutex to lock arm_smmu_get_evt_info and
> > > arm_smmu_handle_evt.
> > >
> > 
> > Yea, we did, I'm wondering if we really need to read the master_name in
> > arm_smmu_get_evt_info ? I mean, we can find and populate the name here
> > itself under the safety of this mutex and entirely remove that lock from
> > arm_smmu_get_evt_info as we anyway will dump the event after this line.
> 
> We could. I guess the two patches organized in the slightly odd
> way that the arm_smmu_dump_event() gets moved around in the 2nd
> patch, so things look a bit redundant after all.
> 
> I think we could do two patches like:
> PATCH-1: Add struct arm_smmu_event and swap all FIELD_GETs with
>          the structure members.
> PATCH-2: Add arm_smmu_dump_event() and put in arm_smmu_handle_evt
>          directly.

Ahh, right! That makes more sense. I'll re-organize them like this.

> 
> The master and master_name are not that necessary to be in the
> arm_smmu_event. I would keep both of them as local variables in
> arm_smmu_handle_evt and pass master_name in to dump().

Hmm, right or maybe ONLY have the master_name in arm_smmu_event?

> 
> The dev_name() returns "const char *init_name", so it'd likely
> be safe to put the arm_smmu_dump_event(&event, master_name) call
> outside the mutex.
> 
> > I'm not too sure if we should hold this lock for the entire duration of
> > get_evt_info and handle_evt ?
> 
> It should be fine for being a mutex. Yet, shrinking its scope is
> always optimal.

Ack.

> 
> > Also, shall we rename it to `arm_smmu_read_evt_info` ?
> 
> I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
> light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
> no strong feeling about that.
> 

Ack. How about `read_arm_smmu_event` :) 
Unless, we wanna follow the arm_smmu_<func> convention?

> Thanks
> Nicolin

Thanks!
Praan

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Nicolin Chen]

On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
> > The master and master_name are not that necessary to be in the
> > arm_smmu_event. I would keep both of them as local variables in
> > arm_smmu_handle_evt and pass master_name in to dump().
> 
> Hmm, right or maybe ONLY have the master_name in arm_smmu_event?

Just move both out of arm_smmu_event. Keep the master_name as a
local variable like the "master". You'd just need to pass it in
to the dump(). They both are in that same handle_evt().

> > > Also, shall we rename it to `arm_smmu_read_evt_info` ?
> >
> > I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
> > light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
> > no strong feeling about that.
> >
> 
> Ack. How about `read_arm_smmu_event` :)
> Unless, we wanna follow the arm_smmu_<func> convention?

I think it'd be nicer if we do.

Nicolin

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava]

On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
> On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
> > > The master and master_name are not that necessary to be in the
> > > arm_smmu_event. I would keep both of them as local variables in
> > > arm_smmu_handle_evt and pass master_name in to dump().
> > 
> > Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
> 
> Just move both out of arm_smmu_event. Keep the master_name as a
> local variable like the "master". You'd just need to pass it in
> to the dump(). They both are in that same handle_evt().
> 

Ack.

> > > > Also, shall we rename it to `arm_smmu_read_evt_info` ?
> > >
> > > I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
> > > light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
> > > no strong feeling about that.
> > >
> > 
> > Ack. How about `read_arm_smmu_event` :)
> > Unless, we wanna follow the arm_smmu_<func> convention?
> 
> I think it'd be nicer if we do.
> 

Ack. `arm_smmu_get_evt_from_raw` it is!

> Nicolin
Thanks,
Pranjal

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Robin Murphy]

On 2024-10-11 8:55 am, Pranjal Shrivastava wrote:
> On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
>> On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
>>>> The master and master_name are not that necessary to be in the
>>>> arm_smmu_event. I would keep both of them as local variables in
>>>> arm_smmu_handle_evt and pass master_name in to dump().
>>>
>>> Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
>>
>> Just move both out of arm_smmu_event. Keep the master_name as a
>> local variable like the "master". You'd just need to pass it in
>> to the dump(). They both are in that same handle_evt().
>>
> 
> Ack.
> 
>>>>> Also, shall we rename it to `arm_smmu_read_evt_info` ?
>>>>
>>>> I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
>>>> light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
>>>> no strong feeling about that.
>>>>
>>>
>>> Ack. How about `read_arm_smmu_event` :)
>>> Unless, we wanna follow the arm_smmu_<func> convention?
>>
>> I think it'd be nicer if we do.
>>
> 
> Ack. `arm_smmu_get_evt_from_raw` it is!

FWIW that sounds needlessly overcomplicated to me - the "raw" event 
array should be a member of arm_smmu_event itself, since it seems silly 
to have them separate with one pointing to the other when they have the 
exact same scope and lifetime anyway. There still shouldn't need to be 
more than a single logical step to process an evtq record into a 
finished arm_smmu_event, just that that processing is now going to go a 
bit further than simply le64_to_cpu().

Thanks,
Robin.

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava]

On Fri, Oct 11, 2024 at 11:21:48AM +0100, Robin Murphy wrote:
> On 2024-10-11 8:55 am, Pranjal Shrivastava wrote:
> > On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
> > > On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
> > > > > The master and master_name are not that necessary to be in the
> > > > > arm_smmu_event. I would keep both of them as local variables in
> > > > > arm_smmu_handle_evt and pass master_name in to dump().
> > > > 
> > > > Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
> > > 
> > > Just move both out of arm_smmu_event. Keep the master_name as a
> > > local variable like the "master". You'd just need to pass it in
> > > to the dump(). They both are in that same handle_evt().
> > > 
> > 
> > Ack.
> > 
> > > > > > Also, shall we rename it to `arm_smmu_read_evt_info` ?
> > > > > 
> > > > > I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
> > > > > light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
> > > > > no strong feeling about that.
> > > > > 
> > > > 
> > > > Ack. How about `read_arm_smmu_event` :)
> > > > Unless, we wanna follow the arm_smmu_<func> convention?
> > > 
> > > I think it'd be nicer if we do.
> > > 
> > 
> > Ack. `arm_smmu_get_evt_from_raw` it is!
> 
> FWIW that sounds needlessly overcomplicated to me - the "raw" event array
> should be a member of arm_smmu_event itself, since it seems silly to have
> them separate with one pointing to the other when they have the exact same
> scope and lifetime anyway. There still shouldn't need to be more than a
> single logical step to process an evtq record into a finished
> arm_smmu_event, just that that processing is now going to go a bit further
> than simply le64_to_cpu().

Hmm, are you suggesting something like queue_remove_raw(q, event->raw)?
OR do you mean we should have our own queue_parse_evt(q, &event) that
parses out the raw event into an `arm_smmu_event` record eliminating the
need for the `arm_smmu_read_evt_info` altogether?

Because we'd still need to store the raw event anyway since we're still
logging the raw event.

I think we can go with the former, i.e. queue_remove_raw(q, event->raw)
and then use ``arm_smmu_read_evt_info` to populate other `event` fields 

> Thanks,
> Robin.

Thanks,
Pranjal

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava]

On Fri, Oct 11, 2024 at 10:45:52AM +0000, Pranjal Shrivastava wrote:
> On Fri, Oct 11, 2024 at 11:21:48AM +0100, Robin Murphy wrote:
> > On 2024-10-11 8:55 am, Pranjal Shrivastava wrote:
> > > On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
> > > > On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
> > > > > > The master and master_name are not that necessary to be in the
> > > > > > arm_smmu_event. I would keep both of them as local variables in
> > > > > > arm_smmu_handle_evt and pass master_name in to dump().
> > > > > 
> > > > > Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
> > > > 
> > > > Just move both out of arm_smmu_event. Keep the master_name as a
> > > > local variable like the "master". You'd just need to pass it in
> > > > to the dump(). They both are in that same handle_evt().
> > > > 
> > > 
> > > Ack.
> > > 
> > > > > > > Also, shall we rename it to `arm_smmu_read_evt_info` ?
> > > > > > 
> > > > > > I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
> > > > > > light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
> > > > > > no strong feeling about that.
> > > > > > 
> > > > > 
> > > > > Ack. How about `read_arm_smmu_event` :)
> > > > > Unless, we wanna follow the arm_smmu_<func> convention?
> > > > 
> > > > I think it'd be nicer if we do.
> > > > 
> > > 
> > > Ack. `arm_smmu_get_evt_from_raw` it is!
> > 
> > FWIW that sounds needlessly overcomplicated to me - the "raw" event array
> > should be a member of arm_smmu_event itself, since it seems silly to have
> > them separate with one pointing to the other when they have the exact same
> > scope and lifetime anyway. There still shouldn't need to be more than a
> > single logical step to process an evtq record into a finished
> > arm_smmu_event, just that that processing is now going to go a bit further
> > than simply le64_to_cpu().
> 
> Hmm, are you suggesting something like queue_remove_raw(q, event->raw)?
> OR do you mean we should have our own queue_parse_evt(q, &event) that
> parses out the raw event into an `arm_smmu_event` record eliminating the
> need for the `arm_smmu_read_evt_info` altogether?
> 
> Because we'd still need to store the raw event anyway since we're still
> logging the raw event.
> 
> I think we can go with the former, i.e. queue_remove_raw(q, event->raw)
> and then use ``arm_smmu_read_evt_info` to populate other `event` fields

Based on your comment, I was thinking like the attached sample code.
LMK if you were referring to something like that?

> 
> > Thanks,
> > Robin.
> 

Thanks,
Pranjal
--------------------------------------------------------------------------
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 0636b81eef79..5845793c0a41 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1929,6 +1929,35 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu, u64 *evt,
 	event->smmu = smmu;
 }
 
+static int queue_remove_raw(struct arm_smmu_queue *q
+			    struct arm_smmu_event *event)
+{
+	int ret;
+
+	ret = queue_remove_raw(q,event->raw);
+	if (ret)
+		return ret;
+
+	/* Parse out the event fields */
+	event->id = FIELD_GET(EVTQ_0_ID, evt[0]);
+	event->sid = FIELD_GET(EVTQ_0_SID, evt[0]);
+	event->ssid_valid = evt[0] & EVTQ_0_SSV;
+	event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, evt[0]) : IOMMU_NO_PASID;
+	event->class = FIELD_GET(EVTQ_1_CLASS, evt[1]);
+	event->iova = FIELD_GET(EVTQ_2_ADDR, evt[2]);
+	event->ipa = FIELD_GET(EVTQ_3_IPA, evt[3]);
+	event->privileged = FIELD_GET(EVTQ_1_PnU, evt[1]);
+	event->instruction = FIELD_GET(EVTQ_1_InD, evt[1]);
+	event->s2 = FIELD_GET(EVTQ_1_S2, evt[1]);
+	event->read = FIELD_GET(EVTQ_1_RnW, evt[1]);
+	event->stag = FIELD_GET(EVTQ_1_STAG, evt[1]);
+	event->stall = evt[1] & EVTQ_1_STALL;
+	event->raw = evt;
+	event->smmu = smmu;
+
+	return 0;
+}
+
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
 	int ret;
@@ -1941,9 +1970,8 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 	u64 evt[EVTQ_ENT_DWORDS];
 
 	do {
-		while (!queue_remove_raw(q, evt)) {
+		while (!queue_remove_evt(q, &event)) {
 
-			arm_smmu_get_event_from_raw(smmu, evt, &event);
 			ret = arm_smmu_handle_evt(&event);
 			if (!ret || !__ratelimit(&rs))
 				continue;

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Robin Murphy]

On 2024-10-11 12:06 pm, Pranjal Shrivastava wrote:
> On Fri, Oct 11, 2024 at 10:45:52AM +0000, Pranjal Shrivastava wrote:
>> On Fri, Oct 11, 2024 at 11:21:48AM +0100, Robin Murphy wrote:
>>> On 2024-10-11 8:55 am, Pranjal Shrivastava wrote:
>>>> On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
>>>>> On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
>>>>>>> The master and master_name are not that necessary to be in the
>>>>>>> arm_smmu_event. I would keep both of them as local variables in
>>>>>>> arm_smmu_handle_evt and pass master_name in to dump().
>>>>>>
>>>>>> Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
>>>>>
>>>>> Just move both out of arm_smmu_event. Keep the master_name as a
>>>>> local variable like the "master". You'd just need to pass it in
>>>>> to the dump(). They both are in that same handle_evt().
>>>>>
>>>>
>>>> Ack.
>>>>
>>>>>>>> Also, shall we rename it to `arm_smmu_read_evt_info` ?
>>>>>>>
>>>>>>> I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
>>>>>>> light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
>>>>>>> no strong feeling about that.
>>>>>>>
>>>>>>
>>>>>> Ack. How about `read_arm_smmu_event` :)
>>>>>> Unless, we wanna follow the arm_smmu_<func> convention?
>>>>>
>>>>> I think it'd be nicer if we do.
>>>>>
>>>>
>>>> Ack. `arm_smmu_get_evt_from_raw` it is!
>>>
>>> FWIW that sounds needlessly overcomplicated to me - the "raw" event array
>>> should be a member of arm_smmu_event itself, since it seems silly to have
>>> them separate with one pointing to the other when they have the exact same
>>> scope and lifetime anyway. There still shouldn't need to be more than a
>>> single logical step to process an evtq record into a finished
>>> arm_smmu_event, just that that processing is now going to go a bit further
>>> than simply le64_to_cpu().
>>
>> Hmm, are you suggesting something like queue_remove_raw(q, event->raw)?
>> OR do you mean we should have our own queue_parse_evt(q, &event) that
>> parses out the raw event into an `arm_smmu_event` record eliminating the
>> need for the `arm_smmu_read_evt_info` altogether?
>>
>> Because we'd still need to store the raw event anyway since we're still
>> logging the raw event.
>>
>> I think we can go with the former, i.e. queue_remove_raw(q, event->raw)
>> and then use ``arm_smmu_read_evt_info` to populate other `event` fields
> 
> Based on your comment, I was thinking like the attached sample code.
> LMK if you were referring to something like that?

Ah, apologies, it had slipped my mind that queue_read() is already 
buried two layers deep in an abstraction shared with the priq... so 
yeah, rather than start trying to turn that inside-out, I guess the 
neatest approach is to effectively flip this series instead - i.e. start 
with the minor shift to:

	struct arm_smmu_event evt;

	while (!queue_remove_raw(q, evt.raw)) {
		ret = arm_smmu_handle_evt(smmu, &evt);
		...
		dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
		//etc.

and leave arm_smmu_handle_evt() decoding the event as both it and 
arm_smmu_handle_ppr() already do, just now using the new structure 
instead of locals. Then we can come in to factor out and enhance the 
unhandled event report, and anything else specific to that reporting 
beyond the actual SMMU event record, like device names and ratelimiting, 
can remain private to arm_smmu_dump_event().

AFAICS the locking concern seems avoidable as well - we don't actually 
need the whole arm_smmu_master in either case, just the device, so then 
end of arm_smmu_handle_evt() could easily be a little simpler:

	...
	mutex_lock(&smmu->streams_mutex);
	master = arm_smmu_find_master(smmu, sid);
	if (master)
		event->dev = get_device(master->dev);
	mutex_unlock(&smmu->streams_mutex);

	if (!event->dev)
		return -EINVAL;

	return iommu_report_device_fault(event->dev, &fault_evt);

then arm_smmu_dump_event() is safe to use a construct like:

	event->dev ? dev_name(event->dev) : "(unassigned)"

and finally a cleanup in the evtq loop, which by that point probably 
condenses down to something like:

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

Thanks,
Robin.

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava]

On Fri, Oct 11, 2024 at 01:59:36PM +0100, Robin Murphy wrote:
> On 2024-10-11 12:06 pm, Pranjal Shrivastava wrote:
> > On Fri, Oct 11, 2024 at 10:45:52AM +0000, Pranjal Shrivastava wrote:
> > > On Fri, Oct 11, 2024 at 11:21:48AM +0100, Robin Murphy wrote:
> > > > On 2024-10-11 8:55 am, Pranjal Shrivastava wrote:
> > > > > On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
> > > > > > On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
> > > > > > > > The master and master_name are not that necessary to be in the
> > > > > > > > arm_smmu_event. I would keep both of them as local variables in
> > > > > > > > arm_smmu_handle_evt and pass master_name in to dump().
> > > > > > > 
> > > > > > > Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
> > > > > > 
> > > > > > Just move both out of arm_smmu_event. Keep the master_name as a
> > > > > > local variable like the "master". You'd just need to pass it in
> > > > > > to the dump(). They both are in that same handle_evt().
> > > > > > 
> > > > > 
> > > > > Ack.
> > > > > 
> > > > > > > > > Also, shall we rename it to `arm_smmu_read_evt_info` ?
> > > > > > > > 
> > > > > > > > I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
> > > > > > > > light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
> > > > > > > > no strong feeling about that.
> > > > > > > > 
> > > > > > > 
> > > > > > > Ack. How about `read_arm_smmu_event` :)
> > > > > > > Unless, we wanna follow the arm_smmu_<func> convention?
> > > > > > 
> > > > > > I think it'd be nicer if we do.
> > > > > > 
> > > > > 
> > > > > Ack. `arm_smmu_get_evt_from_raw` it is!
> > > > 
> > > > FWIW that sounds needlessly overcomplicated to me - the "raw" event array
> > > > should be a member of arm_smmu_event itself, since it seems silly to have
> > > > them separate with one pointing to the other when they have the exact same
> > > > scope and lifetime anyway. There still shouldn't need to be more than a
> > > > single logical step to process an evtq record into a finished
> > > > arm_smmu_event, just that that processing is now going to go a bit further
> > > > than simply le64_to_cpu().
> > > 
> > > Hmm, are you suggesting something like queue_remove_raw(q, event->raw)?
> > > OR do you mean we should have our own queue_parse_evt(q, &event) that
> > > parses out the raw event into an `arm_smmu_event` record eliminating the
> > > need for the `arm_smmu_read_evt_info` altogether?
> > > 
> > > Because we'd still need to store the raw event anyway since we're still
> > > logging the raw event.
> > > 
> > > I think we can go with the former, i.e. queue_remove_raw(q, event->raw)
> > > and then use ``arm_smmu_read_evt_info` to populate other `event` fields
> > 
> > Based on your comment, I was thinking like the attached sample code.
> > LMK if you were referring to something like that?
> 
> Ah, apologies, it had slipped my mind that queue_read() is already buried
> two layers deep in an abstraction shared with the priq... so yeah, rather
> than start trying to turn that inside-out, I guess the neatest approach is
> to effectively flip this series instead - i.e. start with the minor shift
> to:
> 
> 	struct arm_smmu_event evt;
> 
> 	while (!queue_remove_raw(q, evt.raw)) {
> 		ret = arm_smmu_handle_evt(smmu, &evt);
> 		...
> 		dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> 		//etc.
> 
> and leave arm_smmu_handle_evt() decoding the event as both it and
> arm_smmu_handle_ppr() already do, just now using the new structure instead
> of locals. Then we can come in to factor out and enhance the unhandled event
> report, and anything else specific to that reporting beyond the actual SMMU
> event record, like device names and ratelimiting, can remain private to
> arm_smmu_dump_event().

Ack. That makes sense. I'll re-organize the series accordingly.

> 
> AFAICS the locking concern seems avoidable as well - we don't actually need
> the whole arm_smmu_master in either case, just the device, so then end of
> arm_smmu_handle_evt() could easily be a little simpler:
> 
> 	...
> 	mutex_lock(&smmu->streams_mutex);
> 	master = arm_smmu_find_master(smmu, sid);
> 	if (master)
> 		event->dev = get_device(master->dev);
> 	mutex_unlock(&smmu->streams_mutex);
> 
> 	if (!event->dev)
> 		return -EINVAL;

Ahh, I see! My worry was a using `dev` after free but it looks like
`get_device(dev)` increments the reference count, preventing `dev` from
being freed prematurely.

Looking at the core code in drivers/base/core.c to confirm,
I see that the `device_release` invokes `dev->release` which frees the
`dev` struct and it seems that the `device_release` is called only
kobject's refcount reaches 0. Additionally, I see that `device_release`
is registered as the ->release callback in the device_ktype kobject.

So, looks like we should be good? LMK if I missed something above?

> 
> 	return iommu_report_device_fault(event->dev, &fault_evt);
> 
> then arm_smmu_dump_event() is safe to use a construct like:
> 
> 	event->dev ? dev_name(event->dev) : "(unassigned)"
> 
> and finally a cleanup in the evtq loop, which by that point probably
> condenses down to something like:
> 
> 	while (!queue_remove_raw(q, evt.raw)) {
> 		if (arm_smmu_handle_evt(smmu, &evt))
> 			arm_smmu_dump_event(&evt);
> 		put_device(evt.dev);

Ack. Although, I'm curious to know the concern while calling
`arm_smmu_dump_event` within `arm_smmu_handle_evt`, is it because we
might hold the lock for a longer duration due to the super-long prints?

> 		cond_resched();
> 	}
> 
> Thanks,
> Robin.

Thanks,
Pranjal

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava]

Just a few things before I respin,
On Fri, Oct 11, 2024 at 01:59:36PM +0100, Robin Murphy wrote:
> On 2024-10-11 12:06 pm, Pranjal Shrivastava wrote:
> > On Fri, Oct 11, 2024 at 10:45:52AM +0000, Pranjal Shrivastava wrote:
> > > On Fri, Oct 11, 2024 at 11:21:48AM +0100, Robin Murphy wrote:
> > > > On 2024-10-11 8:55 am, Pranjal Shrivastava wrote:
> > > > > On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
> > > > > > On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
> > > > > > > > The master and master_name are not that necessary to be in the
> > > > > > > > arm_smmu_event. I would keep both of them as local variables in
> > > > > > > > arm_smmu_handle_evt and pass master_name in to dump().
> > > > > > > 
> > > > > > > Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
> > > > > > 
> > > > > > Just move both out of arm_smmu_event. Keep the master_name as a
> > > > > > local variable like the "master". You'd just need to pass it in
> > > > > > to the dump(). They both are in that same handle_evt().
> > > > > > 
> > > > > 
> > > > > Ack.
> > > > > 
> > > > > > > > > Also, shall we rename it to `arm_smmu_read_evt_info` ?
> > > > > > > > 
> > > > > > > > I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
> > > > > > > > light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
> > > > > > > > no strong feeling about that.
> > > > > > > > 
> > > > > > > 
> > > > > > > Ack. How about `read_arm_smmu_event` :)
> > > > > > > Unless, we wanna follow the arm_smmu_<func> convention?
> > > > > > 
> > > > > > I think it'd be nicer if we do.
> > > > > > 
> > > > > 
> > > > > Ack. `arm_smmu_get_evt_from_raw` it is!
> > > > 
> > > > FWIW that sounds needlessly overcomplicated to me - the "raw" event array
> > > > should be a member of arm_smmu_event itself, since it seems silly to have
> > > > them separate with one pointing to the other when they have the exact same
> > > > scope and lifetime anyway. There still shouldn't need to be more than a
> > > > single logical step to process an evtq record into a finished
> > > > arm_smmu_event, just that that processing is now going to go a bit further
> > > > than simply le64_to_cpu().
> > > 
> > > Hmm, are you suggesting something like queue_remove_raw(q, event->raw)?
> > > OR do you mean we should have our own queue_parse_evt(q, &event) that
> > > parses out the raw event into an `arm_smmu_event` record eliminating the
> > > need for the `arm_smmu_read_evt_info` altogether?
> > > 
> > > Because we'd still need to store the raw event anyway since we're still
> > > logging the raw event.
> > > 
> > > I think we can go with the former, i.e. queue_remove_raw(q, event->raw)
> > > and then use ``arm_smmu_read_evt_info` to populate other `event` fields
> > 
> > Based on your comment, I was thinking like the attached sample code.
> > LMK if you were referring to something like that?
> 
> Ah, apologies, it had slipped my mind that queue_read() is already buried
> two layers deep in an abstraction shared with the priq... so yeah, rather
> than start trying to turn that inside-out, I guess the neatest approach is
> to effectively flip this series instead - i.e. start with the minor shift
> to:
> 
> 	struct arm_smmu_event evt;
> 
> 	while (!queue_remove_raw(q, evt.raw)) {
> 		ret = arm_smmu_handle_evt(smmu, &evt);
> 		...
> 		dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> 		//etc.
> 
> and leave arm_smmu_handle_evt() decoding the event as both it and
> arm_smmu_handle_ppr() already do, just now using the new structure instead
> of locals. Then we can come in to factor out and enhance the unhandled event
> report, and anything else specific to that reporting beyond the actual SMMU
> event record, like device names and ratelimiting, can remain private to
> arm_smmu_dump_event().

Quick clarification, the ratelimiting here was ONLY for logs or to limit
the number of thread / loop executions?

> 
> AFAICS the locking concern seems avoidable as well - we don't actually need
> the whole arm_smmu_master in either case, just the device, so then end of
> arm_smmu_handle_evt() could easily be a little simpler:
> 
> 	...
> 	mutex_lock(&smmu->streams_mutex);
> 	master = arm_smmu_find_master(smmu, sid);
> 	if (master)
> 		event->dev = get_device(master->dev);
> 	mutex_unlock(&smmu->streams_mutex);
> 
> 	if (!event->dev)
> 		return -EINVAL;

Thinking about this a little more, I guess there are a few paths that
may return before we even get to the `mutex_lock(&smmu->streams_mutex)`
For example: if (!event->stall) return -ENOTSUPP; and the default case
return from the switch case. Which has the following implications:

1. We might return from `arm_smmu_handle_evt` before logging the event
2. Even if we move the logging out of `arm_smmu_handle_evt` as presented
   by Robin's snippet above, event->dev might still be NULL.

In order to fix this, there are two ways to go:

1. Add a new "goto" label and instead of returning, jump to that label.
   Yet, we'd have to acquire the mutex again and read the dev ptr, like:
	
	...
	out_unlock:
		mutex_unlock(&smmu->streams_mutex);
	out_dump_evt:
		mutex_lock(&smmu->stream_mutext);
		// Find master and populate event->dev
		mutex_unlock(&smmu->streams_mutex);
		if (ret)
			arm_smmu_dump_event(event);

	return ret;
	...

Personally, I find this to be pretty messy!
Instead, it'd be nice to  simply read the dev ptr within the
`arm_smmu_get_event_from_raw` as:

	/* 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]);

	...
	mutex_lock(&smmu->streams_mutex);
	master = arm_smmu_find_master(smmu, sid);    	
	if (master)
		event->dev = get_device(master->dev);
	mutex_unlock(&smmu->streams_mutex);
	...

And then dump based on the return value as we do now and maybe
put_device(evt->dev) in the `arm_smmu_dump_event` after logging?

> 
> 	return iommu_report_device_fault(event->dev, &fault_evt);
> 
> then arm_smmu_dump_event() is safe to use a construct like:
> 
> 	event->dev ? dev_name(event->dev) : "(unassigned)"
> 
> and finally a cleanup in the evtq loop, which by that point probably
> condenses down to something like:
> 
> 	while (!queue_remove_raw(q, evt.raw)) {
> 		if (arm_smmu_handle_evt(smmu, &evt))
> 			arm_smmu_dump_event(&evt);
> 		put_device(evt.dev);
> 		cond_resched();
> 	}
> 
> Thanks,
> Robin.

Thanks,
Pranjal

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Robin Murphy]

On 2024-10-15 7:34 pm, Pranjal Shrivastava wrote:
> Just a few things before I respin,
> On Fri, Oct 11, 2024 at 01:59:36PM +0100, Robin Murphy wrote:
>> On 2024-10-11 12:06 pm, Pranjal Shrivastava wrote:
>>> On Fri, Oct 11, 2024 at 10:45:52AM +0000, Pranjal Shrivastava wrote:
>>>> On Fri, Oct 11, 2024 at 11:21:48AM +0100, Robin Murphy wrote:
>>>>> On 2024-10-11 8:55 am, Pranjal Shrivastava wrote:
>>>>>> On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
>>>>>>> On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
>>>>>>>>> The master and master_name are not that necessary to be in the
>>>>>>>>> arm_smmu_event. I would keep both of them as local variables in
>>>>>>>>> arm_smmu_handle_evt and pass master_name in to dump().
>>>>>>>>
>>>>>>>> Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
>>>>>>>
>>>>>>> Just move both out of arm_smmu_event. Keep the master_name as a
>>>>>>> local variable like the "master". You'd just need to pass it in
>>>>>>> to the dump(). They both are in that same handle_evt().
>>>>>>>
>>>>>>
>>>>>> Ack.
>>>>>>
>>>>>>>>>> Also, shall we rename it to `arm_smmu_read_evt_info` ?
>>>>>>>>>
>>>>>>>>> I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
>>>>>>>>> light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
>>>>>>>>> no strong feeling about that.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ack. How about `read_arm_smmu_event` :)
>>>>>>>> Unless, we wanna follow the arm_smmu_<func> convention?
>>>>>>>
>>>>>>> I think it'd be nicer if we do.
>>>>>>>
>>>>>>
>>>>>> Ack. `arm_smmu_get_evt_from_raw` it is!
>>>>>
>>>>> FWIW that sounds needlessly overcomplicated to me - the "raw" event array
>>>>> should be a member of arm_smmu_event itself, since it seems silly to have
>>>>> them separate with one pointing to the other when they have the exact same
>>>>> scope and lifetime anyway. There still shouldn't need to be more than a
>>>>> single logical step to process an evtq record into a finished
>>>>> arm_smmu_event, just that that processing is now going to go a bit further
>>>>> than simply le64_to_cpu().
>>>>
>>>> Hmm, are you suggesting something like queue_remove_raw(q, event->raw)?
>>>> OR do you mean we should have our own queue_parse_evt(q, &event) that
>>>> parses out the raw event into an `arm_smmu_event` record eliminating the
>>>> need for the `arm_smmu_read_evt_info` altogether?
>>>>
>>>> Because we'd still need to store the raw event anyway since we're still
>>>> logging the raw event.
>>>>
>>>> I think we can go with the former, i.e. queue_remove_raw(q, event->raw)
>>>> and then use ``arm_smmu_read_evt_info` to populate other `event` fields
>>>
>>> Based on your comment, I was thinking like the attached sample code.
>>> LMK if you were referring to something like that?
>>
>> Ah, apologies, it had slipped my mind that queue_read() is already buried
>> two layers deep in an abstraction shared with the priq... so yeah, rather
>> than start trying to turn that inside-out, I guess the neatest approach is
>> to effectively flip this series instead - i.e. start with the minor shift
>> to:
>>
>> 	struct arm_smmu_event evt;
>>
>> 	while (!queue_remove_raw(q, evt.raw)) {
>> 		ret = arm_smmu_handle_evt(smmu, &evt);
>> 		...
>> 		dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
>> 		//etc.
>>
>> and leave arm_smmu_handle_evt() decoding the event as both it and
>> arm_smmu_handle_ppr() already do, just now using the new structure instead
>> of locals. Then we can come in to factor out and enhance the unhandled event
>> report, and anything else specific to that reporting beyond the actual SMMU
>> event record, like device names and ratelimiting, can remain private to
>> arm_smmu_dump_event().
> 
> Quick clarification, the ratelimiting here was ONLY for logs or to limit
> the number of thread / loop executions?

Yes, the ratelimit is purely for the printks, so once those are factored 
out into a dedicated function it can move with them. The cond_resched() 
is what keeps the loop itself well-behaved if there's a giant flood of 
events; we never want to skip trying to handle any of them.

>>
>> AFAICS the locking concern seems avoidable as well - we don't actually need
>> the whole arm_smmu_master in either case, just the device, so then end of
>> arm_smmu_handle_evt() could easily be a little simpler:
>>
>> 	...
>> 	mutex_lock(&smmu->streams_mutex);
>> 	master = arm_smmu_find_master(smmu, sid);
>> 	if (master)
>> 		event->dev = get_device(master->dev);
>> 	mutex_unlock(&smmu->streams_mutex);
>>
>> 	if (!event->dev)
>> 		return -EINVAL;
> 
> Thinking about this a little more, I guess there are a few paths that
> may return before we even get to the `mutex_lock(&smmu->streams_mutex)`
> For example: if (!event->stall) return -ENOTSUPP; and the default case
> return from the switch case. Which has the following implications:
> 
> 1. We might return from `arm_smmu_handle_evt` before logging the event
> 2. Even if we move the logging out of `arm_smmu_handle_evt` as presented
>     by Robin's snippet above, event->dev might still be NULL.

The point is that it *can* be NULL in general, and the logging must 
expect that. Just move the arm_smmu_find_master() stanza to the top of 
arm_smmu_handle_evt() to grab the device reference before any of the 
actual handling may start wanting to bail out - same as you'll want to 
do for all the other decoding as well - then we know it's either valid 
(and stable) or NULL for the lifetime of the event through all 
subsequent processing. AFAICS calling iommu_report_device_fault() under 
streams_mutex isn't meaningful in itself, right now it just seems to be 
about protecting the inline "master->dev" dereference against "master" 
going away.

We can still have multiple returns as appropriate in the actual handling 
*after* collecting all the arm_smmu_event data, which is a big part of 
why I've suggested keeping the same effective structure as currently - 
we have one function to decode and try to handle the event, and based on 
the final outcome of that function, we may then want to report if it 
wasn't handled (subject to other constraints like ratelimiting). There's 
absolutely no need to tie the control flow in knots attempting to 
nano-optimise that into a tail call - using the returned value in the 
returned-to scope is natural, simple and obvious.

Thanks,
Robin.

Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava]

On Tue, Oct 15, 2024 at 09:03:54PM +0100, Robin Murphy wrote:
> On 2024-10-15 7:34 pm, Pranjal Shrivastava wrote:
> > Just a few things before I respin,
> > On Fri, Oct 11, 2024 at 01:59:36PM +0100, Robin Murphy wrote:
> > > On 2024-10-11 12:06 pm, Pranjal Shrivastava wrote:
> > > > On Fri, Oct 11, 2024 at 10:45:52AM +0000, Pranjal Shrivastava wrote:
> > > > > On Fri, Oct 11, 2024 at 11:21:48AM +0100, Robin Murphy wrote:
> > > > > > On 2024-10-11 8:55 am, Pranjal Shrivastava wrote:
> > > > > > > On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
> > > > > > > > On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
> > > > > > > > > > The master and master_name are not that necessary to be in the
> > > > > > > > > > arm_smmu_event. I would keep both of them as local variables in
> > > > > > > > > > arm_smmu_handle_evt and pass master_name in to dump().
> > > > > > > > > 
> > > > > > > > > Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
> > > > > > > > 
> > > > > > > > Just move both out of arm_smmu_event. Keep the master_name as a
> > > > > > > > local variable like the "master". You'd just need to pass it in
> > > > > > > > to the dump(). They both are in that same handle_evt().
> > > > > > > > 
> > > > > > > 
> > > > > > > Ack.
> > > > > > > 
> > > > > > > > > > > Also, shall we rename it to `arm_smmu_read_evt_info` ?
> > > > > > > > > > 
> > > > > > > > > > I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
> > > > > > > > > > light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
> > > > > > > > > > no strong feeling about that.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Ack. How about `read_arm_smmu_event` :)
> > > > > > > > > Unless, we wanna follow the arm_smmu_<func> convention?
> > > > > > > > 
> > > > > > > > I think it'd be nicer if we do.
> > > > > > > > 
> > > > > > > 
> > > > > > > Ack. `arm_smmu_get_evt_from_raw` it is!
> > > > > > 
> > > > > > FWIW that sounds needlessly overcomplicated to me - the "raw" event array
> > > > > > should be a member of arm_smmu_event itself, since it seems silly to have
> > > > > > them separate with one pointing to the other when they have the exact same
> > > > > > scope and lifetime anyway. There still shouldn't need to be more than a
> > > > > > single logical step to process an evtq record into a finished
> > > > > > arm_smmu_event, just that that processing is now going to go a bit further
> > > > > > than simply le64_to_cpu().
> > > > > 
> > > > > Hmm, are you suggesting something like queue_remove_raw(q, event->raw)?
> > > > > OR do you mean we should have our own queue_parse_evt(q, &event) that
> > > > > parses out the raw event into an `arm_smmu_event` record eliminating the
> > > > > need for the `arm_smmu_read_evt_info` altogether?
> > > > > 
> > > > > Because we'd still need to store the raw event anyway since we're still
> > > > > logging the raw event.
> > > > > 
> > > > > I think we can go with the former, i.e. queue_remove_raw(q, event->raw)
> > > > > and then use ``arm_smmu_read_evt_info` to populate other `event` fields
> > > > 
> > > > Based on your comment, I was thinking like the attached sample code.
> > > > LMK if you were referring to something like that?
> > > 
> > > Ah, apologies, it had slipped my mind that queue_read() is already buried
> > > two layers deep in an abstraction shared with the priq... so yeah, rather
> > > than start trying to turn that inside-out, I guess the neatest approach is
> > > to effectively flip this series instead - i.e. start with the minor shift
> > > to:
> > > 
> > > 	struct arm_smmu_event evt;
> > > 
> > > 	while (!queue_remove_raw(q, evt.raw)) {
> > > 		ret = arm_smmu_handle_evt(smmu, &evt);
> > > 		...
> > > 		dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > > 		//etc.
> > > 
> > > and leave arm_smmu_handle_evt() decoding the event as both it and
> > > arm_smmu_handle_ppr() already do, just now using the new structure instead
> > > of locals. Then we can come in to factor out and enhance the unhandled event
> > > report, and anything else specific to that reporting beyond the actual SMMU
> > > event record, like device names and ratelimiting, can remain private to
> > > arm_smmu_dump_event().
> > 
> > Quick clarification, the ratelimiting here was ONLY for logs or to limit
> > the number of thread / loop executions?
> 
> Yes, the ratelimit is purely for the printks, so once those are factored out
> into a dedicated function it can move with them. The cond_resched() is what
> keeps the loop itself well-behaved if there's a giant flood of events; we
> never want to skip trying to handle any of them.
> 
> > > 
> > > AFAICS the locking concern seems avoidable as well - we don't actually need
> > > the whole arm_smmu_master in either case, just the device, so then end of
> > > arm_smmu_handle_evt() could easily be a little simpler:
> > > 
> > > 	...
> > > 	mutex_lock(&smmu->streams_mutex);
> > > 	master = arm_smmu_find_master(smmu, sid);
> > > 	if (master)
> > > 		event->dev = get_device(master->dev);
> > > 	mutex_unlock(&smmu->streams_mutex);
> > > 
> > > 	if (!event->dev)
> > > 		return -EINVAL;
> > 
> > Thinking about this a little more, I guess there are a few paths that
> > may return before we even get to the `mutex_lock(&smmu->streams_mutex)`
> > For example: if (!event->stall) return -ENOTSUPP; and the default case
> > return from the switch case. Which has the following implications:
> > 
> > 1. We might return from `arm_smmu_handle_evt` before logging the event
> > 2. Even if we move the logging out of `arm_smmu_handle_evt` as presented
> >     by Robin's snippet above, event->dev might still be NULL.
> 
> The point is that it *can* be NULL in general, and the logging must expect
> that. Just move the arm_smmu_find_master() stanza to the top of
> arm_smmu_handle_evt() to grab the device reference before any of the actual
> handling may start wanting to bail out - same as you'll want to do for all
> the other decoding as well - then we know it's either valid (and stable) or
> NULL for the lifetime of the event through all subsequent processing. AFAICS
> calling iommu_report_device_fault() under streams_mutex isn't meaningful in
> itself, right now it just seems to be about protecting the inline
> "master->dev" dereference against "master" going away.

Ahh, alright. Let me refactor that too then.

> 
> We can still have multiple returns as appropriate in the actual handling
> *after* collecting all the arm_smmu_event data, which is a big part of why
> I've suggested keeping the same effective structure as currently - we have
> one function to decode and try to handle the event, and based on the final
> outcome of that function, we may then want to report if it wasn't handled
> (subject to other constraints like ratelimiting). There's absolutely no need
> to tie the control flow in knots attempting to nano-optimise that into a
> tail call - using the returned value in the returned-to scope is natural,
> simple and obvious.
> 

Agreed.

> Thanks,
> Robin.

Thanks,
Pranjal