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

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

[Pranjal Shrivastava]

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

Some sample events, powered by QEMU:

1. Bad StreamID:

[    7.521940] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x02 received:
[    7.522485] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000002
[    7.523335] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[    7.523814] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[    7.524281] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[    7.524759] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x2 received: C_BAD_STREAMID
[    7.524759]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0

2. STE Fetch Fault:

[    7.531332] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x03 received:
[    7.531890] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000003
[    7.532371] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[    7.533189] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[    7.533674] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000000001e0
[    7.534136] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x3 received: F_STE_FETCH
[    7.534136]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
[    7.534136]  Fetch Address: 0x1e0

3. Permission Fault:

[    7.547455] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x13 received:
[    7.547997] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000013
[    7.548476] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
[    7.548949] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
[    7.549418] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[    7.550168] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x13 received: F_PERMISSION
[    7.550168]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
[    7.550168]  iova 0xfffff040 ipa 0x0
[    7.550168]  Unpriv | Data | Write | S1 | Input address caused fault
[    7.550168]  STAG: 0x0

4. Translation Fault:

[    7.586428] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x10 received:
[    7.587012] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000010
[    7.587504] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
[    7.587986] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
[    7.588745] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
[    7.589219]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
[    7.589219]  iova 0xfffff040 ipa 0x0
[    7.589219]  Unpriv | Data | Write | S1 | Input address caused fault
[    7.589219]  STAG: 0x0

5. Unknown / Implementation-defined Fault:

[    7.152599] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x0b received:
[    7.153064] arm-smmu-v3 arm-smmu-v3.0.auto:  0x000000080000000b
[    7.153755] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000010000000000
[    7.154239] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
[    7.154702] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
[    7.155169] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0xb received: UNKNOWN
[    7.155169]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0

v5
 * Replaced strings with used macros to conserve stack space
 * Fixed IPA log truncation,(e.g. logged 0xfffff instead of 0xfffff040)
 * Bounded evt->id by ARRAY_SIZE to avoid inflating event_str
 * Restored the original raw dump format
 * Decoupled "raw" event from `struct arm_smmu_event`
 * Moved the raw event log before the decoded/pretty log
 * Improved the sid.ssid format to be more readable
 * Zero-initialized the `arm_smmu_event` var in `arm_smmu_handle_evt`
 * Refactored struct arm_smmu_events to use bitfields instead of bools
 * Removed `struct arm_smmu_device` field from `struct arm_smmu_event`
 * Removed `master_name` field to use `dev_name(event->dev)` instead
 * Pahole reports a total of 3-byte holes & struct size = 40 bytes
 * Moved most constant parts of the log to the fmt string
 * Moved the event decoding within `arm_smmu_handle_evt`
 * Renamed `arm_smmu_get_event_from_raw` => `arm_smmu_decode_event`
 * Renamed a few EVT_IDs for consistency as per Daniel's suggestions
 * Renamed "master" to "client" in the logs as suggested in reviews
 * Corrected "F_VMS_FAULT" => "F_VMS_FETCH"
 * Re-ordered event strings in event_str as they appear in the spec
 * Removed less useful comments

v4
 https://lore.kernel.org/all/20241018180022.807928-1-praan@google.com/
 * Re-arranged the series to first introduce struct arm_smmu_event
 * Improved the complex ternary expression that prints TTRnW info
 * Added consistent spacing to the logs & resized log strings
 * Moved ratelimiting within `arm_smmu_dump_event`
 * Refactored master_name printing by getting a ref to the device
 * Refactored `arm_smmu_handle_evt` to avoid redundant master lookup

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

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

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


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

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 173 ++++++++++++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  28 ++++
 2 files changed, 165 insertions(+), 36 deletions(-)

-- 
2.47.0.277.g8800431eea-goog

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

[Pranjal Shrivastava]

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

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

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 737c5b882355..e1b69aa04382 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1757,17 +1757,33 @@ 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 void arm_smmu_decode_event(u64 *raw, struct arm_smmu_event *event)
+{
+	event->id = FIELD_GET(EVTQ_0_ID, raw[0]);
+	event->sid = FIELD_GET(EVTQ_0_SID, raw[0]);
+	event->ssv = raw[0] & EVTQ_0_SSV;
+	event->ssid = event->ssv ? FIELD_GET(EVTQ_0_SSID, raw[0]) : IOMMU_NO_PASID;
+	event->privileged = FIELD_GET(EVTQ_1_PnU, raw[1]);
+	event->instruction = FIELD_GET(EVTQ_1_InD, raw[1]);
+	event->s2 = FIELD_GET(EVTQ_1_S2, raw[1]);
+	event->read = FIELD_GET(EVTQ_1_RnW, raw[1]);
+	event->stag = FIELD_GET(EVTQ_1_STAG, raw[1]);
+	event->stall = raw[1] & EVTQ_1_STALL;
+	event->class = FIELD_GET(EVTQ_1_CLASS, raw[1]);
+	event->iova = FIELD_GET(EVTQ_2_ADDR, raw[2]);
+	event->ipa = raw[3];
+}
+
+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:
@@ -1777,35 +1793,35 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
 		return -EOPNOTSUPP;
 	}
 
-	if (!(evt[1] & EVTQ_1_STALL))
+	if (!event->stall)
 		return -EOPNOTSUPP;
 
-	if (evt[1] & EVTQ_1_RnW)
+	if (event->read)
 		perm |= IOMMU_FAULT_PERM_READ;
 	else
 		perm |= IOMMU_FAULT_PERM_WRITE;
 
-	if (evt[1] & EVTQ_1_InD)
+	if (event->instruction)
 		perm |= IOMMU_FAULT_PERM_EXEC;
 
-	if (evt[1] & EVTQ_1_PnU)
+	if (event->privileged)
 		perm |= IOMMU_FAULT_PERM_PRIV;
 
 	flt->type = IOMMU_FAULT_PAGE_REQ;
 	flt->prm = (struct iommu_fault_page_request) {
 		.flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
-		.grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
+		.grpid = event->stag,
 		.perm = perm,
-		.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
+		.addr = event->iova,
 	};
 
