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

[PATCH] 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.

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 | 102 ++++++++++++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  10 ++
 2 files changed, 105 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 a31460f9f3d4..993ded10c7b1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1783,9 +1783,102 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
 	return ret;
 }
 
+static void arm_smmu_print_evt_record(struct arm_smmu_device *smmu, u64 *evt)
+{
+	static const char * const evts[] = {
+		/* 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"
+	};
+
+	static const char * const class_str[] = {
+		[0] = "CD",
+		[1] = "TTD",
+		[2] = "IN",
+		[3] = "RES",
+	};
+
+	const char *master_name = "(unassigned sid)";
+	struct arm_smmu_master *master;
+	u64 iova, ipa;
+	bool ssid_valid;
+	u32 sid, ssid;
+	u8 evt_id, class;
+	int i;
+
+	/* Pick out the good stuff */
+	evt_id = FIELD_GET(EVTQ_0_ID, evt[0]);
+	sid = FIELD_GET(EVTQ_0_SID, evt[0]);
+	ssid_valid = evt[0] & EVTQ_0_SSV;
+	ssid = ssid_valid ? FIELD_GET(EVTQ_0_SSID, evt[0]) : IOMMU_NO_PASID;
+	class = FIELD_GET(EVTQ_1_CLASS, evt[1]);
+	iova = FIELD_GET(EVTQ_2_ADDR, evt[2]);
+	ipa = FIELD_GET(EVTQ_3_IPA, evt[3]);
+
+	mutex_lock(&smmu->streams_mutex);
+	master = arm_smmu_find_master(smmu, sid);
+	if (master)
+		master_name = dev_name(master->dev);
+	mutex_unlock(&smmu->streams_mutex);
+
+	switch (evt_id) {
+	case EVT_ID_BAD_SID_CONFIG:
+	case EVT_ID_BAD_STE_CONFIG:
+	case EVT_ID_BAD_SSID_CONFIG:
+	case EVT_ID_BAD_CD_CONFIG:
+	case EVT_ID_STREAM_DISABLED:
+		dev_err(smmu->dev, "Bad smmu config - %s client %s sid 0x%08x.0x%05x\n",
+				evts[evt_id], master_name, sid, ssid);
+		break;
+
+	case EVT_ID_TRANSLATION_FAULT:
+	case EVT_ID_ADDR_SIZE_FAULT:
+	case EVT_ID_ACCESS_FAULT:
+	case EVT_ID_PERMISSION_FAULT:
+		dev_err(smmu->dev, "Translation fault:\n");
+		dev_err(smmu->dev, "\t%s client %s sid 0x%08x.0x%05x: iova = %#llx ipa = %#llx (%s%s%s%s%s%s)\n",
+			    evts[evt_id], master_name, sid, ssid, iova, ipa,
+			    FIELD_GET(EVTQ_1_PnU, evt[1]) ? "Priv " : "Unpriv ",
+			    FIELD_GET(EVTQ_1_InD, evt[1]) ? "Inst " : "Data ",
+			    FIELD_GET(EVTQ_1_RnW, evt[1]) ? "Read " : "Write ",
+			    FIELD_GET(EVTQ_1_S2, evt[1]) ? "S2 " : "S1 ", class_str[class],
+			    ((evt_id == EVT_ID_PERMISSION_FAULT) && (class == EVTQ_1_CLASS_TT)) ?
+			    (FIELD_GET(EVTQ_1_TT_READ, evt[1]) ? " TTD Read" : " TTD Write")
+			    : "");
+		break;
+
+	case EVT_ID_STE_FETCH_FAULT:
+	case EVT_ID_CD_FETCH_FAULT:
+	case EVT_ID_VMS_FETCH_FAULT:
+		dev_err(smmu->dev, "Unable to fetch data structure - bad fetch address\n");
+		dev_err(smmu->dev, "\t%s client %s sid 0x%08x.0x%05x: fetch addr = %#llx\n",
+				evts[evt_id], master_name, sid, ssid, ipa);
+		break;
+	}
+
+	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]);
+}
+
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
-	int i, ret;
+	int ret;
 	struct arm_smmu_device *smmu = dev;
 	struct arm_smmu_queue *q = &smmu->evtq.q;
 	struct arm_smmu_ll_queue *llq = &q->llq;
