Re: [PATCH v2 02/22] iommufd: Use iommu_user_domain_alloc()

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Jun 05, 2024 at 10:17:07AM +0800, Baolu Lu wrote:
> On 6/5/24 12:51 AM, Jason Gunthorpe wrote:
> > On Tue, Jun 04, 2024 at 09:51:14AM +0800, Lu Baolu wrote:
> > > Replace iommu_domain_alloc() with iommu_user_domain_alloc().
> > > 
> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > ---
> > >   drivers/iommu/iommufd/hw_pagetable.c | 20 +++++---------------
> > >   1 file changed, 5 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> > > index 33d142f8057d..ada05fccb36a 100644
> > > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > > @@ -127,21 +127,11 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
> > >   	hwpt_paging->ioas = ioas;
> > >   	hwpt_paging->nest_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
> > > -	if (ops->domain_alloc_user) {
> > > -		hwpt->domain = ops->domain_alloc_user(idev->dev, flags, NULL,
> > > -						      user_data);
> >                                                       ^^^^^^^^^^^^
> > 
> > > -		if (IS_ERR(hwpt->domain)) {
> > > -			rc = PTR_ERR(hwpt->domain);
> > > -			hwpt->domain = NULL;
> > > -			goto out_abort;
> > > -		}
> > > -		hwpt->domain->owner = ops;
> > > -	} else {
> > > -		hwpt->domain = iommu_domain_alloc(idev->dev->bus);
> > > -		if (!hwpt->domain) {
> > > -			rc = -ENOMEM;
> > > -			goto out_abort;
> > > -		}
> > > +	hwpt->domain = iommu_user_domain_alloc(idev->dev, flags);
> > > +	if (IS_ERR(hwpt->domain)) {
> > 
> > Where did the user_data go???
> 
> The user_data is not used in allocating a paging user domain, so I
> skipped it.

That's not true.. We have no driver using it right now, but it is
definately part of the uAPI and a driver could start using it. That is
why it was hooked up in the first place.

> In my first try, I simply replaced iommu_domain_alloc() with a new
> paging domain allocation interface. On second thought, it occurred to me
> why not use separate interfaces for different purposes? Even though from
> an iommu perspective, they both use paging domains.

If we want to do that then it needs to forward all the args

> The @flags and @user_data are defined in uapi/linux/iommufd.h, which is
> specific to iommufd. Wrapping them in a common interface for broader use
> seems awkward.

Right, you'd have to forward declare the struct and just let it
be. Really nothing but iommufd could call this API.
 
> So, perhaps we could return to my original idea? Let's just expose one
> interface, iommu_paging_domain_alloc(), and replace iommu_domain_alloc()
> with it everywhere?

That's OK too, this above doesn't really need to be changed, some of
the concept here was that iommufd-only ops would just be directly
called by iommufd itself, to discourage future abuse.

Jason