On Wed, May 17, 2023 at 06:33:30AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> > 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
