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

[Jason Gunthorpe <jgg@nvidia.com>]

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 :\

Maybe we should stash something in the dev_iommu instead?

Or can the PRI stuff provide a cookie per-device?

But it will work like this

>        dev_id = iommufd_get_device_id(hwpt->ioas->ictx, dev);

Where did the hwpt come from?

Jason