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

[Nicolin Chen <nicolinc@nvidia.com>]

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