Re: [PATCH v4 1/3] iommu: Sort out domain user data

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

On 2025/3/7 13:57, Nicolin Chen wrote:
> On Fri, Mar 07, 2025 at 10:28:20AM +0800, Baolu Lu wrote:
>> On 3/7/25 05:00, Nicolin Chen wrote:
>>> From: Robin Murphy<robin.murphy@arm.com>
> Robin had remarks here, wrt iommu_set_fault_handler():
> 
>>> The fact is that all these cookie types are
>>> mutually exclusive, in the sense that combining them makes zero sense
>>> and/or would be catastrophic (iommu_set_fault_handler() on an SVA
>>> domain, anyone?) - the only combination which*might* be reasonable is
>>> perhaps a fault handler and an MSI cookie, but nobody's doing that at
>>> the moment, so let's rule it out as well for the sake of being clear and
>>> robust.
> [...]
>>> @@ -224,10 +234,10 @@ struct iommu_domain {
>>>    		      phys_addr_t msi_addr);
>>>    #endif
>>> -	union { /* Pointer usable by owner of the domain */
>>> -		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
>>> -	};
>>> -	union { /* Fault handler */
>>> +	union { /* cookie */
>>> +		struct iommu_dma_cookie *iova_cookie;
>>> +		struct iommu_dma_msi_cookie *msi_cookie;
>>> +		struct iommufd_hw_pagetable *iommufd_hwpt;
>>>    		struct {
>>>    			iommu_fault_handler_t handler;
>>>    			void *handler_token;exs
>> My feeling is that IOMMU_COOKIE_FAULT_HANDLER isn't exclusive to
>> IOMMU_COOKIE_DMA_IOVA; both might be used for kernel DMA with a paging
>> domain.
>>
>> I am afraid that iommu_set_fault_handler() doesn't work anymore as the
>> domain's cookie type has already been set to IOMMU_COOKIE_DMA_IOVA.
> All three existing iommu_set_fault_handler() callers in the tree
> are UNMANAGED domain users:
>     5    451  drivers/gpu/drm/msm/msm_iommu.c <<msm_iommu_gpu_new>>
>               iommu_set_fault_handler(iommu->domain, msm_fault_handler, iommu);
>     6    453  drivers/infiniband/hw/usnic/usnic_uiom.c <<usnic_uiom_alloc_pd>>
>               iommu_set_fault_handler(pd->domain, usnic_uiom_dma_fault, NULL);
>     8    118  drivers/remoteproc/remoteproc_core.c <<rproc_enable_iommu>>
>               iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
> 
> On the other hand, IOMMU_COOKIE_DMA_IOVA is a private cookie for
> dma-iommu only.
> 
> So, I think we are probably fine?
If all existing use cases are for UNMANAGED domains, that's fine. And
when iommu_set_fault_handler() is miss-used, we already have a WARN_ON()
there.

Thanks,
baolu