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

[Nicolin Chen <nicolinc@nvidia.com>]

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