Re: [PATCH 1/1] iommu: Bind process address spaces to devices

[Jacob Pan]

On Thu, 28 Feb 2019 12:19:22 +0000
Jean-Philippe Brucker <jean-philippe.brucker@xxxxxxx> wrote:

> On 27/02/2019 21:41, Jacob Pan wrote:
> > On Tue, 26 Feb 2019 12:17:43 +0100
> > Joerg Roedel <joro@xxxxxxxxxx> wrote:
> >   
> >> Hi Jean-Philippe,
> >>
> >> Thanks for the patch! I think this is getting close to be applied
> >> after the next merge window.
> >>
> >> On Wed, Feb 20, 2019 at 02:27:59PM +0000, Jean-Philippe Brucker
> >> wrote:  
> >>> +int iommu_sva_bind_device(struct device *dev, struct mm_struct
> >>> *mm, int *pasid,
> >>> +			  iommu_mm_exit_handler_t mm_exit, void
> >>> *drvdata)    
> >>
> >> I think we are better of with introducing a sva-bind handle which
> >> can be used to extend and further configure the binding done with
> >> this function.
> >>
> >> How about a 'struct iommu_sva' with an iommu-private definition
> >> that is returned by this function:
> >>
> >> 	struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> >> 						struct mm_struct
> >> *mm); 
> > Just trying to understand how to use this API.
> > So if we bind the same mm to two different devices, we should get
> > two different iommu_sva handle, right?  
> 
> Yes, the iommu_sva handle is the bond between one mm and one device,
> so you will get two different handles.
> 
> > I think intel-svm still needs a flag argument for supervisor pasid
> > etc. Other than that, I think both interface should work for vt-d.  
> 
> Is supervisor PASID still needed now that we have auxiliary domains,
> and now that VT-d supports nested IOVA? You could have private kernel
> address spaces through auxiliary domains, or simply use DMA API as
> usual with PASID#0. I've been reluctant to make that feature common
> because it looks risky - gives full access to the kernel address
> space to devices and no notification on mapping change.
> 
It is still in the VT-d spec. Ashok will be able to answer this
better :)
> > Another question is that for nested SVA, we will need to bind guest
> > mm. Do you think we should try to reuse this or have it separate? I
> > am working on a separate API for now.  
> 
> I also think it should be separate. That bind() operation is performed
> on an auxiliary domain, I guess?
> 
yes the 2nd level is retrieved from aux domain for mdev, but for pdev,
2nd level comes from rid2pasid/default domain.
> >> and the corresponding unbind function:
> >>
> >> 	int iommu_sva_unbind_device(struct iommu_sva* *handle);
> >>
> >> (Btw, does this need to return and int? Can unbinding fail?).
> >>
> >> With that in place we can implement and extentable API base on the
> >> handle:
> >>
> >> 	int iommu_sva_get_pasid(struct iommu_sva *handle);  
> > If multiple bind to the same mm gets multiple handles, this API
> > should retrieve the same pasid for different handle?  
> 
> Yes
> 
> > Just curious why making
> > the handle private instead of returning the pasid value in the
> > handle?  
> 
> I don't have a strong objection against that. One reason to have an
> accessor is that the PASID is freed on mm_exit, so until the device
> driver calls unbind(), the PASID contained in the handle is stale (and
> the accessor returns PASID_INVALID). But that's a bit pedantic, the
> device driver should know that the handle is stale since it got
> notified of it. Having an accessor might also help tracking uses of
> the handle in the kernel, and make future API modifications easier.
> 
make sense.
> Thanks,
> Jean
> 
> >> 	void iommu_sva_set_exit_handler(struct iommu_sva *handle,
> >> 					iommu_mm_exit_handler_t
> >> mm_exit);
> >>
> >> I think at least the AMD IOMMU driver needs more call-backs like a
> >> handler that is invoked when a fault can not be resolved. And there
> >> might be others in the future, putting them all in the parameter
> >> list of the bind function doesn't scale well.
> >>  
> >   
> >> Regards,
> >>
> >> 	Joerg  
> > 
> >   
> 

[Jacob Pan]