Re: [PATCH 6/7] iommu: Use right way to retrieve iommu_ops

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

On 2022-01-25 01:11, Tian, Kevin wrote:
>> From: Jason Gunthorpe via iommu
>> Sent: Tuesday, January 25, 2022 1:37 AM
>>> @@ -1295,7 +1298,7 @@ int iommu_page_response(struct device *dev,
>>>   			msg->pasid = 0;
>>>   		}
>>>
>>> -		ret = domain->ops->page_response(dev, evt, msg);
>>> +		ret = ops->page_response(dev, evt, msg);
>>>   		list_del(&evt->list);
>>>   		kfree(evt);
>>>   		break;
>>
>> Feels weird that page_response is not connected to a domain, the fault
>> originated from a domain after all. I would say this op should be
>> moved to the domain and the caller should provide the a pointer to the
>> domain that originated the fault.
>>
> 
> In concept yes.

Not even that, really. It's true that the "fault" itself is logically 
associated with the domain, but we never see that - the ATS request and 
response which encapsulate it all happen automatically on the PCI side. 
It's the endpoint that then decides to handle ATS translation failure 
via PRI, so all we actually get is a page request message from a 
RID/PASID, which most definitely represents the "device" (and in fact 
having to work backwards from there to figure out which domain/context 
it is currently attached to can be a bit of a pain). Similarly the 
response is a message directly back to the device itself - an operation 
on a domain may (or may not) have happened off the back of receiving the 
initial request, but even if the content of the response is to reflect 
that, the operation of responding is clearly focused on the device.

I fully agree that it's a weird-looking model, but that's how PCI SIG 
made it - and no IOMMU architecture seems to have tried to wrap it up in 
anything nicer either - so I don't see that we'd gain much from trying 
to pretend otherwise :)

Robin.

> But currently the entire sva path is not associated with domain. This was
> one mistake as we discussed in the cover letter. Before extending iommu
> domain to cover CPU page tables we may have to leave it in iommu_ops
> given this series is just for cleanup...
> 
> Thanks
> Kevin