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

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Oct 21, 2024 at 06:26:50AM +0000, Pranjal Shrivastava wrote:
> On Fri, Oct 18, 2024 at 07:06:49PM -0700, Nicolin Chen wrote:
> > On Fri, Oct 18, 2024 at 06:00:21PM +0000, Pranjal Shrivastava wrote:
> > > +static void arm_smmu_dump_event(struct arm_smmu_event *evt, struct ratelimit_state *rs)
> > > +{
> > > +       struct arm_smmu_device *smmu = evt->smmu;
> > > +       char title[100] = {0};
> > > +       char mastr[100] = {0};
> > > +       char addrs[100] = {0};
> > > +       char flags[100] = {0};
> > > +       char other[50] = {0};
> > > +
> > > +       if (!__ratelimit(rs))
> > > +               return;
> > > +
> > > +       snprintf(title, 100, "Unexpected event received: %s\n", event_str[evt->id]);
> > > +       snprintf(mastr, 100, "\tmaster: %s sid: 0x%08x.0x%05x\n",
> > > +                evt->master_name, evt->sid, evt->ssid);
> >
> > Likely I mentioned in the cover-letter, maybe "sid.ssid:"?
> >
> 
> +1. I like the idea. Maybe let's update the log in ppr to follow this
> as well? Does that sound good?

Oh right, you mentioned that once.. Yea, I think that would be
nicer.

> > > +       switch (evt->id) {
> > > +       case EVT_ID_TRANSLATION_FAULT:
> > > +       case EVT_ID_ADDR_SIZE_FAULT:
> > > +       case EVT_ID_ACCESS_FAULT:
> > > +       case EVT_ID_PERMISSION_FAULT:
> > > +               snprintf(addrs, 100, "\tiova = %#llx ipa = %#llx\n", evt->iova, evt->ipa);
> > > +               snprintf(other, 50, "\tSTAG = %#x Stall = %#x\n", evt->stag, evt->stall);
> > > +               snprintf(flags, 100, "\t%s%s%s%s%s%s\n",
> > > +                        evt->privileged ? "Priv | " : "Unpriv | ",
> > > +                        evt->instruction ? "Inst | " : "Data | ",
> > > +                        evt->read ? "Read | " : "Write | ",
> > > +                        evt->s2 ? "S2 | " : "S1 | ", event_class_str[evt->class],
> > > +                        evt->ttrnw_valid ? (evt->ttrnw ? "| TTD Read" : "| TTD Write") : "");
> >
> > Should the last one be "TTD Read |" : "TTD Write|"?
> > Otherwise, it would be "S2 || TTD Read" combined.
> 
> Umm.. I don't expect the event_class_str[evt->class] to be NULL ever.
> Hence the logs would always be in the following format:
> 
> Unpriv | Data | Write | S2 | <class_str> | TTD <Read/Write>

Ah, I overlooked that one :) We are fine then.

THanks
Nicolin