diff mbox series

[01/12] iommu: Add new iommu op to create domains owned by userspace

Message ID 20230309080910.607396-2-yi.l.liu@intel.com (mailing list archive)
State New
Headers show
Series iommufd: Add nesting infrastructure | expand

Commit Message

Yi Liu March 9, 2023, 8:08 a.m. UTC 
From: Lu Baolu <baolu.lu@linux.intel.com>

Introduce a new iommu_domain op to create domains owned by userspace,
e.g. through iommufd. These domains have a few different properties
compares to kernel owned domains:

 - They may be UNMANAGED domains, but created with special parameters.
   For instance aperture size changes/number of levels, different
   IOPTE formats, or other things necessary to make a vIOMMU work

 - We have to track all the memory allocations with GFP_KERNEL_ACCOUNT
   to make the cgroup sandbox stronger

 - Device-specialty domains, such as NESTED domains can be created by
   iommufd.

The new op clearly says the domain is being created by IOMMUFD, that
the domain is intended for userspace use, and it provides a way to pass
a driver specific uAPI structure to customize the created domain to
exactly what the vIOMMU userspace driver requires.

iommu drivers that cannot support VFIO/IOMMUFD should not support this
op. This includes any driver that cannot provide a fully functional
UNMANAGED domain.

This op chooses to make the special parameters opaque to the core. This
suits the current usage model where accessing any of the IOMMU device
special parameters does require a userspace driver that matches the
kernel driver. If a need for common parameters, implemented similarly
by several drivers, arises then there is room in the design to grow a
generic parameter set as well.

