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

[Robin Murphy <robin.murphy@arm.com>]

On 2025-03-07 7:03 am, Baolu Lu wrote:
> 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.

Right, it would be illogical for a driver to set a fault handler on a 
DMA domain since it doesn't control the IOVA space to be able to do any 
fault-handling, and iommu-dma itself isn't ever going to use a fault 
handler because it expects the DMA API to be used correctly and thus no 
faults to occur.

TBH at this point I view the fault_handler stuff as a legacy interface 
which we don't really want to encourage use of anyway - it's already 
proven not to be great for any true fault handling since many drivers 
can only call report_iommu_fault() in IRQ context. If some new case does 
come up in future where this mutual exclusion gets in the way, I would 
say that's the point where we then look at reworking the whole thing 
into a dedicated "fault notifier" mechanism instead, which could then 
logically be orthogonal to the IOVA-space-owner cookie.

Thanks,
Robin.