Re: [PATCH v7 03/19] iommufd: Replace the hwpt->devices list with iommufd_group

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, May 17, 2023 at 06:33:30AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Wednesday, May 17, 2023 12:15 PM
> > 
> > On 5/16/23 8:27 PM, Jason Gunthorpe wrote:
> > > On Tue, May 16, 2023 at 11:00:16AM +0800, Baolu Lu wrote:
> > >> On 5/15/23 10:00 PM, Jason Gunthorpe wrote:
> > >>> The devices list was used as a simple way to avoid having per-group
> > >>> information. Now that this seems to be unavoidable, just commit to
> > >>> per-group information fully and remove the devices list from the HWPT.
> > >>>
> > >>> The iommufd_group stores the currently assigned HWPT for the entire
> > group
> > >>> and we can manage the per-device attach/detach with a list in the
> > >>> iommufd_group.
> > >>
> > >> I am preparing the patches to route I/O page faults to user space
> > >> through iommufd. The iommufd page fault handler knows the hwpt and
> > the
> > >> device pointer, but it needs to convert the device pointer into its
> > >> iommufd object id and pass the id to user space.
> > >>
> > >> It's fine that we remove the hwpt->devices here, but perhaps I need to
> > >> add the context pointer in ioas later,
> > >>
> > >> struct iommufd_ioas {
> > >>          struct io_pagetable iopt;
> > >>          struct mutex mutex;
> > >>          struct list_head hwpt_list;
> > >> +       struct iommufd_ctx *ictx;
> > >>   };
> > >>
> > >> and, use below helper to look up the device id.
> > >>
> > >> +u32 iommufd_get_device_id(struct iommufd_ctx *ictx, struct device *dev)
> > >> +{
> > >> +       struct iommu_group *group = iommu_group_get(dev);
> > >> +       u32 dev_id = IOMMUFD_INVALID_OBJ_ID;
> > >> +       struct iommufd_group *igroup;
> > >> +       struct iommufd_device *cur;
> > >> +       unsigned int id;
> > >> +
> > >> +       if (!group)
> > >> +               return IOMMUFD_INVALID_OBJ_ID;
> > >> +
> > >> +       id = iommu_group_id(group);
> > >> +       xa_lock(&ictx->groups);
> > >> +       igroup = xa_load(&ictx->groups, id);
> > >> +       if (!iommufd_group_try_get(igroup, group)) {
> > >> +               xa_unlock(&ictx->groups);
> > >> +               iommu_group_put(group);
> > >> +               return IOMMUFD_INVALID_OBJ_ID;
> > >> +        }
> > >> +        xa_unlock(&ictx->groups);
> > >> +
> > >> +       mutex_lock(&igroup->lock);
> > >> +       list_for_each_entry(cur, &igroup->device_list, group_item) {
> > >> +               if (cur->dev == dev) {
> > >> +                       dev_id = cur->obj.id;
> > >> +                       break;
> > >> +               }
> > >> +       }
> > >
> > > I dislike how slow this is on something resembling a fastish path :\
> > 
> > Yes, agreed.
> > 
> > > Maybe we should stash something in the dev_iommu instead?
> > >
> > > Or can the PRI stuff provide a cookie per-device?
> > 
> > We already have a per-device fault cookie:
> > 
> > /**
> >   * struct iommu_fault_param - per-device IOMMU fault data
> >   * @handler: Callback function to handle IOMMU faults at device level
> >   * @data: handler private data
> >   * @faults: holds the pending faults which needs response
> >   * @lock: protect pending faults list
> >   */
> > struct iommu_fault_param {
> >          iommu_dev_fault_handler_t handler;
> >          void *data;
> >          struct list_head faults;
> >          struct mutex lock;
> > };
> > 
> > Perhaps we can add a @dev_id memory here?
> > 
> 
> what about SIOV? There is only one cookie per parent device.

It doesn't make any sense to store a struct like that in dev_iommu.

The fault handler should come from the domain and we should be able to
have a unique 'void *data' cookie linked to the (dev,PASID) to go
along with the fault handler.

This is all going to need some revising before we can expose it to
iommufd

Jason