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

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

On 2023/5/18 20:02, Jason Gunthorpe wrote:
> On Thu, May 18, 2023 at 03:05:23PM +0800, Baolu Lu wrote:
> 
>>> 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.
>>
>> If I get your point correctly, the iommu core should provide some places
>> for the iommufd to put a cookie for each pair of {device, pasid}, and
>> provide interfaces to manage it. For example,
> 
> I'd say when you attach a PRI capable domain (eg to a PASID) then provide
> a 'void * data' during the attach.
> 
>> If so, perhaps we need some special treatment for ARM as a user hwpt
>> actually presents the PASID table of the device and the guest setting
>> pasid table entry will not be propagated to host. Then, the @pasid in
>> above interfaces is meaningless.
> 
> As above, when attaching to a RID you'd still pass in the *data

Yes! Merging these with hwpt attach/detach would be more logical.

> 
>> 1) Move iommu faults uapi from uapi/linux/iommu.h to uapi/linux
>>    /iommufd.h and remove the former.
> 
> Please no, delete all the dead code from here and move whatever is
> still in use into include/linux/
> 
> Then we can talk about what parts of it become uAPI and how best to
> manage that on a patch by patch basis.

Okay, let's rebuild it from the ground up.

> 
>> 2) Add a device id in the iommu_fault structure.
>>   struct iommu_fault {
>>          __u32   type;
>> -       __u32   padding;
>> +       __u32   dev_id;
> 
> Why? This is iommufd internal, the void * above should cover it.

This is actually part of 1). :-)

> 
>> 3) Add the device pointer to the parameters of domain fault handler.
> 
> That makes some sense
>   
>> 4) Decouple I/O page fault handling from IOMMU_SVA in the iommu core and
>>     the drivers.
> 
> Definately, this SVA stuff need to be scrubbed out.
> 
> SVA is only a special domain type that takes the page table top from a
> mmu_stuct and a shared fault handler in the core code to do handle_mm_fault()
> 
> It should not be in drivers any more deeply than that. We still have a
> ways to go.

Agreed. In addition to fault handler, SVA usually needs to register a
callback to mm_notifier to keep the IO or device TLB cache consistent.
This part of code could also be consolidated to the core.

There are still things to do here, but the priority (from my point of
view) is to make the iopf handling framework in the iommu core more
generic, rather than just serving SVA.

Best regards,
baolu