-	if (ssid_valid) {
+	if (event->ssv) {
 		flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
-		flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
+		flt->prm.pasid = event->ssid;
 	}
 
 	mutex_lock(&smmu->streams_mutex);
-	master = arm_smmu_find_master(smmu, sid);
+	master = arm_smmu_find_master(smmu, event->sid);
 	if (!master) {
 		ret = -EINVAL;
 		goto out_unlock;
@@ -1820,25 +1836,26 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
 	int i, ret;
+	u64 raw_evt[EVTQ_ENT_DWORDS];
+	struct arm_smmu_event evt = {0};
 	struct arm_smmu_device *smmu = dev;
 	struct arm_smmu_queue *q = &smmu->evtq.q;
 	struct arm_smmu_ll_queue *llq = &q->llq;
 	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
 				      DEFAULT_RATELIMIT_BURST);
-	u64 evt[EVTQ_ENT_DWORDS];
 
 	do {
-		while (!queue_remove_raw(q, evt)) {
-			u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
+		while (!queue_remove_raw(q, raw_evt)) {
 
-			ret = arm_smmu_handle_evt(smmu, evt);
+			arm_smmu_decode_event(raw_evt, &evt);
+			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, "event 0x%02x received:\n", evt.id);
+			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
 				dev_info(smmu->dev, "\t0x%016llx\n",
-					 (unsigned long long)evt[i]);
+					(unsigned long long)raw_evt[i]);
 
 			cond_resched();
 		}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 1e9952ca989f..abb543d987f6 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -771,6 +771,22 @@ struct arm_smmu_stream {
 	struct rb_node			node;
 };
 
+struct arm_smmu_event {
+	u8				stall		: 1,
+					ssv		: 1,
+					privileged	: 1,
+					instruction	: 1,
+					s2		: 1,
+					read		: 1;
+	u8				id;
+	u8				class;
+	u16				stag;
+	u32				sid;
+	u32				ssid;
+	u64				iova;
+	u64				ipa;
+};
+
 /* SMMU private data for each master */
 struct arm_smmu_master {
 	struct arm_smmu_device		*smmu;
-- 
2.47.0.277.g8800431eea-goog

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

[Nicolin Chen]

On Tue, Nov 12, 2024 at 08:30:16AM +0000, Pranjal Shrivastava wrote:
> +static void arm_smmu_decode_event(u64 *raw, struct arm_smmu_event *event)
..
> +static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
> +                              struct arm_smmu_event *event)

s/arm_smmu_handle_evt/arm_smmu_handle_event

Also, given that these two define "struct arm_smmu_event *event"
nicely, ...

> @@ -1820,25 +1836,26 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
>  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>  {
>         int i, ret;
> +       u64 raw_evt[EVTQ_ENT_DWORDS];
> +       struct arm_smmu_event evt = {0};

I think we can keep the existing "evt" for raw and simply add:
	struct arm_smmu_event event = { };

So the patch could look cleaner, less the "s/evt/raw_evt" part.

>         struct arm_smmu_device *smmu = dev;
>         struct arm_smmu_queue *q = &smmu->evtq.q;
>         struct arm_smmu_ll_queue *llq = &q->llq;
>         static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
>                                       DEFAULT_RATELIMIT_BURST);
> -       u64 evt[EVTQ_ENT_DWORDS];
> 
>         do {
> -               while (!queue_remove_raw(q, evt)) {
> -                       u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> +               while (!queue_remove_raw(q, raw_evt)) {
> 
> -                       ret = arm_smmu_handle_evt(smmu, evt);
> +                       arm_smmu_decode_event(raw_evt, &evt);

Drop the empty line in-between please.

> +                       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, "event 0x%02x received:\n", evt.id);
> +                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
>                                 dev_info(smmu->dev, "\t0x%016llx\n",
> -                                        (unsigned long long)evt[i]);
> +                                       (unsigned long long)raw_evt[i]);

Please keep the previous indentations.

> 
>                         cond_resched();
>                 }
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 1e9952ca989f..abb543d987f6 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -771,6 +771,22 @@ struct arm_smmu_stream {
>         struct rb_node                  node;
>  };
> 
> +struct arm_smmu_event {
> +       u8                              stall           : 1,
> +                                       ssv             : 1,
> +                                       privileged      : 1,
> +                                       instruction     : 1,
> +                                       s2              : 1,
> +                                       read            : 1;

Feels odd to do so...should all of them be:
	u8			name : 1;
?

Thanks
Nicolin

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

[Pranjal Shrivastava]

On Tue, Nov 12, 2024 at 03:30:58PM -0800, Nicolin Chen wrote:
> On Tue, Nov 12, 2024 at 08:30:16AM +0000, Pranjal Shrivastava wrote:
> > +static void arm_smmu_decode_event(u64 *raw, struct arm_smmu_event *event)
> ..
> > +static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
> > +                              struct arm_smmu_event *event)
> 
> s/arm_smmu_handle_evt/arm_smmu_handle_event
> 
> Also, given that these two define "struct arm_smmu_event *event"
> nicely, ...
> 

Ack.

> > @@ -1820,25 +1836,26 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >  {
> >         int i, ret;
> > +       u64 raw_evt[EVTQ_ENT_DWORDS];
> > +       struct arm_smmu_event evt = {0};
> 
> I think we can keep the existing "evt" for raw and simply add:
> 	struct arm_smmu_event event = { };
> 
> So the patch could look cleaner, less the "s/evt/raw_evt" part.
> 

Ack. Makes sense. Will rename accordingly.

> >         struct arm_smmu_device *smmu = dev;
> >         struct arm_smmu_queue *q = &smmu->evtq.q;
> >         struct arm_smmu_ll_queue *llq = &q->llq;
> >         static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> >                                       DEFAULT_RATELIMIT_BURST);
> > -       u64 evt[EVTQ_ENT_DWORDS];
> > 
> >         do {
> > -               while (!queue_remove_raw(q, evt)) {
> > -                       u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> > +               while (!queue_remove_raw(q, raw_evt)) {
> > 
> > -                       ret = arm_smmu_handle_evt(smmu, evt);
> > +                       arm_smmu_decode_event(raw_evt, &evt);
> 
> Drop the empty line in-between please.

Ack.

> 
> > +                       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, "event 0x%02x received:\n", evt.id);
> > +                       for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> >                                 dev_info(smmu->dev, "\t0x%016llx\n",
> > -                                        (unsigned long long)evt[i]);
> > +                                       (unsigned long long)raw_evt[i]);
> 
> Please keep the previous indentations.

Ack

>
> > 
> >                         cond_resched();
> >                 }
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > index 1e9952ca989f..abb543d987f6 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -771,6 +771,22 @@ struct arm_smmu_stream {
> >         struct rb_node                  node;
> >  };
> > 
> > +struct arm_smmu_event {
> > +       u8                              stall           : 1,
> > +                                       ssv             : 1,
> > +                                       privileged      : 1,
> > +                                       instruction     : 1,
> > +                                       s2              : 1,
> > +                                       read            : 1;
> 
> Feels odd to do so...should all of them be:
> 	u8			name : 1;
> ?

Hmmm, do you mean something like the following?

struct arm_smmu_event {
	u8				stall           : 1,
	u8				ssv             : 1,
	u8				privileged      : 1,
	u8				instruction     : 1,
	u8				s2              : 1,
	u8				read            : 1;
}

> 
> Thanks
> Nicolin

Thanks,
Praan

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

[Nicolin Chen]

On Fri, Nov 15, 2024 at 01:13:36PM +0000, Pranjal Shrivastava wrote:
> On Tue, Nov 12, 2024 at 03:30:58PM -0800, Nicolin Chen wrote:
> > On Tue, Nov 12, 2024 at 08:30:16AM +0000, Pranjal Shrivastava wrote:
> > > +struct arm_smmu_event {
> > > +       u8                              stall           : 1,
> > > +                                       ssv             : 1,
> > > +                                       privileged      : 1,
> > > +                                       instruction     : 1,
> > > +                                       s2              : 1,
> > > +                                       read            : 1;
> > 
> > Feels odd to do so...should all of them be:
> > 	u8			name : 1;
> > ?
> 
> Hmmm, do you mean something like the following?

Never mind. I see amd_iommu_types.h does something just like that.

Nicolin

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

[Jason Gunthorpe]

On Fri, Nov 15, 2024 at 01:34:12PM -0800, Nicolin Chen wrote:
> On Fri, Nov 15, 2024 at 01:13:36PM +0000, Pranjal Shrivastava wrote:
> > On Tue, Nov 12, 2024 at 03:30:58PM -0800, Nicolin Chen wrote:
> > > On Tue, Nov 12, 2024 at 08:30:16AM +0000, Pranjal Shrivastava wrote:
> > > > +struct arm_smmu_event {
> > > > +       u8                              stall           : 1,
> > > > +                                       ssv             : 1,
> > > > +                                       privileged      : 1,
> > > > +                                       instruction     : 1,
> > > > +                                       s2              : 1,
> > > > +                                       read            : 1;
> > > 
> > > Feels odd to do so...should all of them be:
> > > 	u8			name : 1;
> > > ?
> > 
> > Hmmm, do you mean something like the following?
> 
> Never mind. I see amd_iommu_types.h does something just like that.

I certainly expect to see the u8 repeated and not use , for members of
a structure. Also don't add excessive horizonal alignment.. C code is
not excel. :)

Jason

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

[Will Deacon]

On Mon, Nov 18, 2024 at 12:24:42PM -0400, Jason Gunthorpe wrote:
> On Fri, Nov 15, 2024 at 01:34:12PM -0800, Nicolin Chen wrote:
> > On Fri, Nov 15, 2024 at 01:13:36PM +0000, Pranjal Shrivastava wrote:
> > > On Tue, Nov 12, 2024 at 03:30:58PM -0800, Nicolin Chen wrote:
> > > > On Tue, Nov 12, 2024 at 08:30:16AM +0000, Pranjal Shrivastava wrote:
> > > > > +struct arm_smmu_event {
> > > > > +       u8                              stall           : 1,
> > > > > +                                       ssv             : 1,
> > > > > +                                       privileged      : 1,
> > > > > +                                       instruction     : 1,
> > > > > +                                       s2              : 1,
> > > > > +                                       read            : 1;
> > > > 
> > > > Feels odd to do so...should all of them be:
> > > > 	u8			name : 1;
> > > > ?
> > > 
> > > Hmmm, do you mean something like the following?
> > 
> > Never mind. I see amd_iommu_types.h does something just like that.
> 
> I certainly expect to see the u8 repeated and not use , for members of
> a structure. Also don't add excessive horizonal alignment.. C code is
> not excel. :)

I think the comma syntax is fine (there's precedence for it and it works),
but agreed on removing some of the tabs.

Will

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

[Pranjal Shrivastava]

On Tue, Nov 19, 2024 at 09:03:08AM +0000, Will Deacon wrote:
> On Mon, Nov 18, 2024 at 12:24:42PM -0400, Jason Gunthorpe wrote:
> > On Fri, Nov 15, 2024 at 01:34:12PM -0800, Nicolin Chen wrote:
> > > On Fri, Nov 15, 2024 at 01:13:36PM +0000, Pranjal Shrivastava wrote:
> > > > On Tue, Nov 12, 2024 at 03:30:58PM -0800, Nicolin Chen wrote:
> > > > > On Tue, Nov 12, 2024 at 08:30:16AM +0000, Pranjal Shrivastava wrote:
> > > > > > +struct arm_smmu_event {
> > > > > > +       u8                              stall           : 1,
> > > > > > +                                       ssv             : 1,
> > > > > > +                                       privileged      : 1,
> > > > > > +                                       instruction     : 1,
> > > > > > +                                       s2              : 1,
> > > > > > +                                       read            : 1;
> > > > > 
> > > > > Feels odd to do so...should all of them be:
> > > > > 	u8			name : 1;
> > > > > ?
> > > > 
> > > > Hmmm, do you mean something like the following?
> > > 
> > > Never mind. I see amd_iommu_types.h does something just like that.
> > 
> > I certainly expect to see the u8 repeated and not use , for members of
> > a structure. Also don't add excessive horizonal alignment.. C code is
> > not excel. :)
> 
> I think the comma syntax is fine (there's precedence for it and it works),
> but agreed on removing some of the tabs.

Ohh alright. I'm assuming both of you are referring to the tabs before
the colon ':' ? 

As in, is the following fine?

struct arm_smmu_event {
       u8		stall : 1,
			ssv : 1,
			privileged : 1,
			instruction : 1,
			s2 : 1,
			read : 1;
	.....
};


> 
> Will

Thanks,
Praan

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

[Will Deacon]

On Tue, Nov 19, 2024 at 09:27:47AM +0000, Pranjal Shrivastava wrote:
> On Tue, Nov 19, 2024 at 09:03:08AM +0000, Will Deacon wrote:
> > On Mon, Nov 18, 2024 at 12:24:42PM -0400, Jason Gunthorpe wrote:
> > > On Fri, Nov 15, 2024 at 01:34:12PM -0800, Nicolin Chen wrote:
> > > > On Fri, Nov 15, 2024 at 01:13:36PM +0000, Pranjal Shrivastava wrote:
> > > > > On Tue, Nov 12, 2024 at 03:30:58PM -0800, Nicolin Chen wrote:
> > > > > > On Tue, Nov 12, 2024 at 08:30:16AM +0000, Pranjal Shrivastava wrote:
> > > > > > > +struct arm_smmu_event {
> > > > > > > +       u8                              stall           : 1,
> > > > > > > +                                       ssv             : 1,
> > > > > > > +                                       privileged      : 1,
> > > > > > > +                                       instruction     : 1,
> > > > > > > +                                       s2              : 1,
> > > > > > > +                                       read            : 1;
> > > > > > 
> > > > > > Feels odd to do so...should all of them be:
> > > > > > 	u8			name : 1;
> > > > > > ?
> > > > > 
> > > > > Hmmm, do you mean something like the following?
> > > > 
> > > > Never mind. I see amd_iommu_types.h does something just like that.
> > > 
> > > I certainly expect to see the u8 repeated and not use , for members of
> > > a structure. Also don't add excessive horizonal alignment.. C code is
> > > not excel. :)
> > 
> > I think the comma syntax is fine (there's precedence for it and it works),
> > but agreed on removing some of the tabs.
> 
> Ohh alright. I'm assuming both of you are referring to the tabs before
> the colon ':' ? 
> 
> As in, is the following fine?
> 
> struct arm_smmu_event {
>        u8		stall : 1,
> 			ssv : 1,
> 			privileged : 1,
> 			instruction : 1,
> 			s2 : 1,
> 			read : 1;
> 	.....

I was actually thinking of the whitespace between the 'u8' and the field
names. But this is silly nit-picking. Please just follow something like
the use of bitfields in 'struct perf_event_attr', as that's a readable
example of recent-ish in-tree code.

Ta,

Will

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

[Pranjal Shrivastava]

On Wed, Nov 20, 2024 at 09:56:08AM +0000, Will Deacon wrote:
> On Tue, Nov 19, 2024 at 09:27:47AM +0000, Pranjal Shrivastava wrote:
> > On Tue, Nov 19, 2024 at 09:03:08AM +0000, Will Deacon wrote:
> > > On Mon, Nov 18, 2024 at 12:24:42PM -0400, Jason Gunthorpe wrote:
> > > > On Fri, Nov 15, 2024 at 01:34:12PM -0800, Nicolin Chen wrote:
> > > > > On Fri, Nov 15, 2024 at 01:13:36PM +0000, Pranjal Shrivastava wrote:
> > > > > > On Tue, Nov 12, 2024 at 03:30:58PM -0800, Nicolin Chen wrote:
> > > > > > > On Tue, Nov 12, 2024 at 08:30:16AM +0000, Pranjal Shrivastava wrote:
> > > > > > > > +struct arm_smmu_event {
> > > > > > > > +       u8                              stall           : 1,
> > > > > > > > +                                       ssv             : 1,
> > > > > > > > +                                       privileged      : 1,
> > > > > > > > +                                       instruction     : 1,
> > > > > > > > +                                       s2              : 1,
> > > > > > > > +                                       read            : 1;
> > > > > > > 
> > > > > > > Feels odd to do so...should all of them be:
> > > > > > > 	u8			name : 1;
> > > > > > > ?
> > > > > > 
> > > > > > Hmmm, do you mean something like the following?
> > > > > 
> > > > > Never mind. I see amd_iommu_types.h does something just like that.
> > > > 
> > > > I certainly expect to see the u8 repeated and not use , for members of
> > > > a structure. Also don't add excessive horizonal alignment.. C code is
> > > > not excel. :)
> > > 
> > > I think the comma syntax is fine (there's precedence for it and it works),
> > > but agreed on removing some of the tabs.
> > 
> > Ohh alright. I'm assuming both of you are referring to the tabs before
> > the colon ':' ? 
> > 
> > As in, is the following fine?
> > 
> > struct arm_smmu_event {
> >        u8		stall : 1,
> > 			ssv : 1,
> > 			privileged : 1,
> > 			instruction : 1,
> > 			s2 : 1,
> > 			read : 1;
> > 	.....
> 
> I was actually thinking of the whitespace between the 'u8' and the field
> names. But this is silly nit-picking. Please just follow something like
> the use of bitfields in 'struct perf_event_attr', as that's a readable
> example of recent-ish in-tree code.
> 
> Ta,

Ack. Thanks for the reference, I'll fix it!

> 
> Will

Thanks,
Praan

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

[Daniel Mentz]

On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
> +       event->stag = FIELD_GET(EVTQ_1_STAG, raw[1]);
> +       event->stall = raw[1] & EVTQ_1_STALL;

Any particular reason you're not using FIELD_GET on EVTQ_1_STALL like
in FIELD_GET(EVTQ_1_STALL, raw[1]); ?

> +       event->class = FIELD_GET(EVTQ_1_CLASS, raw[1]);
> +       event->iova = FIELD_GET(EVTQ_2_ADDR, raw[2]);
> +       event->ipa = raw[3];

Shouldn't this be

event->ipa =FIELD_GET(EVTQ_3_IPA, raw[3]);

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

[Pranjal Shrivastava]

On Tue, Nov 26, 2024 at 07:25:12PM -0800, Daniel Mentz wrote:
> On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
> > +       event->stag = FIELD_GET(EVTQ_1_STAG, raw[1]);
> > +       event->stall = raw[1] & EVTQ_1_STALL;
> 
> Any particular reason you're not using FIELD_GET on EVTQ_1_STALL like
> in FIELD_GET(EVTQ_1_STALL, raw[1]); ?
> 

No particular reason as such, just wanted to ensure a bool value as the
current usage in `arm_smmu_handle_evt` fetched the stall field similarly

I'll update this to use a FIELD_GET 

> > +       event->class = FIELD_GET(EVTQ_1_CLASS, raw[1]);
> > +       event->iova = FIELD_GET(EVTQ_2_ADDR, raw[2]);
> > +       event->ipa = raw[3];
> 
> Shouldn't this be
> 
> event->ipa =FIELD_GET(EVTQ_3_IPA, raw[3]);

No. For this, there's a reason for not using the 
`FIELD_GET(EVTQ_3_IPA, raw[3]);`. As mentioned in the cover letter, this
was the root cause of the IPA truncation bug, i.e. FIELD_GET only
fetches the IPA field without trailing zeroes, truncating the address.
For e.g. IPA = 0xfffff instead of 0xfffff000 which is faulting address.

Also, since the definition of the field is GENMASK_ULL(51,12) it doesn't
work for events like `F_STE_FETCH` where the Fetch address field is:
GENMASK_ULL(51,3).

Since the SMMUv3 spec ensures that the bitfields NOT representing an
address, i.e. fields other than GENMASK_ULL(51,12) or GENMASK_ULL(51,3),
are zeros. It's safe to read the raw[3] dword directly.

I think this wasn't discovered until now as we are NOT using the
EVTQ_3_IPA mask anywhere in the `arm-smmu-v3*` source files.


Thanks,
Praan

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

[Daniel Mentz]

On Wed, Nov 27, 2024 at 1:25 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > +       event->class = FIELD_GET(EVTQ_1_CLASS, raw[1]);
> > > +       event->iova = FIELD_GET(EVTQ_2_ADDR, raw[2]);
> > > +       event->ipa = raw[3];
> >
> > Shouldn't this be
> >
> > event->ipa =FIELD_GET(EVTQ_3_IPA, raw[3]);
>
> No. For this, there's a reason for not using the
> `FIELD_GET(EVTQ_3_IPA, raw[3]);`. As mentioned in the cover letter, this
> was the root cause of the IPA truncation bug, i.e. FIELD_GET only
> fetches the IPA field without trailing zeroes, truncating the address.
> For e.g. IPA = 0xfffff instead of 0xfffff000 which is faulting address.

I disagree with the term truncation bug, because the information was
never there in the first place. Truncating the IPA was presumably a
conscious decision made by the architect. It is not a bug.

>
> Also, since the definition of the field is GENMASK_ULL(51,12) it doesn't
> work for events like `F_STE_FETCH` where the Fetch address field is:
> GENMASK_ULL(51,3).

Using the ipa member for FetchAddr appears to be a bit hacky. Can you
use a union in the struct that combines ipa and fetchaddr (or
fetch_addr), and then decode either field depending on the event id? I
understand that no event type has both FetchAddr and IPA fields.

>
> Since the SMMUv3 spec ensures that the bitfields NOT representing an
> address, i.e. fields other than GENMASK_ULL(51,12) or GENMASK_ULL(51,3),
> are zeros. It's safe to read the raw[3] dword directly.

Somewhere in the Arm ARM, I found the following language:

"The RES0 description can apply to bits or fields that are read-only,
or are write-only: For a read-only bit, RES0 indicates that the bit
reads as 0, but software must treat the bit as UNKNOWN."
"A bit that is RES0 in a context is reserved for possible future use
in that context. To preserve forward compatibility, software: Must not
rely on the bit reading as 0."

As it stands, I believe the code relies on RES0 fields reading 0 which
would violate this rule.

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

[Pranjal Shrivastava]

On Wed, Nov 27, 2024 at 11:42:11AM -0800, Daniel Mentz wrote:
> On Wed, Nov 27, 2024 at 1:25 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > > +       event->class = FIELD_GET(EVTQ_1_CLASS, raw[1]);
> > > > +       event->iova = FIELD_GET(EVTQ_2_ADDR, raw[2]);
> > > > +       event->ipa = raw[3];
> > >
> > > Shouldn't this be
> > >
> > > event->ipa =FIELD_GET(EVTQ_3_IPA, raw[3]);
> >
> > No. For this, there's a reason for not using the
> > `FIELD_GET(EVTQ_3_IPA, raw[3]);`. As mentioned in the cover letter, this
> > was the root cause of the IPA truncation bug, i.e. FIELD_GET only
> > fetches the IPA field without trailing zeroes, truncating the address.
> > For e.g. IPA = 0xfffff instead of 0xfffff000 which is faulting address.
> 
> I disagree with the term truncation bug, because the information was
> never there in the first place. Truncating the IPA was presumably a
> conscious decision made by the architect. It is not a bug.
> 

I'm NOT claiming that the HW or architecture has a bug.
I meant that SW can't ONLY look at (and log) bits 51:12 only which the
`FIELD_GET(EVTQ_3_IPA, raw[3]);` does (the values I've shared in the
cover letter are actual values printed on QEMU). If we'd wanna use the
`FIELD_GET(EVTQ_3_IPA, raw[3]);` then we'd also have to shift it by 12.

The architects provided bits 51:12 as IPA since the min Translation
Granule supported by SMMUv3 is 4KB, hence the last 12-bits will be 0s.
Thus, an "IPA" or "PA" (in S1 only) *has* to have the last 12-bits as 0s
hence, I'd believe IPA = 0xFFFFF would not make sense as min TG is 4KB
and 0xFFFFF is not 4K aligned.

> >
> > Also, since the definition of the field is GENMASK_ULL(51,12) it doesn't
> > work for events like `F_STE_FETCH` where the Fetch address field is:
> > GENMASK_ULL(51,3).
> 
> Using the ipa member for FetchAddr appears to be a bit hacky. Can you
> use a union in the struct that combines ipa and fetchaddr (or
> fetch_addr), and then decode either field depending on the event id? I
> understand that no event type has both FetchAddr and IPA fields.
> 

Ack. We can use unions, but I'd still say that we can simply read raw[3]
for all addresses as the spec marks them as 0 (not RES0).

> >
> > Since the SMMUv3 spec ensures that the bitfields NOT representing an
> > address, i.e. fields other than GENMASK_ULL(51,12) or GENMASK_ULL(51,3),
> > are zeros. It's safe to read the raw[3] dword directly.
> 
> Somewhere in the Arm ARM, I found the following language:
> 
> "The RES0 description can apply to bits or fields that are read-only,
> or are write-only: For a read-only bit, RES0 indicates that the bit
> reads as 0, but software must treat the bit as UNKNOWN."
> "A bit that is RES0 in a context is reserved for possible future use
> in that context. To preserve forward compatibility, software: Must not
> rely on the bit reading as 0."
> 
> As it stands, I believe the code relies on RES0 fields reading 0 which
> would violate this rule.

The fields I'm talking about aren't marked as `RES0` in the spec. For
example, if we look at Section 7.3.4 in the spec, F_STE_FETCH
*explicitly* marks non-address fields as 0 not RES0. The same can be
noticed in sections 7.3.10, 7.3.12 - 7.3.17 & 7.3.20. Unless I missed
something that mentions that the "0" fields in event records are RES0?

Additionally, I don't think we can generalize this a lot because the
SMMUv3 spec also says stuff like: "In SMMUv3.0, FetchAddr bits [51:48]
are RES0." Now in this case, we'll have to read the IDR, figure out the
SMMU version and then accordingly mask out RES0 bits because they can be
UNKNOWN, which in my opinion would be a bit too much.

Thanks,
Praan

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

[Daniel Mentz]

On Wed, Nov 27, 2024 at 12:53 PM Pranjal Shrivastava <praan@google.com> wrote:
>
> On Wed, Nov 27, 2024 at 11:42:11AM -0800, Daniel Mentz wrote:
> > On Wed, Nov 27, 2024 at 1:25 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > > > +       event->class = FIELD_GET(EVTQ_1_CLASS, raw[1]);
> > > > > +       event->iova = FIELD_GET(EVTQ_2_ADDR, raw[2]);
> > > > > +       event->ipa = raw[3];
> > > >
> > > > Shouldn't this be
> > > >
> > > > event->ipa =FIELD_GET(EVTQ_3_IPA, raw[3]);
> > >
> > > No. For this, there's a reason for not using the
> > > `FIELD_GET(EVTQ_3_IPA, raw[3]);`. As mentioned in the cover letter, this
> > > was the root cause of the IPA truncation bug, i.e. FIELD_GET only
> > > fetches the IPA field without trailing zeroes, truncating the address.
> > > For e.g. IPA = 0xfffff instead of 0xfffff000 which is faulting address.
> >
> > I disagree with the term truncation bug, because the information was
> > never there in the first place. Truncating the IPA was presumably a
> > conscious decision made by the architect. It is not a bug.
> >
>
> I'm NOT claiming that the HW or architecture has a bug.
> I meant that SW can't ONLY look at (and log) bits 51:12 only which the
> `FIELD_GET(EVTQ_3_IPA, raw[3]);` does (the values I've shared in the
> cover letter are actual values printed on QEMU). If we'd wanna use the
> `FIELD_GET(EVTQ_3_IPA, raw[3]);` then we'd also have to shift it by 12.

I believe that using FIELD_GET(EVTQ_3_IPA, raw[3]) and then left
shifting by 12 is the correct approach. As per the Arm ARM, software
shouldn't rely on RES0 bits being 0.

> > >
> > > Since the SMMUv3 spec ensures that the bitfields NOT representing an
> > > address, i.e. fields other than GENMASK_ULL(51,12) or GENMASK_ULL(51,3),
> > > are zeros. It's safe to read the raw[3] dword directly.
> >
> > Somewhere in the Arm ARM, I found the following language:
> >
> > "The RES0 description can apply to bits or fields that are read-only,
> > or are write-only: For a read-only bit, RES0 indicates that the bit
> > reads as 0, but software must treat the bit as UNKNOWN."
> > "A bit that is RES0 in a context is reserved for possible future use
> > in that context. To preserve forward compatibility, software: Must not
> > rely on the bit reading as 0."
> >
> > As it stands, I believe the code relies on RES0 fields reading 0 which
> > would violate this rule.
>
> The fields I'm talking about aren't marked as `RES0` in the spec. For
> example, if we look at Section 7.3.4 in the spec, F_STE_FETCH
> *explicitly* marks non-address fields as 0 not RES0. The same can be
> noticed in sections 7.3.10, 7.3.12 - 7.3.17 & 7.3.20. Unless I missed
> something that mentions that the "0" fields in event records are RES0?

It's RES0 in version G.a of the spec. It appears as if Arm changed
that between versions D.a and D.b.

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

[Pranjal Shrivastava]

On Thu, Nov 28, 2024 at 03:40:51PM -0800, Daniel Mentz wrote:
> On Wed, Nov 27, 2024 at 12:53 PM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Wed, Nov 27, 2024 at 11:42:11AM -0800, Daniel Mentz wrote:
> > > On Wed, Nov 27, 2024 at 1:25 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > > > > +       event->class = FIELD_GET(EVTQ_1_CLASS, raw[1]);
> > > > > > +       event->iova = FIELD_GET(EVTQ_2_ADDR, raw[2]);
> > > > > > +       event->ipa = raw[3];
> > > > >
> > > > > Shouldn't this be
> > > > >
> > > > > event->ipa =FIELD_GET(EVTQ_3_IPA, raw[3]);
> > > >
> > > > No. For this, there's a reason for not using the
> > > > `FIELD_GET(EVTQ_3_IPA, raw[3]);`. As mentioned in the cover letter, this
> > > > was the root cause of the IPA truncation bug, i.e. FIELD_GET only
> > > > fetches the IPA field without trailing zeroes, truncating the address.
> > > > For e.g. IPA = 0xfffff instead of 0xfffff000 which is faulting address.
> > >
> > > I disagree with the term truncation bug, because the information was
> > > never there in the first place. Truncating the IPA was presumably a
> > > conscious decision made by the architect. It is not a bug.
> > >
> >
> > I'm NOT claiming that the HW or architecture has a bug.
> > I meant that SW can't ONLY look at (and log) bits 51:12 only which the
> > `FIELD_GET(EVTQ_3_IPA, raw[3]);` does (the values I've shared in the
> > cover letter are actual values printed on QEMU). If we'd wanna use the
> > `FIELD_GET(EVTQ_3_IPA, raw[3]);` then we'd also have to shift it by 12.
> 
> I believe that using FIELD_GET(EVTQ_3_IPA, raw[3]) and then left
> shifting by 12 is the correct approach. As per the Arm ARM, software
> shouldn't rely on RES0 bits being 0.
> 

Ack. Looking at the G.a version[1] of the spec, I agree with this.
Apologies for the confusion!

I'm planning to take the raw[3] dword, mask it appropriately and store
it in 2 different members, ipa & fetch_addr: 

	event->ipa = raw[3] & EVTQ_3_IPA_MASK;
	event->fetch_addr = raw[3] & EVTQ_3_FETCH_ADDR_MASK;

This way, we can use either field as required and not clobber the decode
func with if (event->id == 'a' || event->id == 'b' || event->id == 'c').
Allowing the user to use the appropriate member  as necessary.

For example, in the `arm_smmu_dump_event` we'll use event->ipa or
event->fetch_addr as per the `event->id`

I'm slightly against the `event_id` based checks because it may not
scale well if (in the future) more fetch faults are added.

> > > >
> > > > Since the SMMUv3 spec ensures that the bitfields NOT representing an
> > > > address, i.e. fields other than GENMASK_ULL(51,12) or GENMASK_ULL(51,3),
> > > > are zeros. It's safe to read the raw[3] dword directly.
> > >
> > > Somewhere in the Arm ARM, I found the following language:
> > >
> > > "The RES0 description can apply to bits or fields that are read-only,
> > > or are write-only: For a read-only bit, RES0 indicates that the bit
> > > reads as 0, but software must treat the bit as UNKNOWN."
> > > "A bit that is RES0 in a context is reserved for possible future use
> > > in that context. To preserve forward compatibility, software: Must not
> > > rely on the bit reading as 0."
> > >
> > > As it stands, I believe the code relies on RES0 fields reading 0 which
> > > would violate this rule.
> >
> > The fields I'm talking about aren't marked as `RES0` in the spec. For
> > example, if we look at Section 7.3.4 in the spec, F_STE_FETCH
> > *explicitly* marks non-address fields as 0 not RES0. The same can be
> > noticed in sections 7.3.10, 7.3.12 - 7.3.17 & 7.3.20. Unless I missed
> > something that mentions that the "0" fields in event records are RES0?
> 
> It's RES0 in version G.a of the spec. It appears as if Arm changed
> that between versions D.a and D.b.

Ahh alright. I was referring to D.a version. Thanks for catching this!
The G.a version clearly marks everything as RES0 which can cause trouble

Thanks,
Praan

[1] https://developer.arm.com/documentation/ihi0070/latest/

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

[Pranjal Shrivastava]

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

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 110 ++++++++++++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  14 ++-
 2 files changed, 112 insertions(+), 12 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 e1b69aa04382..c0ecd74efe58 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,28 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ 0, NULL},
 };
 
+static const char * const event_str[] = {
+	[EVT_ID_BAD_STREAMID_CONFIG] = "C_BAD_STREAMID",
+	[EVT_ID_STE_FETCH_FAULT] = "F_STE_FETCH",
+	[EVT_ID_BAD_STE_CONFIG] = "C_BAD_STE",
+	[EVT_ID_STREAM_DISABLED_FAULT] = "F_STREAM_DISABLED",
+	[EVT_ID_BAD_SUBSTREAMID_CONFIG] = "C_BAD_SUBSTREAMID",
+	[EVT_ID_CD_FETCH_FAULT] = "F_CD_FETCH",
+	[EVT_ID_BAD_CD_CONFIG] = "C_BAD_CD",
+	[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",
+	[EVT_ID_VMS_FETCH_FAULT] = "F_VMS_FETCH",
+};
+
+static const char * const event_class_str[] = {
+	[0] = "CD fetch",
+	[1] = "Stage 1 translation table fetch",
+	[2] = "Input address caused fault ",
+	[3] = "Reserved",
+};
+
 static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
 				    struct arm_smmu_device *smmu, u32 flags);
 static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
@@ -1757,8 +1779,11 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
 }
 
 /* IRQ and event handlers */
-static void arm_smmu_decode_event(u64 *raw, struct arm_smmu_event *event)
+static void arm_smmu_decode_event(struct arm_smmu_device *smmu, u64 *raw,
+				  struct arm_smmu_event *event)
 {
+	struct arm_smmu_master *master;
+
 	event->id = FIELD_GET(EVTQ_0_ID, raw[0]);
 	event->sid = FIELD_GET(EVTQ_0_SID, raw[0]);
 	event->ssv = raw[0] & EVTQ_0_SSV;
@@ -1772,6 +1797,18 @@ static void arm_smmu_decode_event(u64 *raw, struct arm_smmu_event *event)
 	event->class = FIELD_GET(EVTQ_1_CLASS, raw[1]);
 	event->iova = FIELD_GET(EVTQ_2_ADDR, raw[2]);
 	event->ipa = raw[3];
+	event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, raw[1]);
+	event->class_tt = false;
+	event->dev = NULL;
+
+	if (event->id == EVT_ID_PERMISSION_FAULT)
+		event->class_tt = (event->class == EVTQ_1_CLASS_TT);
+
+	mutex_lock(&smmu->streams_mutex);
+	master = arm_smmu_find_master(smmu, event->sid);
+	if (master)
+		event->dev = get_device(master->dev);
+	mutex_unlock(&smmu->streams_mutex);
 }
 
 static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
@@ -1833,9 +1870,65 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
 	return ret;
 }
 
+static void arm_smmu_dump_raw_event(struct arm_smmu_device *smmu, u64 *raw,
+				    struct arm_smmu_event *event)
+{
+	int i;
+
+	dev_err(smmu->dev, "event 0x%02x received:\n", event->id);
+
+	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
+		dev_err(smmu->dev, "\t0x%016llx\n", raw[i]);
+}
+
+#define ARM_SMMU_EVT_KNOWN(e)	((e)->id < ARRAY_SIZE(event_str) && event_str[(e)->id])
+#define ARM_SMMU_LOG_EVT_STR(e) ARM_SMMU_EVT_KNOWN(e) ? event_str[(e)->id] : "UNKNOWN"
+#define ARM_SMMU_LOG_CLIENT(e)	(e)->dev ? dev_name((e)->dev) : "(unassigned sid)"
+
+#define ARM_SMMU_EVT_LOG_PFX	"Event %#x received: %s\n\tclient: %s sid: %#x ssid: %#x\n"
+#define ARM_SMMU_EVT_LOG_FMT(e) (e)->id, ARM_SMMU_LOG_EVT_STR(e), \
+				ARM_SMMU_LOG_CLIENT(e), (e)->sid, (e)->ssid
+
+#define ARM_SMMU_EVT_LOG(dev, evt, msg, ...)    \
+	dev_err(dev, ARM_SMMU_EVT_LOG_PFX msg "\n", ARM_SMMU_EVT_LOG_FMT(evt), ##__VA_ARGS__)
+
+static void arm_smmu_dump_event(struct arm_smmu_device *smmu, u64 *raw,
+				struct arm_smmu_event *evt,
+				struct ratelimit_state *rs)
+{
+	if (!__ratelimit(rs))
+		return;
+
+	arm_smmu_dump_raw_event(smmu, raw, evt);
+
+	switch (evt->id) {
+	case EVT_ID_TRANSLATION_FAULT:
+	case EVT_ID_ADDR_SIZE_FAULT:
+	case EVT_ID_ACCESS_FAULT:
+	case EVT_ID_PERMISSION_FAULT:
+		ARM_SMMU_EVT_LOG(smmu->dev, evt,
+				 "\tiova %#llx ipa %#llx\n\t%s | %s | %s | %s | %s%s\n\t%sSTAG: %#x",
+				 evt->iova, evt->ipa, evt->privileged ? "Priv" : "Unpriv",
+				 evt->instruction ? "Inst" : "Data",
+				 evt->read ? "Read" : "Write",
+				 evt->s2 ? "S2" : "S1", event_class_str[evt->class],
+				 evt->class_tt ? (evt->ttrnw ? " | TTD Read" : " | TTD Write") : "",
+				 evt->stall ? "Stall | " : "", evt->stag);
+		break;
+
+	case EVT_ID_STE_FETCH_FAULT:
+	case EVT_ID_CD_FETCH_FAULT:
+	case EVT_ID_VMS_FETCH_FAULT:
+		ARM_SMMU_EVT_LOG(smmu->dev, evt, "\tFetch Address: %#llx", evt->ipa);
+		break;
+
+	default:
+		ARM_SMMU_EVT_LOG(smmu->dev, evt, "");
+	}
+}
+
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
-	int i, ret;
 	u64 raw_evt[EVTQ_ENT_DWORDS];
 	struct arm_smmu_event evt = {0};
 	struct arm_smmu_device *smmu = dev;
@@ -1847,16 +1940,11 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 	do {
 		while (!queue_remove_raw(q, raw_evt)) {
 
-			arm_smmu_decode_event(raw_evt, &evt);
-			ret = arm_smmu_handle_evt(smmu, &evt);
-			if (!ret || !__ratelimit(&rs))
-				continue;
-
-			dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
-			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
-				dev_info(smmu->dev, "\t0x%016llx\n",
-					(unsigned long long)raw_evt[i]);
+			arm_smmu_decode_event(smmu, raw_evt, &evt);
+			if (arm_smmu_handle_evt(smmu, &evt))
+				arm_smmu_dump_event(smmu, raw_evt, &evt, &rs);
 
+			put_device(evt.dev);
 			cond_resched();
 		}
 
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index abb543d987f6..33bcea7fc03d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -437,10 +437,18 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
 
 #define EVTQ_0_ID			GENMASK_ULL(7, 0)
 
+#define EVT_ID_BAD_STREAMID_CONFIG	0x02
+#define EVT_ID_STE_FETCH_FAULT		0x03
+#define EVT_ID_BAD_STE_CONFIG		0x04
+#define EVT_ID_STREAM_DISABLED_FAULT	0x06
+#define EVT_ID_BAD_SUBSTREAMID_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)
@@ -452,6 +460,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
 #define EVTQ_1_RnW			(1UL << 35)
 #define EVTQ_1_S2			(1UL << 39)
 #define EVTQ_1_CLASS			GENMASK_ULL(41, 40)
+#define EVTQ_1_CLASS_TT			0x01
 #define EVTQ_1_TT_READ			(1UL << 44)
 #define EVTQ_2_ADDR			GENMASK_ULL(63, 0)
 #define EVTQ_3_IPA			GENMASK_ULL(51, 12)
@@ -777,7 +786,9 @@ struct arm_smmu_event {
 					privileged	: 1,
 					instruction	: 1,
 					s2		: 1,
-					read		: 1;
+					read		: 1,
+					ttrnw		: 1,
+					class_tt	: 1;
 	u8				id;
 	u8				class;
 	u16				stag;
@@ -785,6 +796,7 @@ struct arm_smmu_event {
 	u32				ssid;
 	u64				iova;
 	u64				ipa;
+	struct device			*dev;
 };
 
 /* SMMU private data for each master */
-- 
2.47.0.277.g8800431eea-goog

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

[Pranjal Shrivastava]

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

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

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 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 c0ecd74efe58..ddb7850b098c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1814,9 +1814,7 @@ static void arm_smmu_decode_event(struct arm_smmu_device *smmu, u64 *raw,
 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;
 	struct iopf_fault fault_evt = { };
 	struct iommu_fault *flt = &fault_evt.fault;
 
@@ -1857,17 +1855,14 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
 		flt->prm.pasid = event->ssid;
 	}
 
-	mutex_lock(&smmu->streams_mutex);
-	master = arm_smmu_find_master(smmu, event->sid);
-	if (!master) {
-		ret = -EINVAL;
-		goto out_unlock;
-	}
+	/*
+	 * If the master wasn't found while reading the event or
+	 * get_device() returned NULL, we shouldn't report a fault.
+	 */
+	if (!event->dev)
+		return -EINVAL;
 
-	ret = iommu_report_device_fault(master->dev, &fault_evt);
-out_unlock:
-	mutex_unlock(&smmu->streams_mutex);
-	return ret;
+	return iommu_report_device_fault(event->dev, &fault_evt);
 }
 
 static void arm_smmu_dump_raw_event(struct arm_smmu_device *smmu, u64 *raw,
@@ -1927,6 +1922,7 @@ static void arm_smmu_dump_event(struct arm_smmu_device *smmu, u64 *raw,
 	}
 }
 
+
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
 	u64 raw_evt[EVTQ_ENT_DWORDS];
-- 
2.47.0.277.g8800431eea-goog

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

[Nicolin Chen]

On Tue, Nov 12, 2024 at 08:30:18AM +0000, Pranjal Shrivastava wrote:
> Remove the call to `arm_smmu_find_master` in `arm_smmu_handle_evt`.
> 
> The call was primarily to retrieve `master->dev` ptr for reporting iommu
> device faults. Since this info is already available in the `event->dev`
> field, we no longer need to lookup the master to fetch the dev pointer.

We only protected the "dev" for dev_name() while the streams_mutex
is still required against arm_smmu_remove_master?

> @@ -1857,17 +1855,14 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
>                 flt->prm.pasid = event->ssid;
>         }
> 
> -       mutex_lock(&smmu->streams_mutex);
> -       master = arm_smmu_find_master(smmu, event->sid);
> -       if (!master) {
> -               ret = -EINVAL;
> -               goto out_unlock;
> -       }
> +       /*
> +        * If the master wasn't found while reading the event or
> +        * get_device() returned NULL, we shouldn't report a fault.
> +        */
> +       if (!event->dev)
> +               return -EINVAL;
> 
> -       ret = iommu_report_device_fault(master->dev, &fault_evt);
> -out_unlock:
> -       mutex_unlock(&smmu->streams_mutex);
> -       return ret;
> +       return iommu_report_device_fault(event->dev, &fault_evt);

Apart from the question that I asked above, it'd be convenient for
my vIRQ series to add on top if we can keep the master here:
https://github.com/nicolinc/iommufd/blob/66b3f9cdb39920880fb052b53f11ceb16af007fe/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c#L1782

Given that your flow is:
	arm_smmu_decode_event(...);		// get event->dev
	if (arm_smmu_handle_evt(...))		// validate event->dev
		arm_smmu_dump_event(...);	// use event->dev 

Could we just keep the driver as it is, so your flow will be:
	arm_smmu_decode_event(...);		// no event->dev
	if (arm_smmu_handle_evt(...))		// get event->dev 
		arm_smmu_dump_event(...);	// use event->dev
?

>  static void arm_smmu_dump_raw_event(struct arm_smmu_device *smmu, u64 *raw,
> @@ -1927,6 +1922,7 @@ static void arm_smmu_dump_event(struct arm_smmu_device *smmu, u64 *raw,
>         }
>  }
> 
> +
>  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)

Should drop this.

Thanks
Nicolin

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

[Pranjal Shrivastava]

On Tue, Nov 12, 2024 at 03:52:36PM -0800, Nicolin Chen wrote:
> On Tue, Nov 12, 2024 at 08:30:18AM +0000, Pranjal Shrivastava wrote:
> > Remove the call to `arm_smmu_find_master` in `arm_smmu_handle_evt`.
> > 
> > The call was primarily to retrieve `master->dev` ptr for reporting iommu
> > device faults. Since this info is already available in the `event->dev`
> > field, we no longer need to lookup the master to fetch the dev pointer.
> 
> We only protected the "dev" for dev_name() while the streams_mutex
> is still required against arm_smmu_remove_master?
> 

Umm.. as per the code upstream, we were just finding master because we
wanted to pass the master->dev in "iommu_report_device_fault", thus even
if the master is free'd by arm_smmu_remove_master, the struct dev
wouldn't be free'd as we still hold a reference to it here due to the
call to get_device in arm_smmu_decode_event.

> > @@ -1857,17 +1855,14 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
> >                 flt->prm.pasid = event->ssid;
> >         }
> > 
> > -       mutex_lock(&smmu->streams_mutex);
> > -       master = arm_smmu_find_master(smmu, event->sid);
> > -       if (!master) {
> > -               ret = -EINVAL;
> > -               goto out_unlock;
> > -       }
> > +       /*
> > +        * If the master wasn't found while reading the event or
> > +        * get_device() returned NULL, we shouldn't report a fault.
> > +        */
> > +       if (!event->dev)
> > +               return -EINVAL;
> > 
> > -       ret = iommu_report_device_fault(master->dev, &fault_evt);
> > -out_unlock:
> > -       mutex_unlock(&smmu->streams_mutex);
> > -       return ret;
> > +       return iommu_report_device_fault(event->dev, &fault_evt);
> 
> Apart from the question that I asked above, it'd be convenient for
> my vIRQ series to add on top if we can keep the master here:
> https://github.com/nicolinc/iommufd/blob/66b3f9cdb39920880fb052b53f11ceb16af007fe/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c#L1782
> 
> Given that your flow is:
> 	arm_smmu_decode_event(...);		// get event->dev
> 	if (arm_smmu_handle_evt(...))		// validate event->dev
> 		arm_smmu_dump_event(...);	// use event->dev 
> 
> Could we just keep the driver as it is, so your flow will be:
> 	arm_smmu_decode_event(...);		// no event->dev
> 	if (arm_smmu_handle_evt(...))		// get event->dev 
> 		arm_smmu_dump_event(...);	// use event->dev
> ?

Hmm.. looking at the vIRQ series it seems like we'd anyways need to call
arm_smmu_find_master here as you're adding more fields to that struct
and plan to use them here. 

In that case, I don't think we need this patch [Patch 3/3] ? 
We can keep the code as is and simply populate the event->dev ptr in
arm_smmu_handle_evt when we lock the streams_mutex as you suggested.

> 
> >  static void arm_smmu_dump_raw_event(struct arm_smmu_device *smmu, u64 *raw,
> > @@ -1927,6 +1922,7 @@ static void arm_smmu_dump_event(struct arm_smmu_device *smmu, u64 *raw,
> >         }
> >  }
> > 
> > +
> >  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> 
> Should drop this.

Ahh, my bad, will remove that.
Interesting to note that checkpatch didn't crib about it!

> 
> Thanks
> Nicolin

Thanks,
Praan

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

[Nicolin Chen]

On Fri, Nov 15, 2024 at 01:26:22PM +0000, Pranjal Shrivastava wrote:
> On Tue, Nov 12, 2024 at 03:52:36PM -0800, Nicolin Chen wrote:
> > On Tue, Nov 12, 2024 at 08:30:18AM +0000, Pranjal Shrivastava wrote:
> > > Remove the call to `arm_smmu_find_master` in `arm_smmu_handle_evt`.
> > > 
> > > The call was primarily to retrieve `master->dev` ptr for reporting iommu
> > > device faults. Since this info is already available in the `event->dev`
> > > field, we no longer need to lookup the master to fetch the dev pointer.
> > 
> > We only protected the "dev" for dev_name() while the streams_mutex
> > is still required against arm_smmu_remove_master?
> > 
> 
> Umm.. as per the code upstream, we were just finding master because we
> wanted to pass the master->dev in "iommu_report_device_fault", thus even
> if the master is free'd by arm_smmu_remove_master, the struct dev
> wouldn't be free'd as we still hold a reference to it here due to the
> call to get_device in arm_smmu_decode_event.

The iommu_report_device_fault() needs to dereference dev->iommu
and dev->iommu_group, both of which might be NULL if the master
gets freed?

> > > @@ -1857,17 +1855,14 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu,
> > >                 flt->prm.pasid = event->ssid;
> > >         }
> > > 
> > > -       mutex_lock(&smmu->streams_mutex);
> > > -       master = arm_smmu_find_master(smmu, event->sid);
> > > -       if (!master) {
> > > -               ret = -EINVAL;
> > > -               goto out_unlock;
> > > -       }
> > > +       /*
> > > +        * If the master wasn't found while reading the event or
> > > +        * get_device() returned NULL, we shouldn't report a fault.
> > > +        */
> > > +       if (!event->dev)
> > > +               return -EINVAL;
> > > 
> > > -       ret = iommu_report_device_fault(master->dev, &fault_evt);
> > > -out_unlock:
> > > -       mutex_unlock(&smmu->streams_mutex);
> > > -       return ret;
> > > +       return iommu_report_device_fault(event->dev, &fault_evt);
> > 
> > Apart from the question that I asked above, it'd be convenient for
> > my vIRQ series to add on top if we can keep the master here:
> > 
> > Given that your flow is:
> > 	arm_smmu_decode_event(...);		// get event->dev
> > 	if (arm_smmu_handle_evt(...))		// validate event->dev
> > 		arm_smmu_dump_event(...);	// use event->dev 
> > 
> > Could we just keep the driver as it is, so your flow will be:
> > 	arm_smmu_decode_event(...);		// no event->dev
> > 	if (arm_smmu_handle_evt(...))		// get event->dev 
> > 		arm_smmu_dump_event(...);	// use event->dev
> > ?
> 
> Hmm.. looking at the vIRQ series it seems like we'd anyways need to call
> arm_smmu_find_master here as you're adding more fields to that struct
> and plan to use them here. 
> 
> In that case, I don't think we need this patch [Patch 3/3] ? 
> We can keep the code as is and simply populate the event->dev ptr in
> arm_smmu_handle_evt when we lock the streams_mutex as you suggested.

Yea, that works the best for me.

Thanks
Nicolin

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

[Daniel Mentz]

On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
>
> Enhance the arm-smmu-v3 driver to parse out useful information from
> event records into a structure for better event handling & logging.

Hi Pranjal,

Thank you for putting this together.

>
> Some sample events, powered by QEMU:

> 4. Translation Fault:
>
> [    7.586428] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x10 received:
> [    7.587012] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000010
> [    7.587504] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
> [    7.587986] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
> [    7.588745] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
> [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
> [    7.589219]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
> [    7.589219]  iova 0xfffff040 ipa 0x0
> [    7.589219]  Unpriv | Data | Write | S1 | Input address caused fault
> [    7.589219]  STAG: 0x0

I find that "Event 0x10" is redundant and recommend removing it. Maybe
print just "Event F_TRANSLATION"

Not printing the prefix "arm-smmu-v3 arm-smmu-v3.0.auto:" on
subsequent lines appears to be inconsistent with how the raw event
bits are dumped. Also, I typically run a command like "grep
'arm-smmu-v3 arm-smmu-v3.0.auto' uart.log" while I'm developing smmu
driver code, and with the proposed change, the grep command wouldn't
print most of the event details, because they are not prefixed by
"arm-smmu-v3 arm-smmu-v3.0.auto"

The words "client", "sid" and "ssid" are followed by a colon (":") but
iova and ipa are not.

Can you put everything up to and including iova on the first line and
the rest on the second line?

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

[Pranjal Shrivastava]

Hi Daniel,

On Wed, Nov 20, 2024 at 10:15 AM Daniel Mentz <danielmentz@google.com> wrote:
>
> On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > Enhance the arm-smmu-v3 driver to parse out useful information from
> > event records into a structure for better event handling & logging.
>
> Hi Pranjal,
>
> Thank you for putting this together.
>
> >
> > Some sample events, powered by QEMU:
>
> > 4. Translation Fault:
> >
> > [    7.586428] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x10 received:
> > [    7.587012] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000010
> > [    7.587504] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
> > [    7.587986] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
> > [    7.588745] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
> > [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
> > [    7.589219]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
> > [    7.589219]  iova 0xfffff040 ipa 0x0
> > [    7.589219]  Unpriv | Data | Write | S1 | Input address caused fault
> > [    7.589219]  STAG: 0x0
>
> I find that "Event 0x10" is redundant and recommend removing it. Maybe
> print just "Event F_TRANSLATION"

I agree, however, in the past reviews [1] we've been asked to keep the
existing log
"as is" unless there's a particularly compelling reason to change them.

>
> Not printing the prefix "arm-smmu-v3 arm-smmu-v3.0.auto:" on
> subsequent lines appears to be inconsistent with how the raw event
> bits are dumped. Also, I typically run a command like "grep
> 'arm-smmu-v3 arm-smmu-v3.0.auto' uart.log" while I'm developing smmu
> driver code, and with the proposed change, the grep command wouldn't
> print most of the event details, because they are not prefixed by
> "arm-smmu-v3 arm-smmu-v3.0.auto"

I see. The prefix isn't getting picked up because I'm trying to print
all the lines in
a single `dev_err` in order to avoid other logs inter-mingling with
these ones. I can add
that if we are okay with using multiple `dev_err` calls (i.e.
potentially allowing other logs
to mix-in).  Otherwise, let me see how we can manually add the dev_fmt
string in the log.

>
> The words "client", "sid" and "ssid" are followed by a colon (":") but
> iova and ipa are not.
>

Ack. Will fix that.

> Can you put everything up to and including iova on the first line and
> the rest on the second line?

Do you mean something like the following:
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received:
F_TRANSLATION
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: client: 0000:00:01.0
sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Unpriv | Data | Write |
S1 | Input address caused fault | STAG: 0x0

Thanks.
Praan

[1] https://lore.kernel.org/all/08498356-d892-4756-89e1-45b2654faa42@arm.com/#:~:text=safest%20to%20leave%20any%20existing%20%0Amessages%20exactly

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

[Pranjal Shrivastava]

Hello,
On Wed, Nov 20, 2024 at 12:24 PM Pranjal Shrivastava <praan@google.com> wrote:
>
> Hi Daniel,
>
> On Wed, Nov 20, 2024 at 10:15 AM Daniel Mentz <danielmentz@google.com> wrote:
> >
> > On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
> > >
> > > Enhance the arm-smmu-v3 driver to parse out useful information from
> > > event records into a structure for better event handling & logging.
> >
> > Hi Pranjal,
> >
> > Thank you for putting this together.
> >
> > >
> > > Some sample events, powered by QEMU:
> >
> > > 4. Translation Fault:
> > >
> > > [    7.586428] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x10 received:
> > > [    7.587012] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000010
> > > [    7.587504] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
> > > [    7.587986] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
> > > [    7.588745] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
> > > [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
> > > [    7.589219]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
> > > [    7.589219]  iova 0xfffff040 ipa 0x0
> > > [    7.589219]  Unpriv | Data | Write | S1 | Input address caused fault
> > > [    7.589219]  STAG: 0x0
> >
> > I find that "Event 0x10" is redundant and recommend removing it. Maybe
> > print just "Event F_TRANSLATION"
>
> I agree, however, in the past reviews [1] we've been asked to keep the
> existing log
> "as is" unless there's a particularly compelling reason to change them.
>
> >
> > Not printing the prefix "arm-smmu-v3 arm-smmu-v3.0.auto:" on
> > subsequent lines appears to be inconsistent with how the raw event
> > bits are dumped. Also, I typically run a command like "grep
> > 'arm-smmu-v3 arm-smmu-v3.0.auto' uart.log" while I'm developing smmu
> > driver code, and with the proposed change, the grep command wouldn't
> > print most of the event details, because they are not prefixed by
> > "arm-smmu-v3 arm-smmu-v3.0.auto"
>
> I see. The prefix isn't getting picked up because I'm trying to print
> all the lines in
> a single `dev_err` in order to avoid other logs inter-mingling with
> these ones. I can add
> that if we are okay with using multiple `dev_err` calls (i.e.
> potentially allowing other logs
> to mix-in).  Otherwise, let me see how we can manually add the dev_fmt
> string in the log.
>
> >
> > The words "client", "sid" and "ssid" are followed by a colon (":") but
> > iova and ipa are not.
> >
>
> Ack. Will fix that.
>
> > Can you put everything up to and including iova on the first line and
> > the rest on the second line?
>

Apologies, I'm not able to use my regular mail client due to corp mail issues.
I'll try again. Do you mean something like the following:

[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received:
F_TRANSLATION
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: client: 0000:00:01.0
sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Unpriv | Data | Write |
S1 | Input address caused fault | STAG: 0x0


> Thanks.
> Praan
>
> [1] https://lore.kernel.org/all/08498356-d892-4756-89e1-45b2654faa42@arm.com/#:~:text=safest%20to%20leave%20any%20existing%20%0Amessages%20exactly

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

[Pranjal Shrivastava]

On Wed, Nov 20, 2024 at 12:24:43PM +0530, Pranjal Shrivastava wrote:
> Hi Daniel,
> 
> On Wed, Nov 20, 2024 at 10:15 AM Daniel Mentz <danielmentz@google.com> wrote:
> >
> > On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
> > >
> > > Enhance the arm-smmu-v3 driver to parse out useful information from
> > > event records into a structure for better event handling & logging.
> >
> > Hi Pranjal,
> >
> > Thank you for putting this together.
> >
> > >
> > > Some sample events, powered by QEMU:
> >
> > > 4. Translation Fault:
> > >
> > > [    7.586428] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x10 received:
> > > [    7.587012] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000010
> > > [    7.587504] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
> > > [    7.587986] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
> > > [    7.588745] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
> > > [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
> > > [    7.589219]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
> > > [    7.589219]  iova 0xfffff040 ipa 0x0
> > > [    7.589219]  Unpriv | Data | Write | S1 | Input address caused fault
> > > [    7.589219]  STAG: 0x0
> >
> > I find that "Event 0x10" is redundant and recommend removing it. Maybe
> > print just "Event F_TRANSLATION"
>

I agree, however, in the past reviews [1] we've been asked to keep the
existing log "as is" unless there's a particularly compelling reason.

> 
> >
> > Not printing the prefix "arm-smmu-v3 arm-smmu-v3.0.auto:" on
> > subsequent lines appears to be inconsistent with how the raw event
> > bits are dumped. Also, I typically run a command like "grep
> > 'arm-smmu-v3 arm-smmu-v3.0.auto' uart.log" while I'm developing smmu
> > driver code, and with the proposed change, the grep command wouldn't
> > print most of the event details, because they are not prefixed by
> > "arm-smmu-v3 arm-smmu-v3.0.auto"
> 
> I see. The prefix isn't getting picked up because I'm trying to print
> all the lines in
> a single `dev_err` in order to avoid other logs inter-mingling with
> these ones. I can add
> that if we are okay with using multiple `dev_err` calls (i.e.
> potentially allowing other logs
> to mix-in).  Otherwise, let me see how we can manually add the dev_fmt
> string in the log.
> 
> >
> > The words "client", "sid" and "ssid" are followed by a colon (":") but
> > iova and ipa are not.
> >
> 
> Ack. Will fix that.
> 
> > Can you put everything up to and including iova on the first line and
> > the rest on the second line?

Finally my email client is fine. 
Did you mean something like the following:
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: client: 0000:00:01.0 sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Unpriv | Data | Write | S1 | Input address caused fault | STAG: 0x0

Thanks.
Praan
 
[1] https://lore.kernel.org/all/08498356-d892-4756-89e1-45b2654faa42@arm.com/#:~:text=safest%20to%20leave%20any%20existing%20%0Amessages%20exactly

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

[Daniel Mentz]

On Wed, Nov 20, 2024 at 3:43 AM Pranjal Shrivastava <praan@google.com> wrote:
>
> On Wed, Nov 20, 2024 at 12:24:43PM +0530, Pranjal Shrivastava wrote:
> > Hi Daniel,
> >
> > On Wed, Nov 20, 2024 at 10:15 AM Daniel Mentz <danielmentz@google.com> wrote:
> > >
> > > On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > >
> > > > Enhance the arm-smmu-v3 driver to parse out useful information from
> > > > event records into a structure for better event handling & logging.
> > >
> > > Hi Pranjal,
> > >
> > > Thank you for putting this together.
> > >
> > > >
> > > > Some sample events, powered by QEMU:
> > >
> > > > 4. Translation Fault:
> > > >
> > > > [    7.586428] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x10 received:
> > > > [    7.587012] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000010
> > > > [    7.587504] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
> > > > [    7.587986] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
> > > > [    7.588745] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
> > > > [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
> > > > [    7.589219]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
> > > > [    7.589219]  iova 0xfffff040 ipa 0x0
> > > > [    7.589219]  Unpriv | Data | Write | S1 | Input address caused fault
> > > > [    7.589219]  STAG: 0x0
> > >
> > > I find that "Event 0x10" is redundant and recommend removing it. Maybe
> > > print just "Event F_TRANSLATION"
> >
>
> I agree, however, in the past reviews [1] we've been asked to keep the
> existing log "as is" unless there's a particularly compelling reason.

I believe that feedback was about changing "event 0x%02x received:\n"
to "event 0x%02x received: master %s:\n" on the line that immediately
precedes the raw dump, but we're not changing that. What I am
suggesting is to remove "Event 0x10 received:" from "Event 0x10
received: F_TRANSLATION" and replace it with "event F_TRANSLATION"

>
> >
> > >
> > > Not printing the prefix "arm-smmu-v3 arm-smmu-v3.0.auto:" on
> > > subsequent lines appears to be inconsistent with how the raw event
> > > bits are dumped. Also, I typically run a command like "grep
> > > 'arm-smmu-v3 arm-smmu-v3.0.auto' uart.log" while I'm developing smmu
> > > driver code, and with the proposed change, the grep command wouldn't
> > > print most of the event details, because they are not prefixed by
> > > "arm-smmu-v3 arm-smmu-v3.0.auto"
> >
> > I see. The prefix isn't getting picked up because I'm trying to print
> > all the lines in
> > a single `dev_err` in order to avoid other logs inter-mingling with
> > these ones. I can add
> > that if we are okay with using multiple `dev_err` calls (i.e.
> > potentially allowing other logs
> > to mix-in).  Otherwise, let me see how we can manually add the dev_fmt
> > string in the log.
> >
> > >
> > > The words "client", "sid" and "ssid" are followed by a colon (":") but
> > > iova and ipa are not.
> > >
> >
> > Ack. Will fix that.
> >
> > > Can you put everything up to and including iova on the first line and
> > > the rest on the second line?
>
> Finally my email client is fine.
> Did you mean something like the following:
> [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
> [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: client: 0000:00:01.0 sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
> [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Unpriv | Data | Write | S1 | Input address caused fault | STAG: 0x0
>

I was thinking about something like the following:

[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: event F_TRANSLATION
client: 0000:00:01.0 sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: unpriv data write s1
"Input address caused fault" stag: 0x0

For sid, ssid, iova and ipa, you used all lowercase. I suggest
extending this rule to Unpriv, Data, Write etc. and use lower case
there as well. I'd remove the "|" characters to save space. I'm not
sure how to represent the "Input address caused fault" part without
the "|" characters to separate it from the rest. Maybe we can put it
in double quotes.

> Thanks.
> Praan
>
> [1] https://lore.kernel.org/all/08498356-d892-4756-89e1-45b2654faa42@arm.com/#:~:text=safest%20to%20leave%20any%20existing%20%0Amessages%20exactly

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

[Pranjal Shrivastava]

On Fri, Nov 22, 2024 at 03:33:42PM -0800, Daniel Mentz wrote:
> On Wed, Nov 20, 2024 at 3:43 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Wed, Nov 20, 2024 at 12:24:43PM +0530, Pranjal Shrivastava wrote:
> > > Hi Daniel,
> > >
> > > On Wed, Nov 20, 2024 at 10:15 AM Daniel Mentz <danielmentz@google.com> wrote:
> > > >
> > > > On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > > >
> > > > > Enhance the arm-smmu-v3 driver to parse out useful information from
> > > > > event records into a structure for better event handling & logging.
> > > >
> > > > Hi Pranjal,
> > > >
> > > > Thank you for putting this together.
> > > >
> > > > >
> > > > > Some sample events, powered by QEMU:
> > > >
> > > > > 4. Translation Fault:
> > > > >
> > > > > [    7.586428] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x10 received:
> > > > > [    7.587012] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000800000010
> > > > > [    7.587504] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000020000000000
> > > > > [    7.587986] arm-smmu-v3 arm-smmu-v3.0.auto:  0x00000000fffff040
> > > > > [    7.588745] arm-smmu-v3 arm-smmu-v3.0.auto:  0x0000000000000000
> > > > > [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
> > > > > [    7.589219]  client: 0000:00:01.0 sid: 0x8 ssid: 0x0
> > > > > [    7.589219]  iova 0xfffff040 ipa 0x0
> > > > > [    7.589219]  Unpriv | Data | Write | S1 | Input address caused fault
> > > > > [    7.589219]  STAG: 0x0
> > > >
> > > > I find that "Event 0x10" is redundant and recommend removing it. Maybe
> > > > print just "Event F_TRANSLATION"
> > >
> >
> > I agree, however, in the past reviews [1] we've been asked to keep the
> > existing log "as is" unless there's a particularly compelling reason.
> 
> I believe that feedback was about changing "event 0x%02x received:\n"
> to "event 0x%02x received: master %s:\n" on the line that immediately
> precedes the raw dump, but we're not changing that. What I am
> suggesting is to remove "Event 0x10 received:" from "Event 0x10
> received: F_TRANSLATION" and replace it with "event F_TRANSLATION"
> 

Ahh, apologies. I'll remove the event code from the pretty print.

> >
> > >
> > > >
> > > > Not printing the prefix "arm-smmu-v3 arm-smmu-v3.0.auto:" on
> > > > subsequent lines appears to be inconsistent with how the raw event
> > > > bits are dumped. Also, I typically run a command like "grep
> > > > 'arm-smmu-v3 arm-smmu-v3.0.auto' uart.log" while I'm developing smmu
> > > > driver code, and with the proposed change, the grep command wouldn't
> > > > print most of the event details, because they are not prefixed by
> > > > "arm-smmu-v3 arm-smmu-v3.0.auto"
> > >
> > > I see. The prefix isn't getting picked up because I'm trying to print
> > > all the lines in
> > > a single `dev_err` in order to avoid other logs inter-mingling with
> > > these ones. I can add
> > > that if we are okay with using multiple `dev_err` calls (i.e.
> > > potentially allowing other logs
> > > to mix-in).  Otherwise, let me see how we can manually add the dev_fmt
> > > string in the log.
> > >
> > > >
> > > > The words "client", "sid" and "ssid" are followed by a colon (":") but
> > > > iova and ipa are not.
> > > >
> > >
> > > Ack. Will fix that.
> > >
> > > > Can you put everything up to and including iova on the first line and
> > > > the rest on the second line?
> >
> > Finally my email client is fine.
> > Did you mean something like the following:
> > [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Event 0x10 received: F_TRANSLATION
> > [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: client: 0000:00:01.0 sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
> > [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: Unpriv | Data | Write | S1 | Input address caused fault | STAG: 0x0
> >
> 
> I was thinking about something like the following:
> 
> [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: event F_TRANSLATION
> client: 0000:00:01.0 sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
> [    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: unpriv data write s1
> "Input address caused fault" stag: 0x0
> 

Ack. This might make the 1st line longer (the following is 133 columns):

[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: event: F_TRANSLATION client: 0000:00:01.0 sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: unpriv data write s1 "Input address caused fault" stag: 0x0

Though, the worst case length would be something like:

[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: event: F_TRANSLATION client: 0000:00:01.0 sid: 0x8 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
[    7.589219] arm-smmu-v3 arm-smmu-v3.0.auto: unpriv data write s2 "Stage 1 translation table fetch" ttd_write stall stag: 0x0

I like this as well, provided the length would be fine?

> For sid, ssid, iova and ipa, you used all lowercase. I suggest
> extending this rule to Unpriv, Data, Write etc. and use lower case
> there as well. I'd remove the "|" characters to save space. I'm not
> sure how to represent the "Input address caused fault" part without
> the "|" characters to separate it from the rest. Maybe we can put it
> in double quotes.
> 

Also, if we wanna print everything in 2 lines along with the dev prefix,
I think we can do away with all the complex macro logic and have simply
two `dev_err`logs? Something like the attached below.

Thanks
Praan

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

dev_err(smmu->dev, "Event received: %s client: %s sid: %#x ssid: %#x iova: %#x ipa: %#x",
	ARM_SMMU_EVT_NAME(evt), dev_name(evt->dev), evt->sid, evt->ssid, evt->iova, evt->ipa);

dev_err(smmu->dev, "%s %s %s %s %s %s %sSTAG: %#x",
	evt->privileged ? "priv" : "unpriv",
	evt->instruction ? "inst" : "data",
	evt->read ? "read" : "write",
	evt->s2 ? "s2" : "s1", event_class_str[evt->class],
	evt->class_tt ? (evt->ttrnw ? "ttd_read" : "ttd_write") : "",
	evt->stall ? "stall " : "", evt->stag);