Re: [PATCH v2 1/3] docs: IOMMU user API

From: Alex Williamson <alex.williamson@redhat.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: "Raj, Ashok" <ashok.raj@intel.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Jean-Philippe Brucker <jean-philippe@linaro.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Christoph Hellwig <hch@infradead.org>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH v2 1/3] docs: IOMMU user API
Date: Thu, 18 Jun 2020 15:48:05 -0600	[thread overview]
Message-ID: <20200618154805.049219db@w520.home> (raw)
In-Reply-To: <MWHPR11MB164595B754BE441255902DCA8C9A0@MWHPR11MB1645.namprd11.prod.outlook.com>

On Wed, 17 Jun 2020 08:28:24 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Wednesday, June 17, 2020 2:20 PM
> >   
> > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > Sent: Tuesday, June 16, 2020 11:22 PM
> > >
> > > On Thu, 11 Jun 2020 17:27:27 -0700
> > > Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> > >  
> > > > >
> > > > > But then I thought it even better if VFIO leaves the entire
> > > > > copy_from_user() to the layer consuming it.
> > > > >  
> > > > OK. Sounds good, that was what Kevin suggested also. I just wasn't
> > > > sure how much VFIO wants to inspect, I thought VFIO layer wanted to do
> > > > a sanity check.
> > > >
> > > > Anyway, I will move copy_from_user to iommu uapi layer.  
> > >
> > > Just one more point brought up by Yi when we discuss this offline.
> > >
> > > If we move copy_from_user to iommu uapi layer, then there will be  
> > multiple  
> > > copy_from_user calls for the same data when a VFIO container has  
> > multiple domains,  
> > > devices. For bind, it might be OK. But might be additional overhead for TLB  
> > flush  
> > > request from the guest.  
> > 
> > I think it is the same with bind and TLB flush path. will be multiple
> > copy_from_user.  
> 
> multiple copies is possibly fine. In reality we allow only one group per
> nesting container (as described in patch [03/15]), and usually there
> is just one SVA-capable device per group.
> 
> > 
> > BTW. for moving data copy to iommy layer, there is another point which
> > need to consider. VFIO needs to do unbind in bind path if bind failed,
> > so it will assemble unbind_data and pass to iommu layer. If iommu layer
> > do the copy_from_user, I think it will be failed. any idea?

If a call into a UAPI fails, there should be nothing to undo.  Creating
a partial setup for a failed call that needs to be undone by the caller
is not good practice.

> This might be mitigated if we go back to use the same bind_data for both
> bind/unbind. Then you can reuse the user object for unwinding.
> 
> However there is another case where VFIO may need to assemble the
> bind_data itself. When a VM is killed, VFIO needs to walk allocated PASIDs
> and unbind them one-by-one. In such case copy_from_user doesn't work
> since the data is created by kernel. Alex, do you have a suggestion how this
> usage can be supported? e.g. asking IOMMU driver to provide two sets of
> APIs to handle user/kernel generated requests?

Yes, it seems like vfio would need to make use of a driver API to do
this, we shouldn't be faking a user buffer in the kernel in order to
call through to a UAPI.  Thanks,

Alex

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu