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