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

[Lu Baolu]

On 1/25/22 10:08 AM, Tian, Kevin wrote:
> > From: Robin Murphy <robin.murphy@xxxxxxx>
> > Sent: Tuesday, January 25, 2022 9:52 AM
> >
> > 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 :)
>
> I think the point here is that although page requests are received
> per device from the wire the low level iommu driver should convert
> those requests into domain-wide requests (with RID/PASID recorded
> as private data in the request) which then can be handled by domain
> ops in iommu core. Once a domain-wide request is completed by
> the iommu core, the low level iommu driver then retrieves RID/PASID
> information from private data of the completed request and triggers
> page response per RID/PASID in bus specific way.

I also have a pending series to associate the sva with an iommu domain
and make the existing I/O page fault framework generic (vs. sva
specific). Perhaps we can discuss the page fault handle/response there
with the real code.

> Does it sound reasonable?
>
> Thanks
> Kevin

Best regards,
baolu