| Message ID | 20240604015134.164206-3-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Refactoring domain allocation interface | expand |
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??? If you are going to wrapper the op function then all the args need to be provided. I'm not sure there is value in having vfio and vdpa call this variation since they won't pass a user_data or flags? Do you imagine there will ever be a difference between what domain_alloc_user(dev, 0, NULL, NULL) returns from domain_alloc_paging(dev) ? That seems like questionable driver behavior to me. Jason
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. > > If you are going to wrapper the op function then all the args need to > be provided. Yes. Agreed. > I'm not sure there is value in having vfio and vdpa call this > variation since they won't pass a user_data or flags? > > Do you imagine there will ever be a difference between what > domain_alloc_user(dev, 0, NULL, NULL) returns from > domain_alloc_paging(dev) ? No. > > That seems like questionable driver behavior to me. 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. 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. 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? Best regards, baolu
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
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)) { + rc = PTR_ERR(hwpt->domain); + hwpt->domain = NULL; + goto out_abort; } /*
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(-)