@@ -1795,17 +1888,12 @@ 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]);
 
 			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_print_evt_record(smmu, evt);
 			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 14bca41a981b..500eecbf2834 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -399,10 +399,19 @@ struct arm_smmu_cd {
 
 #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 EVTQ_0_SSV			(1UL << 11)
 #define EVTQ_0_SSID			GENMASK_ULL(31, 12)
@@ -414,6 +423,7 @@ struct arm_smmu_cd {
 #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)
-- 
2.46.0.184.g6999bdac58-goog

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

[Will Deacon]

Hi Pranjal,

On Fri, Aug 16, 2024 at 09:17:22PM +0000, Pranjal Shrivastava wrote:
> 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.
> 
> 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 | 102 ++++++++++++++++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  10 ++
>  2 files changed, 105 insertions(+), 7 deletions(-)

Thanks for doing this. I've got used to the useless messages we currently
print, but this will be a tonne beter.

> 
> 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 a31460f9f3d4..993ded10c7b1 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1783,9 +1783,102 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
>  	return ret;
>  }
>  
> +static void arm_smmu_print_evt_record(struct arm_smmu_device *smmu, u64 *evt)
> +{
> +	static const char * const evts[] = {
> +		/* 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"
> +	};
> +
> +	static const char * const class_str[] = {
> +		[0] = "CD",
> +		[1] = "TTD",
> +		[2] = "IN",
> +		[3] = "RES",
> +	};

You can move these arrays out of this function...

> +
> +	const char *master_name = "(unassigned sid)";
> +	struct arm_smmu_master *master;
> +	u64 iova, ipa;
> +	bool ssid_valid;
> +	u32 sid, ssid;
> +	u8 evt_id, class;
> +	int i;
> +
> +	/* Pick out the good stuff */
> +	evt_id = FIELD_GET(EVTQ_0_ID, evt[0]);
> +	sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> +	ssid_valid = evt[0] & EVTQ_0_SSV;
> +	ssid = ssid_valid ? FIELD_GET(EVTQ_0_SSID, evt[0]) : IOMMU_NO_PASID;
> +	class = FIELD_GET(EVTQ_1_CLASS, evt[1]);
> +	iova = FIELD_GET(EVTQ_2_ADDR, evt[2]);
> +	ipa = FIELD_GET(EVTQ_3_IPA, evt[3]);

You could encapsulate this in e.g.

	struct arm_smmu_event {
		u8	id;
		u32	sid;
		u32	ssid;
		...
		u64	*raw;
	};

> +
> +	mutex_lock(&smmu->streams_mutex);
> +	master = arm_smmu_find_master(smmu, sid);
> +	if (master)
> +		master_name = dev_name(master->dev);
> +	mutex_unlock(&smmu->streams_mutex);
> +
> +	switch (evt_id) {
> +	case EVT_ID_BAD_SID_CONFIG:
> +	case EVT_ID_BAD_STE_CONFIG:
> +	case EVT_ID_BAD_SSID_CONFIG:
> +	case EVT_ID_BAD_CD_CONFIG:
> +	case EVT_ID_STREAM_DISABLED:
> +		dev_err(smmu->dev, "Bad smmu config - %s client %s sid 0x%08x.0x%05x\n",
> +				evts[evt_id], master_name, sid, ssid);
> +		break;

and have separate dumper functions here, I think.

> +	case EVT_ID_TRANSLATION_FAULT:
> +	case EVT_ID_ADDR_SIZE_FAULT:
> +	case EVT_ID_ACCESS_FAULT:
> +	case EVT_ID_PERMISSION_FAULT:
> +		dev_err(smmu->dev, "Translation fault:\n");
> +		dev_err(smmu->dev, "\t%s client %s sid 0x%08x.0x%05x: iova = %#llx ipa = %#llx (%s%s%s%s%s%s)\n",
> +			    evts[evt_id], master_name, sid, ssid, iova, ipa,
> +			    FIELD_GET(EVTQ_1_PnU, evt[1]) ? "Priv " : "Unpriv ",
> +			    FIELD_GET(EVTQ_1_InD, evt[1]) ? "Inst " : "Data ",
> +			    FIELD_GET(EVTQ_1_RnW, evt[1]) ? "Read " : "Write ",
> +			    FIELD_GET(EVTQ_1_S2, evt[1]) ? "S2 " : "S1 ", class_str[class],
> +			    ((evt_id == EVT_ID_PERMISSION_FAULT) && (class == EVTQ_1_CLASS_TT)) ?
> +			    (FIELD_GET(EVTQ_1_TT_READ, evt[1]) ? " TTD Read" : " TTD Write")
> +			    : "");

i.e. call arm_smmu_dump_translation_fault(&event) here.

> +		break;
> +
> +	case EVT_ID_STE_FETCH_FAULT:
> +	case EVT_ID_CD_FETCH_FAULT:
> +	case EVT_ID_VMS_FETCH_FAULT:
> +		dev_err(smmu->dev, "Unable to fetch data structure - bad fetch address\n");
> +		dev_err(smmu->dev, "\t%s client %s sid 0x%08x.0x%05x: fetch addr = %#llx\n",
> +				evts[evt_id], master_name, sid, ssid, ipa);
> +		break;
> +	}
> +
> +	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]);
> +}

We should probably be consistent with dev_info() vs dev_err().

Will

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

[Pranjal Shrivastava]

