[PATCH v2 0/2] iommu/arm-smmu-v3: Parse out event records

[PATCH v2 0/2] iommu/arm-smmu-v3: Parse out event records

[Pranjal Shrivastava]

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

v2
 * 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 | 165 ++++++++++++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  27 ++++
 2 files changed, 162 insertions(+), 30 deletions(-)

-- 
2.46.0.295.g3b9ea8a38a-goog

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

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 |  27 +++++
 2 files changed, 148 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 8a6cd0adfcf2..65160b52d944 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,33 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ 0, NULL},
 };
 
+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",
+};
+
 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);
@@ -1783,9 +1810,100 @@ 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->stage = FIELD_GET(EVTQ_1_S2, evt[1]);
+	event->read = FIELD_GET(EVTQ_1_RnW, evt[1]);
+	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 void arm_smmu_dump_xlation_fault(struct arm_smmu_device *smmu,
+					struct arm_smmu_event *event)
+{
+	dev_err(smmu->dev, "Fault: %s client %s sid 0x%08x.0x%05x:\n",
+			evts[event->id], event->master_name, event->sid, event->ssid);
+	dev_err(smmu->dev, "\tiova = %#llx ipa = %#llx (%s%s%s%s%s%s)\n", event->iova, event->ipa,
+		    event->privileged ? "Priv " : "Unpriv ",
+		    event->instruction ? "Inst " : "Data ",
+		    event->read ? "Read " : "Write ",
+		    event->stage ? "S2 " : "S1 ", class_str[event->class],
+		    ((event->id == EVT_ID_PERMISSION_FAULT) && (event->class == EVTQ_1_CLASS_TT)) ?
+		    (FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ? " TTD Read" : " TTD Write")
+		    : "");
+}
+
+static void arm_smmu_dump_fetch_fault(struct arm_smmu_device *smmu,
+				      struct arm_smmu_event *event)
+{
+	dev_err(smmu->dev, "Bad fetch: %s client %s sid 0x%08x.0x%05x: fetch addr = %#llx\n",
+			evts[event->id], event->master_name, event->sid, event->ssid, event->ipa);
+}
+
+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: client %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)
+{
+	switch (event->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[event->id], event->master_name, event->sid, event->ssid);
+		break;
+
+	case EVT_ID_TRANSLATION_FAULT:
+	case EVT_ID_ADDR_SIZE_FAULT:
+	case EVT_ID_ACCESS_FAULT:
+	case EVT_ID_PERMISSION_FAULT:
+		arm_smmu_dump_xlation_fault(smmu, event);
+		break;
+
+	case EVT_ID_STE_FETCH_FAULT:
+	case EVT_ID_CD_FETCH_FAULT:
+	case EVT_ID_VMS_FETCH_FAULT:
+		arm_smmu_dump_fetch_fault(smmu, event);
+		break;
+	}
+
+	arm_smmu_dump_raw_event(smmu, event);
+}
+
 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;
