Messages in this thread | | | Date | Fri, 13 Oct 2023 17:08:46 -0700 | From | Nicolin Chen <> | Subject | Re: [PATCH v4 07/17] iommufd: Add user-managed hw_pagetable support |
| |
On Tue, Sep 26, 2023 at 01:14:47AM -0700, Tian, Kevin wrote: > > From: Liu, Yi L <yi.l.liu@intel.com> > > Sent: Thursday, September 21, 2023 3:51 PM > > > > +static void iommufd_user_managed_hwpt_abort(struct iommufd_object > > *obj) > > +{ > > + struct iommufd_hw_pagetable *hwpt = > > + container_of(obj, struct iommufd_hw_pagetable, obj); > > + > > + /* The parent->mutex must be held until finalize is called. */ > > + lockdep_assert_held(&hwpt->parent->mutex); > > + > > + iommufd_hw_pagetable_destroy(obj); > > +} > > Can you elaborate what exactly is protected by parent->mutex? > > My gut-feeling that the child just grabs a refcnt on the parent > object. It doesn't change any other data of the parent.
Ah, you are right. It's added here just for symmetry so we wouldn't end up with something like: if (!hwpt->user_managed) mutex_lock(&hwpt->mutex); alloc_fn(); if (!hwpt->user_managed) mutex_unlock(&hwpt->mutex);
Perhaps I should move the pair of mutex calls to the kernel-managed hwpt allocator.
> > +/** > > + * iommufd_user_managed_hwpt_alloc() - Get a user-managed > > hw_pagetable > > + * @ictx: iommufd context > > + * @pt_obj: Parent object to an HWPT to associate the domain with > > + * @idev: Device to get an iommu_domain for > > + * @flags: Flags from userspace > > + * @hwpt_type: Requested type of hw_pagetable > > + * @user_data: user_data pointer > > + * @dummy: never used > > + * > > + * Allocate a new iommu_domain (must be IOMMU_DOMAIN_NESTED) and > > return it as > > + * a user-managed hw_pagetable. > > Add some text to highlight the requirement being a parent, e.g. not > an auto domain, must be capable of being a parent, etc.
OK.
> > + case IOMMUFD_OBJ_HW_PAGETABLE: > > + parent = container_of(pt_obj, struct iommufd_hw_pagetable, > > obj); > > + /* No user-managed HWPT on top of an user-managed one > > */ > > + if (parent->user_managed) { > > + rc = -EINVAL; > > + goto out_put_pt; > > + } > > move to alloc_fn().
Though being a bit covert, this is actually to avoid a data buffer allocation in the common pathway before calling alloc_fn(), which is added in the following patch. And the reason why it's in the common function is because we previously support a kernel-managed hwpt allocation with data too.
But now, I think we can just move this sanity and data allocation together into the user-managed hwpt allocator.
Thanks Nicolin
| |