On Fri, Aug 23, 2024 at 05:31:05PM +0100, Will Deacon wrote:
> Hi Pranjal,
> 
> On Fri, Aug 16, 2024 at 09:17:22PM +0000, Pranjal Shrivastava wrote:
> > 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.
> > 
> > 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 | 102 ++++++++++++++++++--
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  10 ++
> >  2 files changed, 105 insertions(+), 7 deletions(-)
> 
> Thanks for doing this. I've got used to the useless messages we currently
> print, but this will be a tonne beter.
> 
> > 
> > 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 a31460f9f3d4..993ded10c7b1 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -1783,9 +1783,102 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >  	return ret;
> >  }
> >  
> > +static void arm_smmu_print_evt_record(struct arm_smmu_device *smmu, u64 *evt)
> > +{
> > +	static const char * const evts[] = {
> > +		/* 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"
> > +	};
> > +
> > +	static const char * const class_str[] = {
> > +		[0] = "CD",
> > +		[1] = "TTD",
> > +		[2] = "IN",
> > +		[3] = "RES",
> > +	};
> 
> You can move these arrays out of this function...
> 

Sure, will move this out.

> > +
> > +	const char *master_name = "(unassigned sid)";
> > +	struct arm_smmu_master *master;
> > +	u64 iova, ipa;
> > +	bool ssid_valid;
> > +	u32 sid, ssid;
> > +	u8 evt_id, class;
> > +	int i;
> > +
> > +	/* Pick out the good stuff */
> > +	evt_id = FIELD_GET(EVTQ_0_ID, evt[0]);
> > +	sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> > +	ssid_valid = evt[0] & EVTQ_0_SSV;
> > +	ssid = ssid_valid ? FIELD_GET(EVTQ_0_SSID, evt[0]) : IOMMU_NO_PASID;
> > +	class = FIELD_GET(EVTQ_1_CLASS, evt[1]);
> > +	iova = FIELD_GET(EVTQ_2_ADDR, evt[2]);
> > +	ipa = FIELD_GET(EVTQ_3_IPA, evt[3]);
> 
> You could encapsulate this in e.g.
> 
> 	struct arm_smmu_event {
> 		u8	id;
> 		u32	sid;
> 		u32	ssid;
> 		...
> 		u64	*raw;
> 	};
> 

Ack, will push all of this into a struct and break out the reading into
a helper function.

> > +
> > +	mutex_lock(&smmu->streams_mutex);
> > +	master = arm_smmu_find_master(smmu, sid);
> > +	if (master)
> > +		master_name = dev_name(master->dev);
> > +	mutex_unlock(&smmu->streams_mutex);
> > +
> > +	switch (evt_id) {
> > +	case EVT_ID_BAD_SID_CONFIG:
> > +	case EVT_ID_BAD_STE_CONFIG:
> > +	case EVT_ID_BAD_SSID_CONFIG:
> > +	case EVT_ID_BAD_CD_CONFIG:
> > +	case EVT_ID_STREAM_DISABLED:
> > +		dev_err(smmu->dev, "Bad smmu config - %s client %s sid 0x%08x.0x%05x\n",
> > +				evts[evt_id], master_name, sid, ssid);
> > +		break;
> 
> and have separate dumper functions here, I think.
> 
> > +	case EVT_ID_TRANSLATION_FAULT:
> > +	case EVT_ID_ADDR_SIZE_FAULT:
> > +	case EVT_ID_ACCESS_FAULT:
> > +	case EVT_ID_PERMISSION_FAULT:
> > +		dev_err(smmu->dev, "Translation fault:\n");
> > +		dev_err(smmu->dev, "\t%s client %s sid 0x%08x.0x%05x: iova = %#llx ipa = %#llx (%s%s%s%s%s%s)\n",
> > +			    evts[evt_id], master_name, sid, ssid, iova, ipa,
> > +			    FIELD_GET(EVTQ_1_PnU, evt[1]) ? "Priv " : "Unpriv ",
> > +			    FIELD_GET(EVTQ_1_InD, evt[1]) ? "Inst " : "Data ",
> > +			    FIELD_GET(EVTQ_1_RnW, evt[1]) ? "Read " : "Write ",
> > +			    FIELD_GET(EVTQ_1_S2, evt[1]) ? "S2 " : "S1 ", class_str[class],
> > +			    ((evt_id == EVT_ID_PERMISSION_FAULT) && (class == EVTQ_1_CLASS_TT)) ?
> > +			    (FIELD_GET(EVTQ_1_TT_READ, evt[1]) ? " TTD Read" : " TTD Write")
> > +			    : "");
> 
> i.e. call arm_smmu_dump_translation_fault(&event) here.
> 

Alright, will move the printing to separate dumper functions.

> > +		break;
> > +
> > +	case EVT_ID_STE_FETCH_FAULT:
> > +	case EVT_ID_CD_FETCH_FAULT:
> > +	case EVT_ID_VMS_FETCH_FAULT:
> > +		dev_err(smmu->dev, "Unable to fetch data structure - bad fetch address\n");
> > +		dev_err(smmu->dev, "\t%s client %s sid 0x%08x.0x%05x: fetch addr = %#llx\n",
> > +				evts[evt_id], master_name, sid, ssid, ipa);
> > +		break;
> > +	}
> > +
> > +	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]);
> > +}
> 
> We should probably be consistent with dev_info() vs dev_err().

Ahh, I missed this, would use dev_err everywhere.

> 
> Will


I'll send out a v2 addressing these review comments.

Thanks,
Pranjal