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

[Baolu Lu <baolu.lu@linux.intel.com>]

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?

> 
> But it will work like this
> 
>>         dev_id = iommufd_get_device_id(hwpt->ioas->ictx, dev);
> 
> Where did the hwpt come from?

It is installed when setting up the iopf handler for the hwpt.

+	iommu_domain_set_iopf_handler(hwpt->domain,
+                                     iommufd_hw_pagetable_iopf_handler,
+                                     hwpt);

Best regards,
baolu