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

[Pranjal Shrivastava <praan@google.com>]

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