This new op for now is only supposed to be used by iommufd, hence no
wrapper for it. iommufd would call the callback directly. As for domain
free, iommufd would use iommu_domain_free().

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 include/linux/iommu.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Jason Gunthorpe March 10, 2023, 12:56 a.m. UTC | #1 
On Thu, Mar 09, 2023 at 12:08:59AM -0800, Yi Liu wrote:
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 3ef84ee359d2..a269bc62a31c 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -229,6 +229,7 @@ struct iommu_iotlb_gather {
>   *           after use. Return the data buffer if success, or ERR_PTR on
>   *           failure.
>   * @domain_alloc: allocate iommu domain
> + * @domain_alloc_user: allocate user iommu domain
>   * @probe_device: Add device to iommu driver handling
>   * @release_device: Remove device from iommu driver handling
>   * @probe_finalize: Do final setup work after the device is added to an IOMMU
> @@ -266,6 +267,9 @@ struct iommu_ops {
>  
>  	/* Domain allocation and freeing by the iommu driver */
>  	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
> +	struct iommu_domain *(*domain_alloc_user)(struct device *dev,
> +						  struct iommu_domain *parent,
> +						  const void *user_data);

Since the kernel does the copy from user and manages the zero fill
compat maybe this user_data have a union like Robin suggested.

But yes, this is the idea.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason
Yi Liu March 29, 2023, 10:56 a.m. UTC | #2 
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, March 10, 2023 8:56 AM
> 
> On Thu, Mar 09, 2023 at 12:08:59AM -0800, Yi Liu wrote:
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 3ef84ee359d2..a269bc62a31c 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -229,6 +229,7 @@ struct iommu_iotlb_gather {
> >   *           after use. Return the data buffer if success, or ERR_PTR on
> >   *           failure.
> >   * @domain_alloc: allocate iommu domain
> > + * @domain_alloc_user: allocate user iommu domain
> >   * @probe_device: Add device to iommu driver handling
> >   * @release_device: Remove device from iommu driver handling
> >   * @probe_finalize: Do final setup work after the device is added to an
> IOMMU
> > @@ -266,6 +267,9 @@ struct iommu_ops {
> >
> >  	/* Domain allocation and freeing by the iommu driver */
> >  	struct iommu_domain *(*domain_alloc)(unsigned
> iommu_domain_type);
> > +	struct iommu_domain *(*domain_alloc_user)(struct device *dev,
> > +						  struct iommu_domain
> *parent,
> > +						  const void *user_data);
> 
> Since the kernel does the copy from user and manages the zero fill
> compat maybe this user_data have a union like Robin suggested.
>
> But yes, this is the idea.

Ok. so it's a union like the below, and in this patch may be only an empty
union can be added as the struct iommu_hwpt_intel_vtd and
struct iommu_hwpt_arm_smmuv3 would be added by the vendor nesting
patch series.

union iommu_hwpt_alloc_user_data {
	struct iommu_hwpt_intel_vtd vtd;
	struct iommu_hwpt_arm_smmuv3 smmuv3;
};

Regards,
Yi Liu
Nicolin Chen April 13, 2023, 12:44 a.m. UTC | #3 
Hi Jason,

On Thu, Mar 09, 2023 at 08:56:06PM -0400, Jason Gunthorpe wrote:
> On Thu, Mar 09, 2023 at 12:08:59AM -0800, Yi Liu wrote:
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 3ef84ee359d2..a269bc62a31c 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -229,6 +229,7 @@ struct iommu_iotlb_gather {
> >   *           after use. Return the data buffer if success, or ERR_PTR on
> >   *           failure.
> >   * @domain_alloc: allocate iommu domain
> > + * @domain_alloc_user: allocate user iommu domain
> >   * @probe_device: Add device to iommu driver handling
> >   * @release_device: Remove device from iommu driver handling
> >   * @probe_finalize: Do final setup work after the device is added to an IOMMU
> > @@ -266,6 +267,9 @@ struct iommu_ops {
> >  
> >  	/* Domain allocation and freeing by the iommu driver */
> >  	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
> > +	struct iommu_domain *(*domain_alloc_user)(struct device *dev,
> > +						  struct iommu_domain *parent,
> > +						  const void *user_data);
> 
> Since the kernel does the copy from user and manages the zero fill
> compat maybe this user_data have a union like Robin suggested.
> 
> But yes, this is the idea.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

We pass in a read-only data to this ->domain_alloc_user() while
it also returns NULL on failure, matching ->domain_alloc(). So,
there seems to be no error feedback pathway from the driver to
user space.

Robin remarked in the SMMU series that an STE configuration can
fail. So, a proper error feedback is required for this callback
too.

To return a driver/HW specific error, I think we could define a
"u8 out_error" in the user_data structure. So, we probably need
a non-const pass-in here. What do you think?

Thanks
Nic
Jason Gunthorpe April 13, 2023, 11:37 a.m. UTC | #4 
On Wed, Apr 12, 2023 at 05:44:04PM -0700, Nicolin Chen wrote:
> Hi Jason,
> 
> On Thu, Mar 09, 2023 at 08:56:06PM -0400, Jason Gunthorpe wrote:
> > On Thu, Mar 09, 2023 at 12:08:59AM -0800, Yi Liu wrote:
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index 3ef84ee359d2..a269bc62a31c 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -229,6 +229,7 @@ struct iommu_iotlb_gather {
> > >   *           after use. Return the data buffer if success, or ERR_PTR on
> > >   *           failure.
> > >   * @domain_alloc: allocate iommu domain
> > > + * @domain_alloc_user: allocate user iommu domain
> > >   * @probe_device: Add device to iommu driver handling
> > >   * @release_device: Remove device from iommu driver handling
> > >   * @probe_finalize: Do final setup work after the device is added to an IOMMU
> > > @@ -266,6 +267,9 @@ struct iommu_ops {
> > >  
> > >  	/* Domain allocation and freeing by the iommu driver */
> > >  	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
> > > +	struct iommu_domain *(*domain_alloc_user)(struct device *dev,
> > > +						  struct iommu_domain *parent,
> > > +						  const void *user_data);
> > 
> > Since the kernel does the copy from user and manages the zero fill
> > compat maybe this user_data have a union like Robin suggested.
> > 
> > But yes, this is the idea.
> > 
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> We pass in a read-only data to this ->domain_alloc_user() while
> it also returns NULL on failure, matching ->domain_alloc(). So,
> there seems to be no error feedback pathway from the driver to
> user space.
> 
> Robin remarked in the SMMU series that an STE configuration can
> fail. So, a proper error feedback is required for this callback
> too.
> 
> To return a driver/HW specific error, I think we could define a
> "u8 out_error" in the user_data structure. So, we probably need
> a non-const pass-in here. What do you think?

What is wrong with err_ptr?

Jason
Nicolin Chen April 13, 2023, 3:25 p.m. UTC | #5 
On Thu, Apr 13, 2023 at 08:37:14AM -0300, Jason Gunthorpe wrote:

> > > > @@ -266,6 +267,9 @@ struct iommu_ops {
> > > >  
> > > >  	/* Domain allocation and freeing by the iommu driver */
> > > >  	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
> > > > +	struct iommu_domain *(*domain_alloc_user)(struct device *dev,
> > > > +						  struct iommu_domain *parent,
> > > > +						  const void *user_data);
> > > 
> > > Since the kernel does the copy from user and manages the zero fill
> > > compat maybe this user_data have a union like Robin suggested.
> > > 
> > > But yes, this is the idea.
> > > 
> > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > 
> > We pass in a read-only data to this ->domain_alloc_user() while
> > it also returns NULL on failure, matching ->domain_alloc(). So,
> > there seems to be no error feedback pathway from the driver to
> > user space.
> > 
> > Robin remarked in the SMMU series that an STE configuration can
> > fail. So, a proper error feedback is required for this callback
> > too.
> > 
> > To return a driver/HW specific error, I think we could define a
> > "u8 out_error" in the user_data structure. So, we probably need
> > a non-const pass-in here. What do you think?
> 
> What is wrong with err_ptr?

I see. That could keep the "const" then. Will try that.

Thanks!
Nic
diff mbox series

Patch

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 3ef84ee359d2..a269bc62a31c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -229,6 +229,7 @@  struct iommu_iotlb_gather {
  *           after use. Return the data buffer if success, or ERR_PTR on
  *           failure.
  * @domain_alloc: allocate iommu domain
+ * @domain_alloc_user: allocate user iommu domain
  * @probe_device: Add device to iommu driver handling
  * @release_device: Remove device from iommu driver handling
  * @probe_finalize: Do final setup work after the device is added to an IOMMU
@@ -266,6 +267,9 @@  struct iommu_ops {
 
 	/* Domain allocation and freeing by the iommu driver */
 	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
+	struct iommu_domain *(*domain_alloc_user)(struct device *dev,
+						  struct iommu_domain *parent,
+						  const void *user_data);
 
 	struct iommu_device *(*probe_device)(struct device *dev);
 	void (*release_device)(struct device *dev);