@@ -1795,17 +1913,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 14bca41a981b..6728e306e4da 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)
@@ -702,6 +712,23 @@ 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;
+	u32				sid;
+	u32				ssid;
+	u64				iova;
+	u64				ipa;
+	u64				*raw;
+	bool				ssid_valid;
+	bool				privileged;
+	bool				instruction;
+	bool				stage;
+	bool				read;
+};
+
 /* SMMU private data for each master */
 struct arm_smmu_master {
 	struct arm_smmu_device		*smmu;
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v2 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`

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 39 ++++++++-------------
 1 file changed, 15 insertions(+), 24 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 65160b52d944..e33f73079a2a 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1746,17 +1746,14 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
 }
 
 /* IRQ and event handlers */
-static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
+static int arm_smmu_handle_evt(struct arm_smmu_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:
@@ -1767,46 +1764,40 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
 	}
 
 	/* Stage-2 is always pinned at the moment */
-	if (evt[1] & EVTQ_1_S2)
+	if (event->stage)
 		return -EFAULT;
 
-	if (!(evt[1] & EVTQ_1_STALL))
+	if (!(event->raw[1] & EVTQ_1_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) {
-		ret = -EINVAL;
-		goto out_unlock;
-	}
+	if (!event->master)
+		return -EINVAL;
 
-	ret = iommu_report_device_fault(master->dev, &fault_evt);
-out_unlock:
-	mutex_unlock(&smmu->streams_mutex);
+	ret = iommu_report_device_fault(event->master->dev, &fault_evt);
 	return ret;
 }
 
@@ -1915,7 +1906,7 @@ 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;
 
-- 
2.46.0.295.g3b9ea8a38a-goog

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

[Nicolin Chen]

Hi Pranjal,

On Tue, Aug 27, 2024 at 12:30:26PM -0700, Pranjal Shrivastava wrote:

>         /* Stage-2 is always pinned at the moment */
> -       if (evt[1] & EVTQ_1_S2)
> +       if (event->stage)
>                 return -EFAULT;

Should it be named to "s2" v.s stage?

> -       mutex_lock(&smmu->streams_mutex);
> -       master = arm_smmu_find_master(smmu, sid);
> -       if (!master) {
> -               ret = -EINVAL;
> -               goto out_unlock;
> -       }
> +       if (!event->master)
> +               return -EINVAL;
> 
> -       ret = iommu_report_device_fault(master->dev, &fault_evt);
> -out_unlock:
> -       mutex_unlock(&smmu->streams_mutex);
> +       ret = iommu_report_device_fault(event->master->dev, &fault_evt);

The iommu_report_device_fault(master->dev) call lost its mutex
lock. I wonder if it could be unsafe to continue with that dev
pointer.

Nicolin 

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

[Nicolin Chen]

Hi Pranjal,

The prints would be very helpful for debugging. Thanks for the
patch!

I have some personally thoughts here, so hopefully we can make
it cleaner and more readable.

On Tue, Aug 27, 2024 at 12:30:25PM -0700, Pranjal Shrivastava wrote:

> +static const char * const evts[] = {

s/evts/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"
> +};
> +
> +static const char * const class_str[] = {
> +       [0] = "CD",
> +       [1] = "TTD",
> +       [2] = "IN",
> +       [3] = "RES",
> +};

Unlike the event IDs, these class code names are still uneasy to
read. Though it'd result in a print-format change, yet could we
simply dump full strings instead?

> +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->stage = FIELD_GET(EVTQ_1_S2, evt[1]);
> +       event->read = FIELD_GET(EVTQ_1_RnW, evt[1]);
> +       event->raw = evt;

Maybe we could define struct arm_smmu_event in this way:

+struct arm_smmu_event {
+	union {
+		u64 evt[4];
+		struct {
+			/* Bit 0:63 */
+			u64 id : 8;
+			u64 _res0 : 3;
+			u64 ssv : 1;
+			u64 ssid : 20;
+			u64 sid : 32;
+			/* Bit 64:127 */
+			u64 stag : 16;
+			u64 _res1 : 15;
+			u64 stall : 1;
+			u64 _res2 : 1;
+			u64 pnu : 1;
+			u64 ind : 1;
+			u64 rnw : 1;
+			u64 _res3 : 2;
+			u64 nsipa : 1;
+			u64 s2 : 1;
+			u64 class : 2;
+			u64 _res4 : 6;
+			u64 impl_def : 16;
+			/* Bit 128:191 */
+			u64 inputaddr;
+			/* Bit 192:255 */
+			u64 _res5 : 12;
+			u64 ipa : 40;
+			u64 _res6 : 12;
+		} f_trans;
+		/* FIXME Add other event structs */
+	};
+};

Then, event would be just:
+		struct arm_smmu_event *event = (struct arm_smmu_event *)evt;

Not sure if Will would like this though...

> +
> +       mutex_lock(&smmu->streams_mutex);
> +       event->master = arm_smmu_find_master(smmu, event->sid);
> +       mutex_unlock(&smmu->streams_mutex);

Same as I pointed out at the other patch, "master" is unprotected
after the unlock. It can unlikely-yet-still-possibly race against
arm_smmu_release_device.

> +static void arm_smmu_dump_xlation_fault(struct arm_smmu_device *smmu,
> +                                       struct arm_smmu_event *event)
> +{
> +       dev_err(smmu->dev, "Fault: %s client %s sid 0x%08x.0x%05x:\n",

"client" doesn't seem to be used anywhere in this driver, I would:
s/client/master

And it'd feel clearer to have an "ssid". And perhaps adding commas
to separate them too.

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

Indentation should follow the existing printk() in this driver. And
those ternary operators at the last field(s?) are hard to ready..

> +static void arm_smmu_dump_fetch_fault(struct arm_smmu_device *smmu,
> +                                     struct arm_smmu_event *event)
> +{
> +       dev_err(smmu->dev, "Bad fetch: %s client %s sid 0x%08x.0x%05x: fetch addr = %#llx\n",
> +                       evts[event->id], event->master_name, event->sid, event->ssid, event->ipa);

Actually, the "Fault", "Bad fetch", and "Bad smmu config" doesn't
feel very necessary, since we prints the event string already.

> +}
> +
> +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: client %s:\n", event->id, event->master_name);

Looks like it would print another title that's sorta duplicated to
other dump functions yet less informative?

> +       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)
> +{
> +       switch (event->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[event->id], event->master_name, event->sid, event->ssid);
> +               break;
> +
> +       case EVT_ID_TRANSLATION_FAULT:
> +       case EVT_ID_ADDR_SIZE_FAULT:
> +       case EVT_ID_ACCESS_FAULT:
> +       case EVT_ID_PERMISSION_FAULT:
> +               arm_smmu_dump_xlation_fault(smmu, event);
> +               break;
> +
> +       case EVT_ID_STE_FETCH_FAULT:
> +       case EVT_ID_CD_FETCH_FAULT:
> +       case EVT_ID_VMS_FETCH_FAULT:
> +               arm_smmu_dump_fetch_fault(smmu, event);
> +               break;
> +       }
> +
> +       arm_smmu_dump_raw_event(smmu, event);

I wonder if something like this would be cleaner:

	...
	char title[256];
	char addrs[256];
	char other[256];
	int i;

	switch () {
	case xxxx:
		/* If duplicated between cases, put them into helpers */
		snprintf(title, 256, "%s: Master %s, sid 0x%08x, ssid 0x%05x\n",
			 event_str(event->id], dev_name(master->dev),
			 event->sid, event->ssid);
		snprintf(addrs, 256, .....);
		snprintf(other, 256, .....);
		break;
	...
	default:
		/* snprintf raw title */
		break;
	}
	if (strlen(title))
		dev_err(smmu->dev, "%s", title);
	if (strlen(addrs))
		dev_err(smmu->dev, "%s", addrs);
	if (strlen(other))
		dev_err(smmu->dev, "%s", other);
	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
		dev_err(smmu->dev, "\t0x%016llx\n", event->evt[i]);
	...

Thanks
Nicolin

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

[Pranjal Shrivastava]

On Wed, Aug 28, 2024 at 11:36:02PM -0700, Nicolin Chen wrote:
Hi Nicolin,
> Hi Pranjal,
> 
> The prints would be very helpful for debugging. Thanks for the
> patch!
> 
> I have some personally thoughts here, so hopefully we can make
> it cleaner and more readable.
> 
> On Tue, Aug 27, 2024 at 12:30:25PM -0700, Pranjal Shrivastava wrote:
> 
> > +static const char * const evts[] = {
> 
> s/evts/event_str
> 

Ack, would change that in the follow up patch.

> > +       /* 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",
> > +};
> 
> Unlike the event IDs, these class code names are still uneasy to
> read. Though it'd result in a print-format change, yet could we
> simply dump full strings instead?
> 

By "full strings" do you mean "CD => CD Fetch" as mentioned in the spec?
Or do you mean "Context Descriptor", "Translation Table Desc" etc.? I'm
afraid that the latter would make the printed string much longer.
Although, I wouldn't mind doing that if we have consensus.

> > +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->stage = FIELD_GET(EVTQ_1_S2, evt[1]);
> > +       event->read = FIELD_GET(EVTQ_1_RnW, evt[1]);
> > +       event->raw = evt;
> 
> Maybe we could define struct arm_smmu_event in this way:
> 
> +struct arm_smmu_event {
> +	union {
> +		u64 evt[4];
> +		struct {
> +			/* Bit 0:63 */
> +			u64 id : 8;
> +			u64 _res0 : 3;
> +			u64 ssv : 1;
> +			u64 ssid : 20;
> +			u64 sid : 32;
> +			/* Bit 64:127 */
> +			u64 stag : 16;
> +			u64 _res1 : 15;
> +			u64 stall : 1;
> +			u64 _res2 : 1;
> +			u64 pnu : 1;
> +			u64 ind : 1;
> +			u64 rnw : 1;
> +			u64 _res3 : 2;
> +			u64 nsipa : 1;
> +			u64 s2 : 1;
> +			u64 class : 2;
> +			u64 _res4 : 6;
> +			u64 impl_def : 16;
> +			/* Bit 128:191 */
> +			u64 inputaddr;
> +			/* Bit 192:255 */
> +			u64 _res5 : 12;
> +			u64 ipa : 40;
> +			u64 _res6 : 12;
> +		} f_trans;
> +		/* FIXME Add other event structs */
> +	};
> +};
> 
> Then, event would be just:
> +		struct arm_smmu_event *event = (struct arm_smmu_event *)evt;
> 
> Not sure if Will would like this though...

Yea, I thought about this too but I felt that it'd bloat up the code
for every new event that's introduced. Also, the printing would become
more complicated as we'd have to log different fields for different
events. Additionally, I don't see that many unions being defined
elsewhere in the kernel.

...and, I'm not a fan of too many unions (personally) :) 

> 
> > +
> > +       mutex_lock(&smmu->streams_mutex);
> > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > +       mutex_unlock(&smmu->streams_mutex);
> 
> Same as I pointed out at the other patch, "master" is unprotected
> after the unlock. It can unlikely-yet-still-possibly race against
> arm_smmu_release_device.
>

Hmm.. are you suggesting that the `master` could've been removed by the
arm_smmu_release_device while we access it in an event handler?

As in, something like the following situation:

1. The evtq_thread gets scheduled
2. arm_smmu_release_device removes the `master` & its streams
3. In the `handle_evt` we dereference `master` which has been `kfree`ed
   (also, we don't return -EINVAL like we ideally should)

In that case, I think I should add back the `arm_smmu_find_master` to
the `arm_smmu_handle_evt` along with the locks. Nice catch! :)


> > +static void arm_smmu_dump_xlation_fault(struct arm_smmu_device *smmu,
> > +                                       struct arm_smmu_event *event)
> > +{
> > +       dev_err(smmu->dev, "Fault: %s client %s sid 0x%08x.0x%05x:\n",
> 
> "client" doesn't seem to be used anywhere in this driver, I would:
> s/client/master
> 

Ack, would change this to "master".

> And it'd feel clearer to have an "ssid". And perhaps adding commas
> to separate them too.

I referred to the log in `arm_smmu_handle_ppr` for the sid.ssid format.
If we'd prefer a separate "ssid", should we change the one in ppr too?
LMK, what you and Will think about that?

> 
> > +                       evts[event->id], event->master_name, event->sid, event->ssid);
> > +       dev_err(smmu->dev, "\tiova = %#llx ipa = %#llx (%s%s%s%s%s%s)\n", event->iova, event->ipa,
> > +                   event->privileged ? "Priv " : "Unpriv ",
> > +                   event->instruction ? "Inst " : "Data ",
> > +                   event->read ? "Read " : "Write ",
> > +                   event->stage ? "S2 " : "S1 ", class_str[event->class],
> > +                   ((event->id == EVT_ID_PERMISSION_FAULT) && (event->class == EVTQ_1_CLASS_TT)) ?
> > +                   (FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ? " TTD Read" : " TTD Write")
> > +                   : "");
> 
> Indentation should follow the existing printk() in this driver. And

I'm sorry but I'm not sure if I understand what is meant by "the
existing printk indentation in this driver", do you mean aligning the
next line with the opening brace? For example:

	dev_err(smmu->dev,
		"failed to allocate queue (0x%zx
		bytes) for %s\n",
		qsz, name);

> those ternary operators at the last field(s?) are hard to ready..
>

Hmm, I wanted the last fields to be printed only when the fault was
F_PERMISSION, in the same log. Any suggestions on what might make it
easier to read? Some helpful comments around it? Something else?

I just noticed that I've missed the `STAG` field in the in xlation_fault
dump, would add that too in the next version of this patch.

> > +static void arm_smmu_dump_fetch_fault(struct arm_smmu_device *smmu,
> > +                                     struct arm_smmu_event *event)
> > +{
> > +       dev_err(smmu->dev, "Bad fetch: %s client %s sid 0x%08x.0x%05x: fetch addr = %#llx\n",
> > +                       evts[event->id], event->master_name, event->sid, event->ssid, event->ipa);
> 
> Actually, the "Fault", "Bad fetch", and "Bad smmu config" doesn't
> feel very necessary, since we prints the event string already.
> 

That makes sense, I'll remove those in a follow up patch.
Although, I guess we should still say "fault" somewhere to hint folks
without arm-smmu-v3 knowledge that the event wasn't normal operation.

LMK what you think? I've had a few interactions where clients tend to
ignore the current "event received" dump considering that to be a part
of normal SMMU operation.

> > +}
> > +
> > +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: client %s:\n", event->id, event->master_name);
> 
> Looks like it would print another title that's sorta duplicated to
> other dump functions yet less informative?
>

Yea, I didn't wanna break backward compatibility, people might have
tools to parse out the existing dump, lol!

On a serious note, I believe it'd be better to print this as some
implmentations might have some "IMPLEMENTATION DEFINED" fields which
people might be interested to look at.

Although, we can dump the raw event only in the `default` case, i.e.
when we don't have a dumper function for that particular event ID but
that might still avoid printing the IMPL_DEFINED fields in fetch faults

> > +       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)
> > +{
> > +       switch (event->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[event->id], event->master_name, event->sid, event->ssid);
> > +               break;
> > +
> > +       case EVT_ID_TRANSLATION_FAULT:
> > +       case EVT_ID_ADDR_SIZE_FAULT:
> > +       case EVT_ID_ACCESS_FAULT:
> > +       case EVT_ID_PERMISSION_FAULT:
> > +               arm_smmu_dump_xlation_fault(smmu, event);
> > +               break;
> > +
> > +       case EVT_ID_STE_FETCH_FAULT:
> > +       case EVT_ID_CD_FETCH_FAULT:
> > +       case EVT_ID_VMS_FETCH_FAULT:
> > +               arm_smmu_dump_fetch_fault(smmu, event);
> > +               break;
> > +       }
> > +
> > +       arm_smmu_dump_raw_event(smmu, event);
> 
> I wonder if something like this would be cleaner:
> 
> 	...
> 	char title[256];
> 	char addrs[256];
> 	char other[256];
> 	int i;
> 
> 	switch () {
> 	case xxxx:
> 		/* If duplicated between cases, put them into helpers */
> 		snprintf(title, 256, "%s: Master %s, sid 0x%08x, ssid 0x%05x\n",
> 			 event_str(event->id], dev_name(master->dev),
> 			 event->sid, event->ssid);
> 		snprintf(addrs, 256, .....);
> 		snprintf(other, 256, .....);
> 		break;
> 	...
> 	default:
> 		/* snprintf raw title */
> 		break;
> 	}
> 	if (strlen(title))
> 		dev_err(smmu->dev, "%s", title);
> 	if (strlen(addrs))
> 		dev_err(smmu->dev, "%s", addrs);
> 	if (strlen(other))
> 		dev_err(smmu->dev, "%s", other);
> 	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> 		dev_err(smmu->dev, "\t0x%016llx\n", event->evt[i]);
> 	...
> 

I thought about something similar as well, but then referred to Robin's
comments on [1]. Also, printing strings in 3 different `dev_err` logs
could result in interruptions from other logs in dmesg, I'd prefer to
see the entire log in a single `dev_err` for ease. Although, I think we
should be able to do something like the following to achieve that:
	...

	dev_err(%s %s %s, strlen(title) ? title : ""
		strlen(addrs) ? addrs : ""
		strlen(other) ? other : ""

	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
		dev_err(smmu->dev, "\t0x%016llx\n", event->evt[i]);

	...

It does look cleaner to me as well. I'd wait for what other have to say.

> Thanks
> Nicolin

[1]
https://lore.kernel.org/linux-arm-kernel/07c7426c-7d01-4160-a344-1857b738e9c4@arm.com/T/#m9c38920897999c56f0b135556133cae38f40b754a

Thanks,
Pranjal

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

[Pranjal Shrivastava]

On Wed, Aug 28, 2024 at 10:20:02PM -0700, Nicolin Chen wrote:
> Hi Pranjal,
> 
> On Tue, Aug 27, 2024 at 12:30:26PM -0700, Pranjal Shrivastava wrote:
> 
> >         /* Stage-2 is always pinned at the moment */
> > -       if (evt[1] & EVTQ_1_S2)
> > +       if (event->stage)
> >                 return -EFAULT;
> 
> Should it be named to "s2" v.s stage?

Sure, will rename it with the next version.

> 
> > -       mutex_lock(&smmu->streams_mutex);
> > -       master = arm_smmu_find_master(smmu, sid);
> > -       if (!master) {
> > -               ret = -EINVAL;
> > -               goto out_unlock;
> > -       }
> > +       if (!event->master)
> > +               return -EINVAL;
> > 
> > -       ret = iommu_report_device_fault(master->dev, &fault_evt);
> > -out_unlock:
> > -       mutex_unlock(&smmu->streams_mutex);
> > +       ret = iommu_report_device_fault(event->master->dev, &fault_evt);
> 
> The iommu_report_device_fault(master->dev) call lost its mutex
> lock. I wonder if it could be unsafe to continue with that dev
> pointer.

Ack, I agree, responded with [Patch 1/2] in detail.

In summary, we need to avoid the following situation:

1. The evtq_thread gets scheduled
2. arm_smmu_release_device removes the `master` & its streams
3. In the `handle_evt` we dereference `master` which has been `kfree`ed
   (also, we don't return -EINVAL in that case as we ideally should)

I'll add back the `arm_smmu_find_master` in the the `arm_smmu_handle_evt`
along with the locks. Nice catch! :)

> 
> Nicolin

Thanks,
Pranjal

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

[Nicolin Chen]

On Thu, Aug 29, 2024 at 11:54:26PM +0000, Pranjal Shrivastava wrote:

> > > +static const char * const class_str[] = {
> > > +       [0] = "CD",
> > > +       [1] = "TTD",
> > > +       [2] = "IN",
> > > +       [3] = "RES",
> > > +};
> >
> > Unlike the event IDs, these class code names are still uneasy to
> > read. Though it'd result in a print-format change, yet could we
> > simply dump full strings instead?
> >
> 
> By "full strings" do you mean "CD => CD Fetch" as mentioned in the spec?

Yes.

> > > +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->stage = FIELD_GET(EVTQ_1_S2, evt[1]);
> > > +       event->read = FIELD_GET(EVTQ_1_RnW, evt[1]);
> > > +       event->raw = evt;
> >
> > Maybe we could define struct arm_smmu_event in this way:
> >
> > +struct arm_smmu_event {
> > +     union {
> > +             u64 evt[4];
> > +             struct {
> > +                     /* Bit 0:63 */
> > +                     u64 id : 8;
> > +                     u64 _res0 : 3;
> > +                     u64 ssv : 1;
> > +                     u64 ssid : 20;
> > +                     u64 sid : 32;
> > +                     /* Bit 64:127 */
> > +                     u64 stag : 16;
> > +                     u64 _res1 : 15;
> > +                     u64 stall : 1;
> > +                     u64 _res2 : 1;
> > +                     u64 pnu : 1;
> > +                     u64 ind : 1;
> > +                     u64 rnw : 1;
> > +                     u64 _res3 : 2;
> > +                     u64 nsipa : 1;
> > +                     u64 s2 : 1;
> > +                     u64 class : 2;
> > +                     u64 _res4 : 6;
> > +                     u64 impl_def : 16;
> > +                     /* Bit 128:191 */
> > +                     u64 inputaddr;
> > +                     /* Bit 192:255 */
> > +                     u64 _res5 : 12;
> > +                     u64 ipa : 40;
> > +                     u64 _res6 : 12;
> > +             } f_trans;
> > +             /* FIXME Add other event structs */
> > +     };
> > +};
> >
> > Then, event would be just:
> > +             struct arm_smmu_event *event = (struct arm_smmu_event *)evt;
> >
> > Not sure if Will would like this though...
> 
> Yea, I thought about this too but I felt that it'd bloat up the code
> for every new event that's introduced.

That'd be in the header. The IRQ Handler would be always clean
and fast.

> Also, the printing would become
> more complicated as we'd have to log different fields for different
> events. Additionally, I don't see that many unions being defined
> elsewhere in the kernel.

OK. That's a fair point. I think we could have just one common
union for the "good stuff" fields. Then, if something isn't in
the common union, do a FIELD_GET(raw)?

> > > +       mutex_lock(&smmu->streams_mutex);
> > > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > > +       mutex_unlock(&smmu->streams_mutex);
> >
> > Same as I pointed out at the other patch, "master" is unprotected
> > after the unlock. It can unlikely-yet-still-possibly race against
> > arm_smmu_release_device.
> >
> 
> Hmm.. are you suggesting that the `master` could've been removed by the
> arm_smmu_release_device while we access it in an event handler?
> 
> As in, something like the following situation:
> 
> 1. The evtq_thread gets scheduled
> 2. arm_smmu_release_device removes the `master` & its streams
> 3. In the `handle_evt` we dereference `master` which has been `kfree`ed
>    (also, we don't return -EINVAL like we ideally should)
> 
> In that case, I think I should add back the `arm_smmu_find_master` to
> the `arm_smmu_handle_evt` along with the locks. Nice catch! :)

Probably could lock the entire iteration, master pointer could
be then passed in safely between the helper functions.

> > And it'd feel clearer to have an "ssid". And perhaps adding commas
> > to separate them too.
> 
> I referred to the log in `arm_smmu_handle_ppr` for the sid.ssid format.
> If we'd prefer a separate "ssid", should we change the one in ppr too?
> LMK, what you and Will think about that?

Didn't realize arm_smmu_handle_ppr does that. It's fine to keep
the format in that way then.

> > > +                       evts[event->id], event->master_name, event->sid, event->ssid);
> > > +       dev_err(smmu->dev, "\tiova = %#llx ipa = %#llx (%s%s%s%s%s%s)\n", event->iova, event->ipa,
> > > +                   event->privileged ? "Priv " : "Unpriv ",
> > > +                   event->instruction ? "Inst " : "Data ",
> > > +                   event->read ? "Read " : "Write ",
> > > +                   event->stage ? "S2 " : "S1 ", class_str[event->class],
> > > +                   ((event->id == EVT_ID_PERMISSION_FAULT) && (event->class == EVTQ_1_CLASS_TT)) ?
> > > +                   (FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ? " TTD Read" : " TTD Write")
> > > +                   : "");
> >
> > Indentation should follow the existing printk() in this driver. And
> 
> I'm sorry but I'm not sure if I understand what is meant by "the
> existing printk indentation in this driver", do you mean aligning the
> next line with the opening brace? For example:
> 
>         dev_err(smmu->dev,
>                 "failed to allocate queue (0x%zx
>                 bytes) for %s\n",
>                 qsz, name);

Yea, just to keep the same coding style, line wrapping can still
happen to 80 characters in general though.

> > those ternary operators at the last field(s?) are hard to ready..
> >
> 
> Hmm, I wanted the last fields to be printed only when the fault was
> F_PERMISSION, in the same log. Any suggestions on what might make it
> easier to read? Some helpful comments around it? Something else?

With an "other" string, we could sprintf on conditions inside the
"case F_PERMISSION:".

> > > +static void arm_smmu_dump_fetch_fault(struct arm_smmu_device *smmu,
> > > +                                     struct arm_smmu_event *event)
> > > +{
> > > +       dev_err(smmu->dev, "Bad fetch: %s client %s sid 0x%08x.0x%05x: fetch addr = %#llx\n",
> > > +                       evts[event->id], event->master_name, event->sid, event->ssid, event->ipa);
> >
> > Actually, the "Fault", "Bad fetch", and "Bad smmu config" doesn't
> > feel very necessary, since we prints the event string already.
> >
> 
> That makes sense, I'll remove those in a follow up patch.
> Although, I guess we should still say "fault" somewhere to hint folks
> without arm-smmu-v3 knowledge that the event wasn't normal operation.
> 
> LMK what you think? I've had a few interactions where clients tend to
> ignore the current "event received" dump considering that to be a part
> of normal SMMU operation.

Well, we could improve the event_str with human-readable ones:
	s/F_TRANSLATION/Translation\ Fault

> > > +}
> > > +
> > > +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: client %s:\n", event->id, event->master_name);
> >
> > Looks like it would print another title that's sorta duplicated to
> > other dump functions yet less informative?
> >
> 
> Yea, I didn't wanna break backward compatibility, people might have
> tools to parse out the existing dump, lol!
>
> On a serious note, I believe it'd be better to print this as some
> implmentations might have some "IMPLEMENTATION DEFINED" fields which
> people might be interested to look at.
> 
> Although, we can dump the raw event only in the `default` case, i.e.
> when we don't have a dumper function for that particular event ID but
> that might still avoid printing the IMPL_DEFINED fields in fetch faults

Makes sense to me by having a different title for the default case.

> > > +       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,
> > > +    
> 
> I thought about something similar as well, but then referred to Robin's
> comments on [1]. Also, printing strings in 3 different `dev_err` logs
> could result in interruptions from other logs in dmesg, I'd prefer to

Ack.

> see the entire log in a single `dev_err` for ease. Although, I think we
> should be able to do something like the following to achieve that:
>         ...
> 
>         dev_err(%s %s %s, strlen(title) ? title : ""
>                 strlen(addrs) ? addrs : ""
>                 strlen(other) ? other : ""

That is fine, though should break the lines too. Maybe:
	dev_err(smmu->dev, "%s%s%s%s%s\n", title,
		strlen(addrs) ? "\n" : "", addrs,
		strlen(other) ? "\n" : "", other);
?

> 
>         for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
>                 dev_err(smmu->dev, "\t0x%016llx\n", event->evt[i]);

Maybe merge the four dev_err iterations too?

Nicolin

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

[Pranjal Shrivastava]

On Thu, Aug 29, 2024 at 06:45:53PM -0700, Nicolin Chen wrote:
> On Thu, Aug 29, 2024 at 11:54:26PM +0000, Pranjal Shrivastava wrote:
> 
> > > > +static const char * const class_str[] = {
> > > > +       [0] = "CD",
> > > > +       [1] = "TTD",
> > > > +       [2] = "IN",
> > > > +       [3] = "RES",
> > > > +};
> > >
> > > Unlike the event IDs, these class code names are still uneasy to
> > > read. Though it'd result in a print-format change, yet could we
> > > simply dump full strings instead?
> > >
> > 
> > By "full strings" do you mean "CD => CD Fetch" as mentioned in the spec?
> 
> Yes.

Ack. So, just for confirmation we want the following 4 class strings:
"CD Fetch"
"Stage 1 translation table fetch"
"Input address caused fault"
"Reserved"

Right?

> 
> > > > +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->stage = FIELD_GET(EVTQ_1_S2, evt[1]);
> > > > +       event->read = FIELD_GET(EVTQ_1_RnW, evt[1]);
> > > > +       event->raw = evt;
> > >
> > > Maybe we could define struct arm_smmu_event in this way:
> > >
> > > +struct arm_smmu_event {
> > > +     union {
> > > +             u64 evt[4];
> > > +             struct {
> > > +                     /* Bit 0:63 */
> > > +                     u64 id : 8;
> > > +                     u64 _res0 : 3;
> > > +                     u64 ssv : 1;
> > > +                     u64 ssid : 20;
> > > +                     u64 sid : 32;
> > > +                     /* Bit 64:127 */
> > > +                     u64 stag : 16;
> > > +                     u64 _res1 : 15;
> > > +                     u64 stall : 1;
> > > +                     u64 _res2 : 1;
> > > +                     u64 pnu : 1;
> > > +                     u64 ind : 1;
> > > +                     u64 rnw : 1;
> > > +                     u64 _res3 : 2;
> > > +                     u64 nsipa : 1;
> > > +                     u64 s2 : 1;
> > > +                     u64 class : 2;
> > > +                     u64 _res4 : 6;
> > > +                     u64 impl_def : 16;
> > > +                     /* Bit 128:191 */
> > > +                     u64 inputaddr;
> > > +                     /* Bit 192:255 */
> > > +                     u64 _res5 : 12;
> > > +                     u64 ipa : 40;
> > > +                     u64 _res6 : 12;
> > > +             } f_trans;
> > > +             /* FIXME Add other event structs */
> > > +     };
> > > +};
> > >
> > > Then, event would be just:
> > > +             struct arm_smmu_event *event = (struct arm_smmu_event *)evt;
> > >
> > > Not sure if Will would like this though...
> > 
> > Yea, I thought about this too but I felt that it'd bloat up the code
> > for every new event that's introduced.
> 
> That'd be in the header. The IRQ Handler would be always clean
> and fast.
> 
> > Also, the printing would become
> > more complicated as we'd have to log different fields for different
> > events. Additionally, I don't see that many unions being defined
> > elsewhere in the kernel.
> 
> OK. That's a fair point. I think we could have just one common
> union for the "good stuff" fields. Then, if something isn't in
> the common union, do a FIELD_GET(raw)?
> 

I'm not sure if I get this right, but are you suggesting something like:

+struct arm_smmu_event {
+	union {
+	u64 raw_evt[4];
+	struct {
+		/* "Good stuff" fields */
+	};
+};

and then based on the fault type we can use the "good stuff" or raw_evt?
So, basically just add a union between raw_evt and the other fields to
improve struct arm_smmu_event present in v2?

> > > > +       mutex_lock(&smmu->streams_mutex);
> > > > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > > > +       mutex_unlock(&smmu->streams_mutex);
> > >
> > > Same as I pointed out at the other patch, "master" is unprotected
> > > after the unlock. It can unlikely-yet-still-possibly race against
> > > arm_smmu_release_device.
> > >
> > 
> > Hmm.. are you suggesting that the `master` could've been removed by the
> > arm_smmu_release_device while we access it in an event handler?
> > 
> > As in, something like the following situation:
> > 
> > 1. The evtq_thread gets scheduled
> > 2. arm_smmu_release_device removes the `master` & its streams
> > 3. In the `handle_evt` we dereference `master` which has been `kfree`ed
> >    (also, we don't return -EINVAL like we ideally should)
> > 
> > In that case, I think I should add back the `arm_smmu_find_master` to
> > the `arm_smmu_handle_evt` along with the locks. Nice catch! :)
> 
> Probably could lock the entire iteration, master pointer could
> be then passed in safely between the helper functions.

I'm just wondering if that'd be too much to print the "master_name", I
mean what if we simply save the master_name in `struct arm_smmu_event`?
That way, we can keep the locking as is in `arm_smmu_handle_evt` and
simply print the stored "master_name".

Note: I'm suggesting to store the entire string and not just the ptr
returned by dev_name(master->dev)), something like: 

`strcpy(event->master_name, dev_name(master->dev))`

> 
> > > And it'd feel clearer to have an "ssid". And perhaps adding commas
> > > to separate them too.
> > 
> > I referred to the log in `arm_smmu_handle_ppr` for the sid.ssid format.
> > If we'd prefer a separate "ssid", should we change the one in ppr too?
> > LMK, what you and Will think about that?
> 
> Didn't realize arm_smmu_handle_ppr does that. It's fine to keep
> the format in that way then.

Ack.

> 
> > > > +                       evts[event->id], event->master_name, event->sid, event->ssid);
> > > > +       dev_err(smmu->dev, "\tiova = %#llx ipa = %#llx (%s%s%s%s%s%s)\n", event->iova, event->ipa,
> > > > +                   event->privileged ? "Priv " : "Unpriv ",
> > > > +                   event->instruction ? "Inst " : "Data ",
> > > > +                   event->read ? "Read " : "Write ",
> > > > +                   event->stage ? "S2 " : "S1 ", class_str[event->class],
> > > > +                   ((event->id == EVT_ID_PERMISSION_FAULT) && (event->class == EVTQ_1_CLASS_TT)) ?
> > > > +                   (FIELD_GET(EVTQ_1_TT_READ, event->raw[1]) ? " TTD Read" : " TTD Write")
> > > > +                   : "");
> > >
> > > Indentation should follow the existing printk() in this driver. And
> > 
> > I'm sorry but I'm not sure if I understand what is meant by "the
> > existing printk indentation in this driver", do you mean aligning the
> > next line with the opening brace? For example:
> > 
> >         dev_err(smmu->dev,
> >                 "failed to allocate queue (0x%zx
> >                 bytes) for %s\n",
> >                 qsz, name);
> 
> Yea, just to keep the same coding style, line wrapping can still
> happen to 80 characters in general though.
> 

Ack.

> > > those ternary operators at the last field(s?) are hard to ready..
> > >
> > 
> > Hmm, I wanted the last fields to be printed only when the fault was
> > F_PERMISSION, in the same log. Any suggestions on what might make it
> > easier to read? Some helpful comments around it? Something else?
> 
> With an "other" string, we could sprintf on conditions inside the
> "case F_PERMISSION:".
> 

Ohh alright, basically we append something extra in the "other" string
for `F_PERMISSION`. Got it.

> > > > +static void arm_smmu_dump_fetch_fault(struct arm_smmu_device *smmu,
> > > > +                                     struct arm_smmu_event *event)
> > > > +{
> > > > +       dev_err(smmu->dev, "Bad fetch: %s client %s sid 0x%08x.0x%05x: fetch addr = %#llx\n",
> > > > +                       evts[event->id], event->master_name, event->sid, event->ssid, event->ipa);
> > >
> > > Actually, the "Fault", "Bad fetch", and "Bad smmu config" doesn't
> > > feel very necessary, since we prints the event string already.
> > >
> > 
> > That makes sense, I'll remove those in a follow up patch.
> > Although, I guess we should still say "fault" somewhere to hint folks
> > without arm-smmu-v3 knowledge that the event wasn't normal operation.
> > 
> > LMK what you think? I've had a few interactions where clients tend to
> > ignore the current "event received" dump considering that to be a part
> > of normal SMMU operation.
> 
> Well, we could improve the event_str with human-readable ones:
> 	s/F_TRANSLATION/Translation\ Fault
> 

Yea, but I'd still want to see a "spec searchable" name for the fault.
Maybe we can have "Unexpected event recieved:<spec_fault_name>" in the
"title" string?

> > > > +}
> > > > +
> > > > +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: client %s:\n", event->id, event->master_name);
> > >
> > > Looks like it would print another title that's sorta duplicated to
> > > other dump functions yet less informative?
> > >
> > 
> > Yea, I didn't wanna break backward compatibility, people might have
> > tools to parse out the existing dump, lol!
> >
> > On a serious note, I believe it'd be better to print this as some
> > implmentations might have some "IMPLEMENTATION DEFINED" fields which
> > people might be interested to look at.
> > 
> > Although, we can dump the raw event only in the `default` case, i.e.
> > when we don't have a dumper function for that particular event ID but
> > that might still avoid printing the IMPL_DEFINED fields in fetch faults
> 
> Makes sense to me by having a different title for the default case.
> 

Ack, we can have a different title for the default case. However, on a
second thought, I believe we should log the "raw" event in all cases,
since we aren't printing all the fields anyway. For example, for
F_TRANSLATION we don't print IMPL_DEF fields, NSIPA etc. It might be 
helpful to see the raw event even for the "non-default" cases.

> > > > +       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,
> > > > +    
> > 
> > I thought about something similar as well, but then referred to Robin's
> > comments on [1]. Also, printing strings in 3 different `dev_err` logs
> > could result in interruptions from other logs in dmesg, I'd prefer to
> 
> Ack.
> 
> > see the entire log in a single `dev_err` for ease. Although, I think we
> > should be able to do something like the following to achieve that:
> >         ...
> > 
> >         dev_err(%s %s %s, strlen(title) ? title : ""
> >                 strlen(addrs) ? addrs : ""
> >                 strlen(other) ? other : ""
> 
> That is fine, though should break the lines too. Maybe:
> 	dev_err(smmu->dev, "%s%s%s%s%s\n", title,
> 		strlen(addrs) ? "\n" : "", addrs,
> 		strlen(other) ? "\n" : "", other);
> ?
> 

I'd like line-feeds too, but I'm unsure if that could cause dmesg log
interruptions? I assume adding a "\n" flushes the console buffer, i.e.
we might get interrupted logs (I maybe wrong here).

> > 
> >         for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> >                 dev_err(smmu->dev, "\t0x%016llx\n", event->evt[i]);
> 
> Maybe merge the four dev_err iterations too?

Ack.

> 
> Nicolin

Thanks,
Pranjal

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

[Nicolin Chen]

On Mon, Sep 02, 2024 at 08:23:20AM +0000, Pranjal Shrivastava wrote:

> On Thu, Aug 29, 2024 at 06:45:53PM -0700, Nicolin Chen wrote:
> > On Thu, Aug 29, 2024 at 11:54:26PM +0000, Pranjal Shrivastava wrote:
> >
> > > > > +static const char * const class_str[] = {
> > > > > +       [0] = "CD",
> > > > > +       [1] = "TTD",
> > > > > +       [2] = "IN",
> > > > > +       [3] = "RES",
> > > > > +};
> > > >
> > > > Unlike the event IDs, these class code names are still uneasy to
> > > > read. Though it'd result in a print-format change, yet could we
> > > > simply dump full strings instead?
> > > >
> > >
> > > By "full strings" do you mean "CD => CD Fetch" as mentioned in the spec?
> >
> > Yes.
> 
> Ack. So, just for confirmation we want the following 4 class strings:
> "CD Fetch"
> "Stage 1 translation table fetch"
> "Input address caused fault"
> "Reserved"
> 
> Right?

Yes. I know they are longer, but more readable. So, you might want
to arrange the output format to present them nicely.

> > > Also, the printing would become
> > > more complicated as we'd have to log different fields for different
> > > events. Additionally, I don't see that many unions being defined
> > > elsewhere in the kernel.
> >
> > OK. That's a fair point. I think we could have just one common
> > union for the "good stuff" fields. Then, if something isn't in
> > the common union, do a FIELD_GET(raw)?
> >
> 
> I'm not sure if I get this right, but are you suggesting something like:
> 
> +struct arm_smmu_event {
> +       union {
> +       u64 raw_evt[4];
> +       struct {
> +               /* "Good stuff" fields */
> +       };
> +};
> 
> and then based on the fault type we can use the "good stuff" or raw_evt?
> So, basically just add a union between raw_evt and the other fields to
> improve struct arm_smmu_event present in v2?

Yea, I attached a test code at the EOM for your reference. Please
feel free to drop fields if they aren't common enough, and confirm
those bits are correctly written too.

> > > > > +       mutex_lock(&smmu->streams_mutex);
> > > > > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > > > > +       mutex_unlock(&smmu->streams_mutex);
> > > >
> > > > Same as I pointed out at the other patch, "master" is unprotected
> > > > after the unlock. It can unlikely-yet-still-possibly race against
> > > > arm_smmu_release_device.
> > > >
> > >
> > > Hmm.. are you suggesting that the `master` could've been removed by the
> > > arm_smmu_release_device while we access it in an event handler?
> > >
> > > As in, something like the following situation:
> > >
> > > 1. The evtq_thread gets scheduled
> > > 2. arm_smmu_release_device removes the `master` & its streams
> > > 3. In the `handle_evt` we dereference `master` which has been `kfree`ed
> > >    (also, we don't return -EINVAL like we ideally should)
> > >
> > > In that case, I think I should add back the `arm_smmu_find_master` to
> > > the `arm_smmu_handle_evt` along with the locks. Nice catch! :)
> >
> > Probably could lock the entire iteration, master pointer could
> > be then passed in safely between the helper functions.
> 
> I'm just wondering if that'd be too much to print the "master_name", I
> mean what if we simply save the master_name in `struct arm_smmu_event`?
> That way, we can keep the locking as is in `arm_smmu_handle_evt` and
> simply print the stored "master_name".
> 
> Note: I'm suggesting to store the entire string and not just the ptr
> returned by dev_name(master->dev)), something like:
> 
> `strcpy(event->master_name, dev_name(master->dev))`

I'd probably move the dump() call inside arm_smmu_handle_evt(), and
within the lock to avoid strcpy. And eventually it would be located
at "else { /* Unhandled events should be pinned */ ret = -EFAULT; }:
https://lore.kernel.org/linux-iommu/8b93be1d913f9e227748de2d07e8540ddc2372ab.1724777091.git.nicolinc@nvidia.com/

> > > > Actually, the "Fault", "Bad fetch", and "Bad smmu config" doesn't
> > > > feel very necessary, since we prints the event string already.
> > > >
> > >
> > > That makes sense, I'll remove those in a follow up patch.
> > > Although, I guess we should still say "fault" somewhere to hint folks
> > > without arm-smmu-v3 knowledge that the event wasn't normal operation.
> > >
> > > LMK what you think? I've had a few interactions where clients tend to
> > > ignore the current "event received" dump considering that to be a part
> > > of normal SMMU operation.
> >
> > Well, we could improve the event_str with human-readable ones:
> >       s/F_TRANSLATION/Translation\ Fault
> >
> 
> Yea, but I'd still want to see a "spec searchable" name for the fault.
> Maybe we can have "Unexpected event recieved:<spec_fault_name>" in the
> "title" string?

That looks good to me.

> > > Although, we can dump the raw event only in the `default` case, i.e.
> > > when we don't have a dumper function for that particular event ID but
> > > that might still avoid printing the IMPL_DEFINED fields in fetch faults
> >
> > Makes sense to me by having a different title for the default case.
> >
> 
> Ack, we can have a different title for the default case. However, on a
> second thought, I believe we should log the "raw" event in all cases,
> since we aren't printing all the fields anyway. For example, for
> F_TRANSLATION we don't print IMPL_DEF fields, NSIPA etc. It might be
> helpful to see the raw event even for the "non-default" cases.

That makes sense. I'd dump the raw dwords outside the switch-case,
i.e. in the common path.

> > That is fine, though should break the lines too. Maybe:
> >       dev_err(smmu->dev, "%s%s%s%s%s\n", title,
> >               strlen(addrs) ? "\n" : "", addrs,
> >               strlen(other) ? "\n" : "", other);
> > ?
> >
> 
> I'd like line-feeds too, but I'm unsure if that could cause dmesg log
> interruptions? I assume adding a "\n" flushes the console buffer, i.e.
> we might get interrupted logs (I maybe wrong here).

I think the console_lock is grabbed per printk call, not per "\n".

Thanks
Nicolin

-------------------------------------------------------------------------------
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 6c48b53fc2b8..6b1ca9379999 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1894,6 +1894,37 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
 	return ret;
 }
 
+union arm_smmu_event {
+	u64 evt[EVTQ_ENT_DWORDS];
+	struct {
+	/* Bit 0:63 */
+	u64 id : 8;
+	u64 _res0 : 3;
+	u64 ssv : 1;
+	u64 ssid : 20;
+	u64 sid : 32;
+	/* Bit 64:127 */
+	u64 stag : 16;
+	u64 _res1 : 15;
+	u64 stall : 1;
+	u64 _res2 : 1;
+	u64 pnu : 1;
+	u64 ind : 1;
+	u64 rnw : 1;
+	u64 _res3 : 2;
+	u64 nsipa : 1;
+	u64 s2 : 1;
+	u64 class : 2;
+	u64 _res4 : 6;
+	u64 impl_def : 16;
+	/* Bit 128:191 */
+	u64 addr1;
+	/* Bit 192:255 */
+	u64 addr2: 56;
+	u64 _res5: 8;
+	};
+};
+
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
 	int i, ret;
@@ -1902,20 +1933,27 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 	struct arm_smmu_ll_queue *llq = &q->llq;
 	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
 				      DEFAULT_RATELIMIT_BURST);
-	u64 evt[EVTQ_ENT_DWORDS];
+	union arm_smmu_event event;
 
 	do {
-		while (!queue_remove_raw(q, evt)) {
-			u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
+		while (!queue_remove_raw(q, event.evt)) {
+			u8 id = FIELD_GET(EVTQ_0_ID, event.evt[0]);
 
-			ret = arm_smmu_handle_evt(smmu, evt);
+			ret = arm_smmu_handle_evt(smmu, event.evt);
 			if (!ret || !__ratelimit(&rs))
 				continue;
 
 			dev_info(smmu->dev, "event 0x%02x received:\n", id);
-			for (i = 0; i < ARRAY_SIZE(evt); ++i)
+			for (i = 0; i < ARRAY_SIZE(event.evt); ++i)
 				dev_info(smmu->dev, "\t0x%016llx\n",
-					 (unsigned long long)evt[i]);
+					 event.evt[i]);
+			dev_info(smmu->dev, "id=%d\n", event.id);
+			dev_info(smmu->dev, "sid=%x\n", event.sid);
+			dev_info(smmu->dev, "class=%d\n", event.class);
+			dev_info(smmu->dev, "s2=%d\n", event.s2);
+			dev_info(smmu->dev, "inputaddr=%llx\n", event.addr1);
+			dev_info(smmu->dev, "ipa=%llx\n",
+					(u64)event.addr2 & GENMASK(55, 12));
 
 			cond_resched();
 		}

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

[Pranjal Shrivastava]

On Mon, Sep 02, 2024 at 04:02:44PM -0700, Nicolin Chen wrote:
> On Mon, Sep 02, 2024 at 08:23:20AM +0000, Pranjal Shrivastava wrote:
> 
> > On Thu, Aug 29, 2024 at 06:45:53PM -0700, Nicolin Chen wrote:
> > > On Thu, Aug 29, 2024 at 11:54:26PM +0000, Pranjal Shrivastava wrote:
> > >
> > > > > > +static const char * const class_str[] = {
> > > > > > +       [0] = "CD",
> > > > > > +       [1] = "TTD",
> > > > > > +       [2] = "IN",
> > > > > > +       [3] = "RES",
> > > > > > +};
> > > > >
> > > > > Unlike the event IDs, these class code names are still uneasy to
> > > > > read. Though it'd result in a print-format change, yet could we
> > > > > simply dump full strings instead?
> > > > >
> > > >
> > > > By "full strings" do you mean "CD => CD Fetch" as mentioned in the spec?
> > >
> > > Yes.
> > 
> > Ack. So, just for confirmation we want the following 4 class strings:
> > "CD Fetch"
> > "Stage 1 translation table fetch"
> > "Input address caused fault"
> > "Reserved"
> > 
> > Right?
> 
> Yes. I know they are longer, but more readable. So, you might want
> to arrange the output format to present them nicely.
> 

Ack, thanks for confirming!

> > > > Also, the printing would become
> > > > more complicated as we'd have to log different fields for different
> > > > events. Additionally, I don't see that many unions being defined
> > > > elsewhere in the kernel.
> > >
> > > OK. That's a fair point. I think we could have just one common
> > > union for the "good stuff" fields. Then, if something isn't in
> > > the common union, do a FIELD_GET(raw)?
> > >
> > 
> > I'm not sure if I get this right, but are you suggesting something like:
> > 
> > +struct arm_smmu_event {
> > +       union {
> > +       u64 raw_evt[4];
> > +       struct {
> > +               /* "Good stuff" fields */
> > +       };
> > +};
> > 
> > and then based on the fault type we can use the "good stuff" or raw_evt?
> > So, basically just add a union between raw_evt and the other fields to
> > improve struct arm_smmu_event present in v2?
> 
> Yea, I attached a test code at the EOM for your reference. Please
> feel free to drop fields if they aren't common enough, and confirm
> those bits are correctly written too.
> 

Hmm, so you're suggesting to make the event a union instead of a struct
thus, making reading event fields easy.

The only thing I'm slightly worried about is if the bitfields don't
remain constant if more events are added in the future.

For example, the "IPA" field is in dword[3] for now, but if it changes
in the future, it may get difficult to scale the union for future events
If we can guarantee that it doesn't happen then I think union is better.

Will, do you have any suggestions here?

> > > > > > +       mutex_lock(&smmu->streams_mutex);
> > > > > > +       event->master = arm_smmu_find_master(smmu, event->sid);
> > > > > > +       mutex_unlock(&smmu->streams_mutex);
> > > > >
> > > > > Same as I pointed out at the other patch, "master" is unprotected
> > > > > after the unlock. It can unlikely-yet-still-possibly race against
> > > > > arm_smmu_release_device.
> > > > >
> > > >
> > > > Hmm.. are you suggesting that the `master` could've been removed by the
> > > > arm_smmu_release_device while we access it in an event handler?
> > > >
> > > > As in, something like the following situation:
> > > >
> > > > 1. The evtq_thread gets scheduled
> > > > 2. arm_smmu_release_device removes the `master` & its streams
> > > > 3. In the `handle_evt` we dereference `master` which has been `kfree`ed
> > > >    (also, we don't return -EINVAL like we ideally should)
> > > >
> > > > In that case, I think I should add back the `arm_smmu_find_master` to
> > > > the `arm_smmu_handle_evt` along with the locks. Nice catch! :)
> > >
> > > Probably could lock the entire iteration, master pointer could
> > > be then passed in safely between the helper functions.
> > 
> > I'm just wondering if that'd be too much to print the "master_name", I
> > mean what if we simply save the master_name in `struct arm_smmu_event`?
> > That way, we can keep the locking as is in `arm_smmu_handle_evt` and
> > simply print the stored "master_name".
> > 
> > Note: I'm suggesting to store the entire string and not just the ptr
> > returned by dev_name(master->dev)), something like:
> > 
> > `strcpy(event->master_name, dev_name(master->dev))`
> 
> I'd probably move the dump() call inside arm_smmu_handle_evt(), and
> within the lock to avoid strcpy. And eventually it would be located
> at "else { /* Unhandled events should be pinned */ ret = -EFAULT; }:
> https://lore.kernel.org/linux-iommu/8b93be1d913f9e227748de2d07e8540ddc2372ab.1724777091.git.nicolinc@nvidia.com/
> 

I think we'll still need to dump the event for other cases, such as the
case where the master wasn't found, we return `-EINVAL` and the case
where EVTQ_1_S2 is set. I guess, we should have a case under the label
`out_unlock` we can call the dump function based on the ret value. E.g.:

out_unlock:
+	if (ret)
+		arm_smmu_dump_event(smmu, evt);
 	mutex_unlock(&smmu->streams_mutex);
 	return ret;

> > > > > Actually, the "Fault", "Bad fetch", and "Bad smmu config" doesn't
> > > > > feel very necessary, since we prints the event string already.
> > > > >
> > > >
> > > > That makes sense, I'll remove those in a follow up patch.
> > > > Although, I guess we should still say "fault" somewhere to hint folks
> > > > without arm-smmu-v3 knowledge that the event wasn't normal operation.
> > > >
> > > > LMK what you think? I've had a few interactions where clients tend to
> > > > ignore the current "event received" dump considering that to be a part
> > > > of normal SMMU operation.
> > >
> > > Well, we could improve the event_str with human-readable ones:
> > >       s/F_TRANSLATION/Translation\ Fault
> > >
> > 
> > Yea, but I'd still want to see a "spec searchable" name for the fault.
> > Maybe we can have "Unexpected event recieved:<spec_fault_name>" in the
> > "title" string?
> 
> That looks good to me.
> 

Ack.

> > > > Although, we can dump the raw event only in the `default` case, i.e.
> > > > when we don't have a dumper function for that particular event ID but
> > > > that might still avoid printing the IMPL_DEFINED fields in fetch faults
> > >
> > > Makes sense to me by having a different title for the default case.
> > >
> > 
> > Ack, we can have a different title for the default case. However, on a
> > second thought, I believe we should log the "raw" event in all cases,
> > since we aren't printing all the fields anyway. For example, for
> > F_TRANSLATION we don't print IMPL_DEF fields, NSIPA etc. It might be
> > helpful to see the raw event even for the "non-default" cases.
> 
> That makes sense. I'd dump the raw dwords outside the switch-case,
> i.e. in the common path.
> 

Ack. I'd dump the raw dwords outside the switch-case in v3.

> > > That is fine, though should break the lines too. Maybe:
> > >       dev_err(smmu->dev, "%s%s%s%s%s\n", title,
> > >               strlen(addrs) ? "\n" : "", addrs,
> > >               strlen(other) ? "\n" : "", other);
> > > ?
> > >
> > 
> > I'd like line-feeds too, but I'm unsure if that could cause dmesg log
> > interruptions? I assume adding a "\n" flushes the console buffer, i.e.
> > we might get interrupted logs (I maybe wrong here).
> 
> I think the console_lock is grabbed per printk call, not per "\n".

Ahh okay! I took the opportunity to read up the printk implementation.
I see the `vprintk_store` stores a single log as a unit in the buffer,
and the `console_unlock` actually flushes it out to the console. That
said, I think nothing (other than hard IRQs printing logs) should cause
interleaving between the log. Interesting stuff!

> 
> Thanks
> Nicolin

Thanks,
Pranjal

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

[Nicolin Chen]

On Thu, Sep 05, 2024 at 04:06:24PM +0000, Pranjal Shrivastava wrote:

> > > > > Also, the printing would become
> > > > > more complicated as we'd have to log different fields for different
> > > > > events. Additionally, I don't see that many unions being defined
> > > > > elsewhere in the kernel.
> > > >
> > > > OK. That's a fair point. I think we could have just one common
> > > > union for the "good stuff" fields. Then, if something isn't in
> > > > the common union, do a FIELD_GET(raw)?
> > > >
> > >
> > > I'm not sure if I get this right, but are you suggesting something like:
> > >
> > > +struct arm_smmu_event {
> > > +       union {
> > > +       u64 raw_evt[4];
> > > +       struct {
> > > +               /* "Good stuff" fields */
> > > +       };
> > > +};
> > >
> > > and then based on the fault type we can use the "good stuff" or raw_evt?
> > > So, basically just add a union between raw_evt and the other fields to
> > > improve struct arm_smmu_event present in v2?
> >
> > Yea, I attached a test code at the EOM for your reference. Please
> > feel free to drop fields if they aren't common enough, and confirm
> > those bits are correctly written too.
> >
> 
> Hmm, so you're suggesting to make the event a union instead of a struct
> thus, making reading event fields easy.
> 
> The only thing I'm slightly worried about is if the bitfields don't
> remain constant if more events are added in the future.
> 
> For example, the "IPA" field is in dword[3] for now, but if it changes
> in the future, it may get difficult to scale the union for future events
> If we can guarantee that it doesn't happen then I think union is better.

I believe HW has its own reason to put those fields constantly
following the same pattern, and must have its tendency to keep
in that way.

The "union { u64 raw; struct {} }" way isn't that uncommon in
the kernel, here are a few existing examples:
	drivers/crypto/marvell/octeontx2/otx2_cpt_hw_types.h
	drivers/crypto/cavium/nitrox/nitrox_req.h
	include/asm-generic/hyperv-tlfs.h

Taking a step back, if one field in a new event in the future
happens to be odd, we could still fetch field in its own way:
event->evt[4] is still available to extract via FIELD_GET; or
just define another union exclusively for that event, neither
of which sounds like the end of world.

Having said that, I'd defer to Will. So, here is just my two
cents.

> > > I'm just wondering if that'd be too much to print the "master_name", I
> > > mean what if we simply save the master_name in `struct arm_smmu_event`?
> > > That way, we can keep the locking as is in `arm_smmu_handle_evt` and
> > > simply print the stored "master_name".
> > >
> > > Note: I'm suggesting to store the entire string and not just the ptr
> > > returned by dev_name(master->dev)), something like:
> > >
> > > `strcpy(event->master_name, dev_name(master->dev))`
> >
> > I'd probably move the dump() call inside arm_smmu_handle_evt(), and
> > within the lock to avoid strcpy. And eventually it would be located
> > at "else { /* Unhandled events should be pinned */ ret = -EFAULT; }:
> > https://lore.kernel.org/linux-iommu/8b93be1d913f9e227748de2d07e8540ddc2372ab.1724777091.git.nicolinc@nvidia.com/
> >
> 
> I think we'll still need to dump the event for other cases, such as the
> case where the master wasn't found, we return `-EINVAL` and the case
> where EVTQ_1_S2 is set. I guess, we should have a case under the label
> `out_unlock` we can call the dump function based on the ret value. E.g.:
> 
> out_unlock:
> +       if (ret)
> +               arm_smmu_dump_event(smmu, evt);
>         mutex_unlock(&smmu->streams_mutex);
>         return ret;

Ah, that's thoughtful. I missed it. I agree.

Thanks
Nicolin

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

[Will Deacon]

On Thu, Sep 05, 2024 at 06:55:33PM -0700, Nicolin Chen wrote:
> On Thu, Sep 05, 2024 at 04:06:24PM +0000, Pranjal Shrivastava wrote:
> 
> > > > > > Also, the printing would become
> > > > > > more complicated as we'd have to log different fields for different
> > > > > > events. Additionally, I don't see that many unions being defined
> > > > > > elsewhere in the kernel.
> > > > >
> > > > > OK. That's a fair point. I think we could have just one common
> > > > > union for the "good stuff" fields. Then, if something isn't in
> > > > > the common union, do a FIELD_GET(raw)?
> > > > >
> > > >
> > > > I'm not sure if I get this right, but are you suggesting something like:
> > > >
> > > > +struct arm_smmu_event {
> > > > +       union {
> > > > +       u64 raw_evt[4];
> > > > +       struct {
> > > > +               /* "Good stuff" fields */
> > > > +       };
> > > > +};
> > > >
> > > > and then based on the fault type we can use the "good stuff" or raw_evt?
> > > > So, basically just add a union between raw_evt and the other fields to
> > > > improve struct arm_smmu_event present in v2?
> > >
> > > Yea, I attached a test code at the EOM for your reference. Please
> > > feel free to drop fields if they aren't common enough, and confirm
> > > those bits are correctly written too.
> > >
> > 
> > Hmm, so you're suggesting to make the event a union instead of a struct
> > thus, making reading event fields easy.
> > 
> > The only thing I'm slightly worried about is if the bitfields don't
> > remain constant if more events are added in the future.
> > 
> > For example, the "IPA" field is in dword[3] for now, but if it changes
> > in the future, it may get difficult to scale the union for future events
> > If we can guarantee that it doesn't happen then I think union is better.
> 
> I believe HW has its own reason to put those fields constantly
> following the same pattern, and must have its tendency to keep
> in that way.
> 
> The "union { u64 raw; struct {} }" way isn't that uncommon in
> the kernel, here are a few existing examples:
> 	drivers/crypto/marvell/octeontx2/otx2_cpt_hw_types.h
> 	drivers/crypto/cavium/nitrox/nitrox_req.h
> 	include/asm-generic/hyperv-tlfs.h
> 
> Taking a step back, if one field in a new event in the future
> happens to be odd, we could still fetch field in its own way:
> event->evt[4] is still available to extract via FIELD_GET; or
> just define another union exclusively for that event, neither
> of which sounds like the end of world.
> 
> Having said that, I'd defer to Will. So, here is just my two
> cents.

My only real concern is the fragility of using bitfields. I don't _think_
the compiler is obliged to lay them out in the obvious way and I can't
think of anything worse than being given a bad pretty-print when debugging
a real driver issue!

Will

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

[Robin Murphy]

On 06/09/2024 1:55 pm, Will Deacon wrote:
> On Thu, Sep 05, 2024 at 06:55:33PM -0700, Nicolin Chen wrote:
>> On Thu, Sep 05, 2024 at 04:06:24PM +0000, Pranjal Shrivastava wrote:
>>
>>>>>>> Also, the printing would become
>>>>>>> more complicated as we'd have to log different fields for different
>>>>>>> events. Additionally, I don't see that many unions being defined
>>>>>>> elsewhere in the kernel.
>>>>>>
>>>>>> OK. That's a fair point. I think we could have just one common
>>>>>> union for the "good stuff" fields. Then, if something isn't in
>>>>>> the common union, do a FIELD_GET(raw)?
>>>>>>
>>>>>
>>>>> I'm not sure if I get this right, but are you suggesting something like:
>>>>>
>>>>> +struct arm_smmu_event {
>>>>> +       union {
>>>>> +       u64 raw_evt[4];
>>>>> +       struct {
>>>>> +               /* "Good stuff" fields */
>>>>> +       };
>>>>> +};
>>>>>
>>>>> and then based on the fault type we can use the "good stuff" or raw_evt?
>>>>> So, basically just add a union between raw_evt and the other fields to
>>>>> improve struct arm_smmu_event present in v2?
>>>>
>>>> Yea, I attached a test code at the EOM for your reference. Please
>>>> feel free to drop fields if they aren't common enough, and confirm
>>>> those bits are correctly written too.
>>>>
>>>
>>> Hmm, so you're suggesting to make the event a union instead of a struct
>>> thus, making reading event fields easy.
>>>
>>> The only thing I'm slightly worried about is if the bitfields don't
>>> remain constant if more events are added in the future.
>>>
>>> For example, the "IPA" field is in dword[3] for now, but if it changes
>>> in the future, it may get difficult to scale the union for future events
>>> If we can guarantee that it doesn't happen then I think union is better.
>>
>> I believe HW has its own reason to put those fields constantly
>> following the same pattern, and must have its tendency to keep
>> in that way.
>>
>> The "union { u64 raw; struct {} }" way isn't that uncommon in
>> the kernel, here are a few existing examples:
>> 	drivers/crypto/marvell/octeontx2/otx2_cpt_hw_types.h
>> 	drivers/crypto/cavium/nitrox/nitrox_req.h
>> 	include/asm-generic/hyperv-tlfs.h
>>
>> Taking a step back, if one field in a new event in the future
>> happens to be odd, we could still fetch field in its own way:
>> event->evt[4] is still available to extract via FIELD_GET; or
>> just define another union exclusively for that event, neither
>> of which sounds like the end of world.
>>
>> Having said that, I'd defer to Will. So, here is just my two
>> cents.
> 
> My only real concern is the fragility of using bitfields. I don't _think_
> the compiler is obliged to lay them out in the obvious way and I can't
> think of anything worse than being given a bad pretty-print when debugging
> a real driver issue!

Shall I mention big-endian? :D

FWIW I'm not a fan of the bitfield trickery either. However, unions 
aside, I do still think it would be a good idea for the raw event data 
storage to be in struct arm_smmu_event itself - having one local 
variable carry a pointer to another local variable in the same scope 
seems a bit silly.

Thanks,
Robin.

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

[Nicolin Chen]

On Fri, Sep 06, 2024 at 05:39:03PM +0100, Robin Murphy wrote:
> On 06/09/2024 1:55 pm, Will Deacon wrote:
> > > > > > +struct arm_smmu_event {
> > > > > > +       union {
> > > > > > +       u64 raw_evt[4];
> > > > > > +       struct {
> > > > > > +               /* "Good stuff" fields */
> > > > > > +       };
> > > > > > +};
> > > > > > 
> > > > > > and then based on the fault type we can use the "good stuff" or raw_evt?
> > > > > > So, basically just add a union between raw_evt and the other fields to
> > > > > > improve struct arm_smmu_event present in v2?
> > > > > 
> > > > > Yea, I attached a test code at the EOM for your reference. Please
> > > > > feel free to drop fields if they aren't common enough, and confirm
> > > > > those bits are correctly written too.
> > > > > 
> > > > 
> > > > Hmm, so you're suggesting to make the event a union instead of a struct
> > > > thus, making reading event fields easy.
> > > > 
> > > > The only thing I'm slightly worried about is if the bitfields don't
> > > > remain constant if more events are added in the future.
> > > > 
> > > > For example, the "IPA" field is in dword[3] for now, but if it changes
> > > > in the future, it may get difficult to scale the union for future events
> > > > If we can guarantee that it doesn't happen then I think union is better.
> > > 
> > > I believe HW has its own reason to put those fields constantly
> > > following the same pattern, and must have its tendency to keep
> > > in that way.
> > > 
> > > The "union { u64 raw; struct {} }" way isn't that uncommon in
> > > the kernel, here are a few existing examples:
> > >      drivers/crypto/marvell/octeontx2/otx2_cpt_hw_types.h
> > >      drivers/crypto/cavium/nitrox/nitrox_req.h
> > >      include/asm-generic/hyperv-tlfs.h
> > > 
> > > Taking a step back, if one field in a new event in the future
> > > happens to be odd, we could still fetch field in its own way:
> > > event->evt[4] is still available to extract via FIELD_GET; or
> > > just define another union exclusively for that event, neither
> > > of which sounds like the end of world.
> > > 
> > > Having said that, I'd defer to Will. So, here is just my two
> > > cents.
> > 
> > My only real concern is the fragility of using bitfields. I don't _think_
> > the compiler is obliged to lay them out in the obvious way and I can't
> > think of anything worse than being given a bad pretty-print when debugging
> > a real driver issue!

Well, if compiler is going to be an issue, I can't disagree with
your point.

Any reference to some existing issue with compiler failing to lay
out properly? I wonder how other headers could define their unions
using bitfields and stay safe..

> Shall I mention big-endian? :D

Ah, right. queue_read() converts le64 to native CPU format..

> FWIW I'm not a fan of the bitfield trickery either. However, unions

I am trying to learn here: apart from what Will mentioned above,
is there any other reason to stay away from bitfield?

Thanks!
Nicolin

> aside, I do still think it would be a good idea for the raw event data
> storage to be in struct arm_smmu_event itself - having one local
> variable carry a pointer to another local variable in the same scope
> seems a bit silly.
> 
> Thanks,
> Robin.

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

[Will Deacon]

On Fri, Sep 06, 2024 at 11:42:31AM -0700, Nicolin Chen wrote:
> On Fri, Sep 06, 2024 at 05:39:03PM +0100, Robin Murphy wrote:
> > On 06/09/2024 1:55 pm, Will Deacon wrote:
> > > > > > > +struct arm_smmu_event {
> > > > > > > +       union {
> > > > > > > +       u64 raw_evt[4];
> > > > > > > +       struct {
> > > > > > > +               /* "Good stuff" fields */
> > > > > > > +       };
> > > > > > > +};
> > > > > > > 
> > > > > > > and then based on the fault type we can use the "good stuff" or raw_evt?
> > > > > > > So, basically just add a union between raw_evt and the other fields to
> > > > > > > improve struct arm_smmu_event present in v2?
> > > > > > 
> > > > > > Yea, I attached a test code at the EOM for your reference. Please
> > > > > > feel free to drop fields if they aren't common enough, and confirm
> > > > > > those bits are correctly written too.
> > > > > > 
> > > > > 
> > > > > Hmm, so you're suggesting to make the event a union instead of a struct
> > > > > thus, making reading event fields easy.
> > > > > 
> > > > > The only thing I'm slightly worried about is if the bitfields don't
> > > > > remain constant if more events are added in the future.
> > > > > 
> > > > > For example, the "IPA" field is in dword[3] for now, but if it changes
> > > > > in the future, it may get difficult to scale the union for future events
> > > > > If we can guarantee that it doesn't happen then I think union is better.
> > > > 
> > > > I believe HW has its own reason to put those fields constantly
> > > > following the same pattern, and must have its tendency to keep
> > > > in that way.
> > > > 
> > > > The "union { u64 raw; struct {} }" way isn't that uncommon in
> > > > the kernel, here are a few existing examples:
> > > >      drivers/crypto/marvell/octeontx2/otx2_cpt_hw_types.h
> > > >      drivers/crypto/cavium/nitrox/nitrox_req.h
> > > >      include/asm-generic/hyperv-tlfs.h
> > > > 
> > > > Taking a step back, if one field in a new event in the future
> > > > happens to be odd, we could still fetch field in its own way:
> > > > event->evt[4] is still available to extract via FIELD_GET; or
> > > > just define another union exclusively for that event, neither
> > > > of which sounds like the end of world.
> > > > 
> > > > Having said that, I'd defer to Will. So, here is just my two
> > > > cents.
> > > 
> > > My only real concern is the fragility of using bitfields. I don't _think_
> > > the compiler is obliged to lay them out in the obvious way and I can't
> > > think of anything worse than being given a bad pretty-print when debugging
> > > a real driver issue!
> 
> Well, if compiler is going to be an issue, I can't disagree with
> your point.
> 
> Any reference to some existing issue with compiler failing to lay
> out properly? I wonder how other headers could define their unions
> using bitfields and stay safe..

It's not so much about buggy compilers, but more that I don't think the
C standard defines the order and so relying on it can be fragile. If you
fancy going to the effort of ensuring that LLVM and GCC will agree on
the "obvious" layout forever more, then don't let me stop you ;)

Will

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

[Pranjal Shrivastava]

On Mon, Sep 09, 2024 at 03:45:06PM +0100, Will Deacon wrote:
> On Fri, Sep 06, 2024 at 11:42:31AM -0700, Nicolin Chen wrote:
> > On Fri, Sep 06, 2024 at 05:39:03PM +0100, Robin Murphy wrote:
> > > On 06/09/2024 1:55 pm, Will Deacon wrote:
> > > > > > > > +struct arm_smmu_event {
> > > > > > > > +       union {
> > > > > > > > +       u64 raw_evt[4];
> > > > > > > > +       struct {
> > > > > > > > +               /* "Good stuff" fields */
> > > > > > > > +       };
> > > > > > > > +};
> > > > > > > > 
> > > > > > > > and then based on the fault type we can use the "good stuff" or raw_evt?
> > > > > > > > So, basically just add a union between raw_evt and the other fields to
> > > > > > > > improve struct arm_smmu_event present in v2?
> > > > > > > 
> > > > > > > Yea, I attached a test code at the EOM for your reference. Please
> > > > > > > feel free to drop fields if they aren't common enough, and confirm
> > > > > > > those bits are correctly written too.
> > > > > > > 
> > > > > > 
> > > > > > Hmm, so you're suggesting to make the event a union instead of a struct
> > > > > > thus, making reading event fields easy.
> > > > > > 
> > > > > > The only thing I'm slightly worried about is if the bitfields don't
> > > > > > remain constant if more events are added in the future.
> > > > > > 
> > > > > > For example, the "IPA" field is in dword[3] for now, but if it changes
> > > > > > in the future, it may get difficult to scale the union for future events
> > > > > > If we can guarantee that it doesn't happen then I think union is better.
> > > > > 
> > > > > I believe HW has its own reason to put those fields constantly
> > > > > following the same pattern, and must have its tendency to keep
> > > > > in that way.
> > > > > 
> > > > > The "union { u64 raw; struct {} }" way isn't that uncommon in
> > > > > the kernel, here are a few existing examples:
> > > > >      drivers/crypto/marvell/octeontx2/otx2_cpt_hw_types.h
> > > > >      drivers/crypto/cavium/nitrox/nitrox_req.h
> > > > >      include/asm-generic/hyperv-tlfs.h
> > > > > 
> > > > > Taking a step back, if one field in a new event in the future
> > > > > happens to be odd, we could still fetch field in its own way:
> > > > > event->evt[4] is still available to extract via FIELD_GET; or
> > > > > just define another union exclusively for that event, neither
> > > > > of which sounds like the end of world.
> > > > > 
> > > > > Having said that, I'd defer to Will. So, here is just my two
> > > > > cents.
> > > > 
> > > > My only real concern is the fragility of using bitfields. I don't _think_
> > > > the compiler is obliged to lay them out in the obvious way and I can't
> > > > think of anything worse than being given a bad pretty-print when debugging
> > > > a real driver issue!
> > 
> > Well, if compiler is going to be an issue, I can't disagree with
> > your point.
> > 
> > Any reference to some existing issue with compiler failing to lay
> > out properly? I wonder how other headers could define their unions
> > using bitfields and stay safe..
> 
> It's not so much about buggy compilers, but more that I don't think the
> C standard defines the order and so relying on it can be fragile. If you
> fancy going to the effort of ensuring that LLVM and GCC will agree on
> the "obvious" layout forever more, then don't let me stop you ;)
> 

Hmmm.. +1
I had some time, so.. I dived into the C specification[1] (free draft)
to look for bitfield layouts, under section 6.7.2.1, point 11 
(page 101 of the pdf), and I see the following paragraph:

"An implementation may allocate any addressable storage unit large
enough to hold a bit-field. If enough space remains, a bit-field that
immediately follows another bit-field in a structure shall be packed
into adjacent bits of the same unit. If insufficient space remains,
whether a bit-field that does not fit is put into the next unit or
overlaps adjacent units is implementation-defined. The order of 
allocation of bit-fields within a unit (high-order to low-order or 
low-order to high-order) is implementation-defined. The alignment
of the addressable storage unit is unspecified."

However, I'm not sure how these "implementations" comprehend the spec.

> Will

> > I am trying to learn here: apart from what Will mentioned above,
> > is there any other reason to stay away from bitfield?

One more thing I can think of is Alignment faults on certain archs.
Accessing individual bitfields within the struct could lead to unaligned
memory accesses. I have run into alignment faults on $ARCH=arm64 couple
of times while using packed structs in the kernel code.

[1]
 https://web.archive.org/web/20181230041359if_/http://www.open-std.org/jtc1/sc22/wg14/www/abq/c17_updated_proposed_fdis.pdf

Thanks,
Pranjal

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

[Nicolin Chen]

On Mon, Sep 09, 2024 at 05:30:22PM +0000, Pranjal Shrivastava wrote:

> > > > > My only real concern is the fragility of using bitfields. I don't _think_
> > > > > the compiler is obliged to lay them out in the obvious way and I can't
> > > > > think of anything worse than being given a bad pretty-print when debugging
> > > > > a real driver issue!
> > >
> > > Well, if compiler is going to be an issue, I can't disagree with
> > > your point.
> > >
> > > Any reference to some existing issue with compiler failing to lay
> > > out properly? I wonder how other headers could define their unions
> > > using bitfields and stay safe..
> >
> > It's not so much about buggy compilers, but more that I don't think the
> > C standard defines the order and so relying on it can be fragile. If you
> > fancy going to the effort of ensuring that LLVM and GCC will agree on
> > the "obvious" layout forever more, then don't let me stop you ;)

I see!
 
> Hmmm.. +1
> I had some time, so.. I dived into the C specification[1] (free draft)
> to look for bitfield layouts, under section 6.7.2.1, point 11
> (page 101 of the pdf), and I see the following paragraph:
> 
> "An implementation may allocate any addressable storage unit large
> enough to hold a bit-field. If enough space remains, a bit-field that
> immediately follows another bit-field in a structure shall be packed
> into adjacent bits of the same unit. If insufficient space remains,
> whether a bit-field that does not fit is put into the next unit or
> overlaps adjacent units is implementation-defined. The order of
> allocation of bit-fields within a unit (high-order to low-order or
> low-order to high-order) is implementation-defined. The alignment
> of the addressable storage unit is unspecified."

Thanks for digging into the standard doc!

> However, I'm not sure how these "implementations" comprehend the spec.
> 
> > Will
> 
> > > I am trying to learn here: apart from what Will mentioned above,
> > > is there any other reason to stay away from bitfield?
> 
> One more thing I can think of is Alignment faults on certain archs.
> Accessing individual bitfields within the struct could lead to unaligned
> memory accesses. I have run into alignment faults on $ARCH=arm64 couple
> of times while using packed structs in the kernel code.

OK. It seems that we should stay safe with verbose FIELD_GETs then.

Thank you
Nicolin

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

[Daniel Mentz]

On Thu, Aug 29, 2024 at 4:54 PM Pranjal Shrivastava <praan@google.com> wrote:
>
> On Wed, Aug 28, 2024 at 11:36:02PM -0700, Nicolin Chen wrote:
> > > +static void arm_smmu_dump_xlation_fault(struct arm_smmu_device *smmu,
> > > +                                       struct arm_smmu_event *event)
> > > +{
> > > +       dev_err(smmu->dev, "Fault: %s client %s sid 0x%08x.0x%05x:\n",
> >
> > "client" doesn't seem to be used anywhere in this driver, I would:
> > s/client/master
> >
>
> Ack, would change this to "master".

To align with Documentation/process/coding-style.rst , I propose using
the term "client" as it can be considered "new usage". In this driver,
there are no existing log messages that use the word "master". The Arm
SMMUv3 arch spec consistently uses the term "client device" and
completely avoids the term "master". (The Arm MMU-700 TRM does use the
master/slave terminology)