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

[Pranjal Shrivastava <praan@google.com>]

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?

> > +       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>

> 
> >  static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> >                                         struct arm_smmu_event *event)
> >  {
> > +       struct arm_smmu_master *master;
> > +
> >         /* Pick out the good stuff */
> >         event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> >         event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > @@ -1833,12 +1917,24 @@ static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> >         event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
> >         event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
> >         event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> > +       event->ttrnw = FIELD_GET(EVTQ_1_TT_READ, event->raw[1]);
> > +       event->ttrnw_valid = false;
> >         event->smmu = smmu;
> > +       event->dev = NULL;
> > +
> > +       if (event->id == EVT_ID_PERMISSION_FAULT)
> > +               event->ttrnw_valid = (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);
> 
> Here, get_device is called upon a valid master...
> 
> >  static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >  {
> > -       int i, ret;
> >         struct arm_smmu_event evt;
> >         struct arm_smmu_device *smmu = dev;
> >         struct arm_smmu_queue *q = &smmu->evtq.q;
> > @@ -1850,15 +1946,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >                 while (!queue_remove_raw(q, evt.raw)) {
> > 
> >                         arm_smmu_get_event_from_raw(smmu, &evt);
> > -                       ret = arm_smmu_handle_evt(&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)evt.raw[i]);
> > +                       if (arm_smmu_handle_evt(&evt))
> > +                               arm_smmu_dump_event(&evt, &rs);
> > 
> > +                       put_device(evt.dev);
> 
> then, here it puts unconditionally.
> 
> Maybe we do need a memset(0) to the event, then here
> 			if (evet.dev)
> 				put_device(evt.dev);

`put_device` takes care of NULL args as well.

> 
> Thanks
> Nicolin

Thanks!
Pranjal