| Message ID | 20230308131340.459224-2-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Make emulated devices prepared for vfio device cdev | expand |
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Wednesday, March 8, 2023 9:14 PM > > @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx *ictx, > u32 ioas_id, > access->data = data; > access->ops = ops; > > - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > - if (IS_ERR(obj)) { > - rc = PTR_ERR(obj); > - goto out_abort; > - } > - access->ioas = container_of(obj, struct iommufd_ioas, obj); > - iommufd_ref_to_users(obj); > - > if (ops->needs_pin_pages) > access->iova_alignment = PAGE_SIZE; > else > access->iova_alignment = 1; > - rc = iopt_add_access(&access->ioas->iopt, access); > - if (rc) > - goto out_put_ioas; > > /* The calling driver is a user until iommufd_access_destroy() */ > refcount_inc(&access->obj.users); > + mutex_init(&access->ioas_lock); > access->ictx = ictx; > iommufd_ctx_get(ictx); this refcnt get should be moved to the start given next patch removes the reference in the caller side.
On Wed, Mar 08, 2023 at 05:13:36AM -0800, Yi Liu wrote: > +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id) > +{ > + struct iommufd_ioas *new_ioas = NULL, *cur_ioas; > + struct iommufd_ctx *ictx = access->ictx; > + struct iommufd_object *obj; > + int rc = 0; > + > + if (ioas_id) { > + obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > + if (IS_ERR(obj)) > + return PTR_ERR(obj); > + new_ioas = container_of(obj, struct iommufd_ioas, obj); > + } > + > + mutex_lock(&access->ioas_lock); > + cur_ioas = access->ioas; > + if (cur_ioas == new_ioas) > + goto out_unlock; > + > + if (new_ioas) { > + rc = iopt_add_access(&new_ioas->iopt, access); > + if (rc) > + goto out_unlock; > + iommufd_ref_to_users(obj); > + } > + > + if (cur_ioas) { > + iopt_remove_access(&cur_ioas->iopt, access); > + refcount_dec(&cur_ioas->obj.users); > + } This should match the physical side with an add/remove/replace API. Especially since remove is implicit in destroy this series only needs the add API And the locking shouldn't come in another patch that brings the replace/remove since with just split add we don't need it. That will make this patch alot smaller Jason
On Fri, Mar 10, 2023 at 01:36:22PM -0400, Jason Gunthorpe wrote: > On Wed, Mar 08, 2023 at 05:13:36AM -0800, Yi Liu wrote: > > > +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id) > > +{ > > + struct iommufd_ioas *new_ioas = NULL, *cur_ioas; > > + struct iommufd_ctx *ictx = access->ictx; > > + struct iommufd_object *obj; > > + int rc = 0; > > + > > + if (ioas_id) { > > + obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > > + if (IS_ERR(obj)) > > + return PTR_ERR(obj); > > + new_ioas = container_of(obj, struct iommufd_ioas, obj); > > + } > > + > > + mutex_lock(&access->ioas_lock); > > + cur_ioas = access->ioas; > > + if (cur_ioas == new_ioas) > > + goto out_unlock; > > + > > + if (new_ioas) { > > + rc = iopt_add_access(&new_ioas->iopt, access); > > + if (rc) > > + goto out_unlock; > > + iommufd_ref_to_users(obj); > > + } > > + > > + if (cur_ioas) { > > + iopt_remove_access(&cur_ioas->iopt, access); > > + refcount_dec(&cur_ioas->obj.users); > > + } > > This should match the physical side with an add/remove/replace > API. Especially since remove is implicit in destroy this series only > needs the add API I assume that the API would be iommufd_access_attach, iommufd_access_detach, and iommufd_access_replace(). And there might be an iommufd_access_change_pt(access, pt, bool replace)? > And the locking shouldn't come in another patch that brings the > replace/remove since with just split add we don't need it. Hmm. The iommufd_access_detach would be needed in the following cdev series, while the iommufd_access_replace would be need in my replace series. So, that would make the API be divided into three series. Perhaps we can have iommufd_access_attach/detach in this series along with a vfio_iommufd_emulated_detach_ioas, and the locking will come with another patch in replace series? Thanks Nic
On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Liu, Yi L <yi.l.liu@intel.com> > > Sent: Wednesday, March 8, 2023 9:14 PM > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx *ictx, > > u32 ioas_id, > > access->data = data; > > access->ops = ops; > > > > - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > > - if (IS_ERR(obj)) { > > - rc = PTR_ERR(obj); > > - goto out_abort; > > - } > > - access->ioas = container_of(obj, struct iommufd_ioas, obj); > > - iommufd_ref_to_users(obj); > > - > > if (ops->needs_pin_pages) > > access->iova_alignment = PAGE_SIZE; > > else > > access->iova_alignment = 1; > > - rc = iopt_add_access(&access->ioas->iopt, access); > > - if (rc) > > - goto out_put_ioas; > > > > /* The calling driver is a user until iommufd_access_destroy() */ > > refcount_inc(&access->obj.users); > > + mutex_init(&access->ioas_lock); > > access->ictx = ictx; > > iommufd_ctx_get(ictx); > > this refcnt get should be moved to the start given next patch > removes the reference in the caller side. I don't feel quite convincing to justify for moving it in this patch. Perhaps we should do that in the following patch, where it needs this? Or another individual patch moving this alone? Thanks Nic
Hi Jason/Kevin, On Tue, Mar 14, 2023 at 01:20:52AM -0700, Nicolin Chen wrote: > On Fri, Mar 10, 2023 at 01:36:22PM -0400, Jason Gunthorpe wrote: > > On Wed, Mar 08, 2023 at 05:13:36AM -0800, Yi Liu wrote: > > > > > +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id) > > > +{ > > > + struct iommufd_ioas *new_ioas = NULL, *cur_ioas; > > > + struct iommufd_ctx *ictx = access->ictx; > > > + struct iommufd_object *obj; > > > + int rc = 0; > > > + > > > + if (ioas_id) { > > > + obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > > > + if (IS_ERR(obj)) > > > + return PTR_ERR(obj); > > > + new_ioas = container_of(obj, struct iommufd_ioas, obj); > > > + } > > > + > > > + mutex_lock(&access->ioas_lock); > > > + cur_ioas = access->ioas; > > > + if (cur_ioas == new_ioas) > > > + goto out_unlock; > > > + > > > + if (new_ioas) { > > > + rc = iopt_add_access(&new_ioas->iopt, access); > > > + if (rc) > > > + goto out_unlock; > > > + iommufd_ref_to_users(obj); > > > + } > > > + > > > + if (cur_ioas) { > > > + iopt_remove_access(&cur_ioas->iopt, access); > > > + refcount_dec(&cur_ioas->obj.users); > > > + } > > > > This should match the physical side with an add/remove/replace > > API. Especially since remove is implicit in destroy this series only > > needs the add API > > I assume that the API would be iommufd_access_attach, > iommufd_access_detach, and iommufd_access_replace(). And there > might be an iommufd_access_change_pt(access, pt, bool replace)? > > > And the locking shouldn't come in another patch that brings the > > replace/remove since with just split add we don't need it. > > Hmm. The iommufd_access_detach would be needed in the following > cdev series, while the iommufd_access_replace would be need in > my replace series. So, that would make the API be divided into > three series. > > Perhaps we can have iommufd_access_attach/detach in this series > along with a vfio_iommufd_emulated_detach_ioas, and the locking > will come with another patch in replace series? I recall that we previously concluded that the unbind() is safe to go without doing access->ops->unmap, because close_device() would be called prior to the unbind(). But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev series, we'd need the access->ops->unmap call, and the locking and "ioas_unpin" too in the detach and attach APIs, right? I tried a bit of some update, across this series, cdev series, and the replace series. Though we might be able to simplify a bit of this patch/series, yet it doesn't seem to simplify the changes overall, particularly in the following cdev series for having an unmap() call and the locking. Then the replace API would mostly overlap with the attach API, by only having an additional detach(), which means actually we only need an iommufd_access_attach API to cover both cases... Can you please take a look at the final access APIs that I've attached at the end of the email for things mentioned above? Hopefully we can confirm and put them correctly into the next version of the three series. Thanks Nic ----------------------------------------------------------------------- static void __iommufd_access_detach(struct iommufd_access *access) { struct iommufd_ioas *cur_ioas = access->ioas; lockdep_assert_held(&access->ioas_lock); /* * Set ioas to NULL to block any further iommufd_access_pin_pages(). * iommufd_access_unpin_pages() can continue using access->ioas_unpin. */ access->ioas = NULL; if (access->ops->unmap) { mutex_unlock(&access->ioas_lock); access->ops->unmap(access->data, 0, ULONG_MAX); mutex_lock(&access->ioas_lock); } iopt_remove_access(&cur_ioas->iopt, access); refcount_dec(&cur_ioas->obj.users); } static int iommufd_access_change_pt(struct iommufd_access *access, u32 ioas_id) { struct iommufd_ioas *new_ioas, *cur_ioas; struct iommufd_object *obj; int rc = 0; obj = iommufd_get_object(access->ictx, ioas_id, IOMMUFD_OBJ_IOAS); if (IS_ERR(obj)) return PTR_ERR(obj); new_ioas = container_of(obj, struct iommufd_ioas, obj); mutex_lock(&access->ioas_lock); cur_ioas = access->ioas; if (cur_ioas == new_ioas) goto out_unlock; rc = iopt_add_access(&new_ioas->iopt, access); if (rc) goto out_unlock; iommufd_ref_to_users(obj); if (cur_ioas) __iommufd_access_detach(access); access->ioas_unpin = new_ioas; access->ioas = new_ioas; mutex_unlock(&access->ioas_lock); return 0; out_unlock: mutex_unlock(&access->ioas_lock); iommufd_put_object(obj); return rc; } int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id) { return iommufd_access_change_pt(access, ioas_id); } EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, IOMMUFD); /* Identical to iommufd_access_attach now... */ int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id) { return iommufd_access_change_pt(access, ioas_id); } EXPORT_SYMBOL_NS_GPL(iommufd_access_replace, IOMMUFD); void iommufd_access_detach(struct iommufd_access *access) { mutex_lock(&access->ioas_lock); if (WARN_ON(!access->ioas)) goto out; __iommufd_access_detach(access); out: access->ioas_unpin = NULL; mutex_unlock(&access->ioas_lock); } EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Wednesday, March 15, 2023 9:01 AM > > Hi Jason/Kevin, > > > > > Perhaps we can have iommufd_access_attach/detach in this series > > along with a vfio_iommufd_emulated_detach_ioas, and the locking > > will come with another patch in replace series? > > I recall that we previously concluded that the unbind() is safe > to go without doing access->ops->unmap, because close_device() > would be called prior to the unbind(). > > But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev > series, we'd need the access->ops->unmap call, and the locking > and "ioas_unpin" too in the detach and attach APIs, right? yes. We need locking since detach can happen any time with cdev while driver is conducting pinning. > > I tried a bit of some update, across this series, cdev series, > and the replace series. Though we might be able to simplify a > bit of this patch/series, yet it doesn't seem to simplify the > changes overall, particularly in the following cdev series for > having an unmap() call and the locking. > > Then the replace API would mostly overlap with the attach API, > by only having an additional detach(), which means actually we > only need an iommufd_access_attach API to cover both cases... there is a subtle difference. to match the physical path: for attach we expect the existing access->ioas is either NULL or same as the new ioas. for replace access->ioas must exist. they need different condition checks.
> From: Nicolin Chen > Sent: Wednesday, March 15, 2023 2:51 AM > > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote: > > External email: Use caution opening links or attachments > > > > > > > From: Liu, Yi L <yi.l.liu@intel.com> > > > Sent: Wednesday, March 8, 2023 9:14 PM > > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx > *ictx, > > > u32 ioas_id, > > > access->data = data; > > > access->ops = ops; > > > > > > - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > > > - if (IS_ERR(obj)) { > > > - rc = PTR_ERR(obj); > > > - goto out_abort; > > > - } > > > - access->ioas = container_of(obj, struct iommufd_ioas, obj); > > > - iommufd_ref_to_users(obj); > > > - > > > if (ops->needs_pin_pages) > > > access->iova_alignment = PAGE_SIZE; > > > else > > > access->iova_alignment = 1; > > > - rc = iopt_add_access(&access->ioas->iopt, access); > > > - if (rc) > > > - goto out_put_ioas; > > > > > > /* The calling driver is a user until iommufd_access_destroy() */ > > > refcount_inc(&access->obj.users); > > > + mutex_init(&access->ioas_lock); > > > access->ictx = ictx; > > > iommufd_ctx_get(ictx); > > > > this refcnt get should be moved to the start given next patch > > removes the reference in the caller side. > > I don't feel quite convincing to justify for moving it in this > patch. Perhaps we should do that in the following patch, where > it needs this? Or another individual patch moving this alone? > Next patch. I just tried to point out the required change caused by next patch.
On Wed, Mar 15, 2023 at 06:16:37AM +0000, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen > > Sent: Wednesday, March 15, 2023 2:51 AM > > > > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > > From: Liu, Yi L <yi.l.liu@intel.com> > > > > Sent: Wednesday, March 8, 2023 9:14 PM > > > > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx > > *ictx, > > > > u32 ioas_id, > > > > access->data = data; > > > > access->ops = ops; > > > > > > > > - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > > > > - if (IS_ERR(obj)) { > > > > - rc = PTR_ERR(obj); > > > > - goto out_abort; > > > > - } > > > > - access->ioas = container_of(obj, struct iommufd_ioas, obj); > > > > - iommufd_ref_to_users(obj); > > > > - > > > > if (ops->needs_pin_pages) > > > > access->iova_alignment = PAGE_SIZE; > > > > else > > > > access->iova_alignment = 1; > > > > - rc = iopt_add_access(&access->ioas->iopt, access); > > > > - if (rc) > > > > - goto out_put_ioas; > > > > > > > > /* The calling driver is a user until iommufd_access_destroy() */ > > > > refcount_inc(&access->obj.users); > > > > + mutex_init(&access->ioas_lock); > > > > access->ictx = ictx; > > > > iommufd_ctx_get(ictx); > > > > > > this refcnt get should be moved to the start given next patch > > > removes the reference in the caller side. > > > > I don't feel quite convincing to justify for moving it in this > > patch. Perhaps we should do that in the following patch, where > > it needs this? Or another individual patch moving this alone? > > > > Next patch. I just tried to point out the required change caused > by next patch.
On Wed, Mar 15, 2023 at 06:15:23AM +0000, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Wednesday, March 15, 2023 9:01 AM > > > > Hi Jason/Kevin, > > > > > > > > Perhaps we can have iommufd_access_attach/detach in this series > > > along with a vfio_iommufd_emulated_detach_ioas, and the locking > > > will come with another patch in replace series? > > > > I recall that we previously concluded that the unbind() is safe > > to go without doing access->ops->unmap, because close_device() > > would be called prior to the unbind(). > > > > But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev > > series, we'd need the access->ops->unmap call, and the locking > > and "ioas_unpin" too in the detach and attach APIs, right? > > yes. We need locking since detach can happen any time with > cdev while driver is conducting pinning. So, this preparatory series will add a pair of simple attach() and detach() APIs. Then the cdev series will add the locking and the ioas_unpin stuff as a rework of the detach() API. > > I tried a bit of some update, across this series, cdev series, > > and the replace series. Though we might be able to simplify a > > bit of this patch/series, yet it doesn't seem to simplify the > > changes overall, particularly in the following cdev series for > > having an unmap() call and the locking. > > > > Then the replace API would mostly overlap with the attach API, > > by only having an additional detach(), which means actually we > > only need an iommufd_access_attach API to cover both cases... > > there is a subtle difference. to match the physical path: > > for attach we expect the existing access->ioas is either NULL or > same as the new ioas. > > for replace access->ioas must exist. > > they need different condition checks. I think they can be something mingled... the sample code that I sent previously could take care of those conditions. But, I am also thinking a bit that maybe attach() does not need the locking? I can do a separate replace() function in this case. Thanks Nic
> From: Nicolin Chen > Sent: Wednesday, March 15, 2023 2:33 PM > > On Wed, Mar 15, 2023 at 06:15:23AM +0000, Tian, Kevin wrote: > > External email: Use caution opening links or attachments > > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > Sent: Wednesday, March 15, 2023 9:01 AM > > > > > > Hi Jason/Kevin, > > > > > > > > > > > Perhaps we can have iommufd_access_attach/detach in this series > > > > along with a vfio_iommufd_emulated_detach_ioas, and the locking > > > > will come with another patch in replace series? > > > > > > I recall that we previously concluded that the unbind() is safe > > > to go without doing access->ops->unmap, because close_device() > > > would be called prior to the unbind(). > > > > > > But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev > > > series, we'd need the access->ops->unmap call, and the locking > > > and "ioas_unpin" too in the detach and attach APIs, right? > > > > yes. We need locking since detach can happen any time with > > cdev while driver is conducting pinning. > > So, this preparatory series will add a pair of simple attach() > and detach() APIs. Then the cdev series will add the locking > and the ioas_unpin stuff as a rework of the detach() API. > > > > I tried a bit of some update, across this series, cdev series, > > > and the replace series. Though we might be able to simplify a > > > bit of this patch/series, yet it doesn't seem to simplify the > > > changes overall, particularly in the following cdev series for > > > having an unmap() call and the locking. > > > > > > Then the replace API would mostly overlap with the attach API, > > > by only having an additional detach(), which means actually we > > > only need an iommufd_access_attach API to cover both cases... > > > > there is a subtle difference. to match the physical path: > > > > for attach we expect the existing access->ioas is either NULL or > > same as the new ioas. > > > > for replace access->ioas must exist. > > > > they need different condition checks. > > I think they can be something mingled... the sample code that > I sent previously could take care of those conditions. But, I > am also thinking a bit that maybe attach() does not need the > locking? I can do a separate replace() function in this case. > w/o locking then you need smp_store_release() and its pair. anyway it's not in perf critical path. Keeping lock for attach is simpler and safe.
> From: Nicolin Chen > Sent: Wednesday, March 15, 2023 2:22 PM > > On Wed, Mar 15, 2023 at 06:16:37AM +0000, Tian, Kevin wrote: > > External email: Use caution opening links or attachments > > > > > > > From: Nicolin Chen > > > Sent: Wednesday, March 15, 2023 2:51 AM > > > > > > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote: > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > > From: Liu, Yi L <yi.l.liu@intel.com> > > > > > Sent: Wednesday, March 8, 2023 9:14 PM > > > > > > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct > iommufd_ctx > > > *ictx, > > > > > u32 ioas_id, > > > > > access->data = data; > > > > > access->ops = ops; > > > > > > > > > > - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > > > > > - if (IS_ERR(obj)) { > > > > > - rc = PTR_ERR(obj); > > > > > - goto out_abort; > > > > > - } > > > > > - access->ioas = container_of(obj, struct iommufd_ioas, obj); > > > > > - iommufd_ref_to_users(obj); > > > > > - > > > > > if (ops->needs_pin_pages) > > > > > access->iova_alignment = PAGE_SIZE; > > > > > else > > > > > access->iova_alignment = 1; > > > > > - rc = iopt_add_access(&access->ioas->iopt, access); > > > > > - if (rc) > > > > > - goto out_put_ioas; > > > > > > > > > > /* The calling driver is a user until iommufd_access_destroy() */ > > > > > refcount_inc(&access->obj.users); > > > > > + mutex_init(&access->ioas_lock); > > > > > access->ictx = ictx; > > > > > iommufd_ctx_get(ictx); > > > > > > > > this refcnt get should be moved to the start given next patch > > > > removes the reference in the caller side. > > > > > > I don't feel quite convincing to justify for moving it in this > > > patch. Perhaps we should do that in the following patch, where > > > it needs this? Or another individual patch moving this alone? > > > > > > > Next patch. I just tried to point out the required change caused > > by next patch.
> From: Tian, Kevin <kevin.tian@intel.com> > Sent: Wednesday, March 15, 2023 2:52 PM > > > From: Nicolin Chen > > Sent: Wednesday, March 15, 2023 2:22 PM > > > > On Wed, Mar 15, 2023 at 06:16:37AM +0000, Tian, Kevin wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > > From: Nicolin Chen > > > > Sent: Wednesday, March 15, 2023 2:51 AM > > > > > > > > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote: > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > > > > > From: Liu, Yi L <yi.l.liu@intel.com> > > > > > > Sent: Wednesday, March 8, 2023 9:14 PM > > > > > > > > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct > > iommufd_ctx > > > > *ictx, > > > > > > u32 ioas_id, > > > > > > access->data = data; > > > > > > access->ops = ops; > > > > > > > > > > > > - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > > > > > > - if (IS_ERR(obj)) { > > > > > > - rc = PTR_ERR(obj); > > > > > > - goto out_abort; > > > > > > - } > > > > > > - access->ioas = container_of(obj, struct iommufd_ioas, obj); > > > > > > - iommufd_ref_to_users(obj); > > > > > > - > > > > > > if (ops->needs_pin_pages) > > > > > > access->iova_alignment = PAGE_SIZE; > > > > > > else > > > > > > access->iova_alignment = 1; > > > > > > - rc = iopt_add_access(&access->ioas->iopt, access); > > > > > > - if (rc) > > > > > > - goto out_put_ioas; > > > > > > > > > > > > /* The calling driver is a user until iommufd_access_destroy() */ > > > > > > refcount_inc(&access->obj.users); > > > > > > + mutex_init(&access->ioas_lock); > > > > > > access->ictx = ictx; > > > > > > iommufd_ctx_get(ictx); > > > > > > > > > > this refcnt get should be moved to the start given next patch > > > > > removes the reference in the caller side. This change is ok but seems not necessary. Yes, vfio_iommufd_emulated_bind() will not have reference on the ictx after the next patch. However, it gets reference only because it wants to store it in vfio_device. Now, it does not store it. So no get. I think the caller of vfio_iommufd_emulated_bind() should ensure the ictx is valid. Also check the physical device bind. So maybe not necessary to get ictx before calling iommufd_access_create(). This is the same with the vfio_iommufd_physical_bind() which calls iommufd_device_bind() without ictx get, and iommufd_device_bind() also gets ictx in the end. If it's really necessary, maybe let the vfio_iommufd_physical_bind() and vfio_iommufd_emulated_bind() get/put ictx around calling iommufd_access_create()/iommufd_device_bind(). > > > > > > > > I don't feel quite convincing to justify for moving it in this > > > > patch. Perhaps we should do that in the following patch, where > > > > it needs this? Or another individual patch moving this alone? > > > > > > > > > > Next patch. I just tried to point out the required change caused > > > by next patch.
Hi, On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote: > > So, this preparatory series will add a pair of simple attach() > > and detach() APIs. Then the cdev series will add the locking > > and the ioas_unpin stuff as a rework of the detach() API. > > I think they can be something mingled... the sample code that > > I sent previously could take care of those conditions. But, I > > am also thinking a bit that maybe attach() does not need the > > locking? I can do a separate replace() function in this case. > > > > w/o locking then you need smp_store_release() and its pair. > > anyway it's not in perf critical path. Keeping lock for attach > is simpler and safe. OK. Basically I followed what Jason suggested by having three APIs and combined Kevin's inputs about the difference between the attach/replace(). I also updated the replace changes, and rebased all nesting (infrastructure, VT-d and SMMU): https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023 The major three changes for those APIs: [1] This adds iommufd_access_attach() in this series: "iommufd: Create access in vfio_iommufd_emulated_bind()" https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcca5ceaeb40e22b5 [2] This adds iommufd_access_detach() in the cdev series: "iommufd/device: Add iommufd_access_detach() API" https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a097e2c9d9e26af4 [3] This adds iommufd_access_replace() in the replace series: "iommufd: Add iommufd_access_replace() API" https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9bc2bf319b7654a8 Please check if they look okay, so that Yi can integrate them accordingly to the emulated/cdev series. [*] This is the patch that I posted in the other mail addressing Kevin's comments on iommufd_ctx_get(): "iommufd/device: Do iommufd_ctx_get() at the top of iommufd_access_create()" https://github.com/nicolinc/iommufd/commit/077b09bb83329dc046753f4ef672f5bf6386755c (I just saw Yi's reply concerning its necessity. Feel free to drop in that case.) Thanks Nicolin P.S. Attaching the list of changes with their locations: 3791dedf98e8 cover-letter: Add IO page table replacement support c8ebf51c3c9b vfio: Support IO page table replacement c5710f23e8f6 iommufd/selftest: Add IOMMU_TEST_OP_ACCESS_REPLACE_IOAS coverage [3] 36507fa9f0f4 iommufd: Add iommufd_access_replace() API 0263855d1e8b vfio: Do not allow !ops->dma_unmap in vfio_pin/unpin_pages() e39ed55e77a0 cover-letter: Add vfio_device cdev for iommufd support 26fd7fccaef3 docs: vfio: Add vfio device cdev description f10f3e3162bb vfio: Compile group optionally 9588ae4c4049 vfio: Add VFIO_DEVICE_AT[DE]TACH_IOMMUFD_PT 3e57108eac64 vfio: Add VFIO_DEVICE_BIND_IOMMUFD b925716dd226 vfio: Add cdev for vfio_device db309463ab92 vfio-iommufd: Add detach_ioas support for emulated VFIO devices [2] 4110522146ca iommufd/device: Add iommufd_access_detach() API abca7e1e063a vfio-iommufd: Add detach_ioas support for physical VFIO devices 9d368f7247c7 vfio: Record devid in vfio_device_file 683af0a471e1 vfio-iommufd: Split the compat_ioas attach out from vfio_iommufd_bind() 32a2e7de1d53 vfio-iommufd: Split the no-iommu support out from vfio_iommufd_bind() 8a1c042379f5 vfio: Make vfio_device_first_open() to accept NULL iommufd for noiommu fc6e0ed2aa44 vfio: Make vfio_device_open() single open for device cdev path 3f6821d507a4 vfio: Add cdev_device_open_cnt to vfio_group 896cde40a016 vfio: Block device access via device fd until device is opened f422c4216a19 vfio: Pass struct vfio_device_file * to vfio_device_open/close() b187f9980fed kvm/vfio: Accept vfio device file from userspace 721e2e60ff54 kvm/vfio: Rename kvm_vfio_group to prepare for accepting vfio device fd 8993c4c75c20 vfio: Accept vfio device file in the KVM facing kAPI a92c45ae0ce6 vfio: Remove vfio_file_is_group() fb586f783934 vfio: Refine vfio file kAPIs for KVM 50694af6f3c0 vfio: Allocate per device file structure df21c0737eef cover-letter: Make vfio-pci hot reset prepared for vfio device cdev 5c25c874d7e0 vfio/pci: Accept device fd in VFIO_DEVICE_PCI_HOT_RESET ioctl 7c30ce8b54db vfio: Accpet device file from vfio PCI hot reset path e3209342db44 vfio: Refine vfio file kAPIs for vfio PCI hot reset 8354fd79944e vfio/pci: Rename the helpers and data in hot reset path to accept device fd 54387efb858c vfio/pci: Allow passing zero-length fd array in VFIO_DEVICE_PCI_HOT_RESET cd93ffb62c51 vfio/pci: Only need to check opened devices in the dev_set for hot reset 2a6fd7231cbf vfio/pci: Update comment around group_fd get in vfio_pci_ioctl_pci_hot_reset() 480abea5961e cover-letter: vfio: Make emulated devices prepared for vfio device cdev 46b6d1ae1754 vfio: Check the presence for iommufd callbacks in __vfio_register_dev() 6064b9f81817 Samples/mdev: Uses the vfio emulated iommufd ops set in the mdev sample drivers c20852af7291 vfio-iommufd: Make vfio_iommufd_emulated_bind() return iommufd_access ID 3405865b0b3f vfio-iommufd: No need to record iommufd_ctx in vfio_device [*] 077b09bb8332 iommufd/device: Do iommufd_ctx_get() at the top of iommufd_access_create() [1] 34fba7509429 iommufd: Create access in vfio_iommufd_emulated_bind() a5d8ac47554f docs: kvm: vfio: Require call KVM_DEV_VFIO_GROUP_ADD before VFIO_GROUP_GET_DEVICE_FD
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Wednesday, March 15, 2023 5:03 PM > > Hi, > > On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote: > > > > So, this preparatory series will add a pair of simple attach() > > > and detach() APIs. Then the cdev series will add the locking > > > and the ioas_unpin stuff as a rework of the detach() API. > > > > I think they can be something mingled... the sample code that > > > I sent previously could take care of those conditions. But, I > > > am also thinking a bit that maybe attach() does not need the > > > locking? I can do a separate replace() function in this case. > > > > > > > w/o locking then you need smp_store_release() and its pair. > > > > anyway it's not in perf critical path. Keeping lock for attach > > is simpler and safe. > > OK. Basically I followed what Jason suggested by having three > APIs and combined Kevin's inputs about the difference between > the attach/replace(). I also updated the replace changes, and > rebased all nesting (infrastructure, VT-d and SMMU): > https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023 > > The major three changes for those APIs: > [1] This adds iommufd_access_attach() in this series: > "iommufd: Create access in vfio_iommufd_emulated_bind()" > > https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dc > ca5ceaeb40e22b5 > [2] This adds iommufd_access_detach() in the cdev series: > "iommufd/device: Add iommufd_access_detach() API" > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > 097e2c9d9e26af4 > [3] This adds iommufd_access_replace() in the replace series: > "iommufd: Add iommufd_access_replace() API" > > https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9 > bc2bf319b7654a8 > > Please check if they look okay, so that Yi can integrate them > accordingly to the emulated/cdev series. Thanks. I'll start to integrate after ack from Kevin or Jason. btw. Below is my latest code (rebased on top of rc-2).
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Wednesday, March 15, 2023 4:53 PM > > > From: Tian, Kevin <kevin.tian@intel.com> > > Sent: Wednesday, March 15, 2023 2:52 PM > > > > > From: Nicolin Chen > > > Sent: Wednesday, March 15, 2023 2:22 PM > > > > > > On Wed, Mar 15, 2023 at 06:16:37AM +0000, Tian, Kevin wrote: > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > > From: Nicolin Chen > > > > > Sent: Wednesday, March 15, 2023 2:51 AM > > > > > > > > > > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote: > > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > > > > > > > > From: Liu, Yi L <yi.l.liu@intel.com> > > > > > > > Sent: Wednesday, March 8, 2023 9:14 PM > > > > > > > > > > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct > > > iommufd_ctx > > > > > *ictx, > > > > > > > u32 ioas_id, > > > > > > > access->data = data; > > > > > > > access->ops = ops; > > > > > > > > > > > > > > - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); > > > > > > > - if (IS_ERR(obj)) { > > > > > > > - rc = PTR_ERR(obj); > > > > > > > - goto out_abort; > > > > > > > - } > > > > > > > - access->ioas = container_of(obj, struct iommufd_ioas, obj); > > > > > > > - iommufd_ref_to_users(obj); > > > > > > > - > > > > > > > if (ops->needs_pin_pages) > > > > > > > access->iova_alignment = PAGE_SIZE; > > > > > > > else > > > > > > > access->iova_alignment = 1; > > > > > > > - rc = iopt_add_access(&access->ioas->iopt, access); > > > > > > > - if (rc) > > > > > > > - goto out_put_ioas; > > > > > > > > > > > > > > /* The calling driver is a user until iommufd_access_destroy() > */ > > > > > > > refcount_inc(&access->obj.users); > > > > > > > + mutex_init(&access->ioas_lock); > > > > > > > access->ictx = ictx; > > > > > > > iommufd_ctx_get(ictx); > > > > > > > > > > > > this refcnt get should be moved to the start given next patch > > > > > > removes the reference in the caller side. > > This change is ok but seems not necessary. > > Yes, vfio_iommufd_emulated_bind() will not have reference on the > ictx after the next patch. However, it gets reference only because it > wants to store it in vfio_device. Now, it does not store it. So no get. > I think the caller of vfio_iommufd_emulated_bind() should ensure > the ictx is valid. Also check the physical device bind. So maybe not > necessary to get ictx before calling iommufd_access_create(). This is > the same with the vfio_iommufd_physical_bind() which calls > iommufd_device_bind() without ictx get, and iommufd_device_bind() > also gets ictx in the end. > You are right. I overlooked the fact that ictx is already held by the caller of bind.
On Thu, Mar 16, 2023 at 12:17:11AM +0000, Tian, Kevin wrote: > > > > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct > > > > iommufd_ctx > > > > > > > > refcount_inc(&access->obj.users); > > > > > > > > + mutex_init(&access->ioas_lock); > > > > > > > > access->ictx = ictx; > > > > > > > > iommufd_ctx_get(ictx); > > > > > > > > > > > > > > this refcnt get should be moved to the start given next patch > > > > > > > removes the reference in the caller side. > > > > This change is ok but seems not necessary. > > > > Yes, vfio_iommufd_emulated_bind() will not have reference on the > > ictx after the next patch. However, it gets reference only because it > > wants to store it in vfio_device. Now, it does not store it. So no get. > > I think the caller of vfio_iommufd_emulated_bind() should ensure > > the ictx is valid. Also check the physical device bind. So maybe not > > necessary to get ictx before calling iommufd_access_create(). This is > > the same with the vfio_iommufd_physical_bind() which calls > > iommufd_device_bind() without ictx get, and iommufd_device_bind() > > also gets ictx in the end. > > > > You are right. I overlooked the fact that ictx is already held by the > caller of bind. I am dropping it then :) Nic
On Wed, Mar 15, 2023 at 12:18:01PM +0000, Liu, Yi L wrote: > > OK. Basically I followed what Jason suggested by having three > > APIs and combined Kevin's inputs about the difference between > > the attach/replace(). I also updated the replace changes, and > > rebased all nesting (infrastructure, VT-d and SMMU): > > https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023 > > > > The major three changes for those APIs: > > [1] This adds iommufd_access_attach() in this series: > > "iommufd: Create access in vfio_iommufd_emulated_bind()" > > > > https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dc > > ca5ceaeb40e22b5 > > [2] This adds iommufd_access_detach() in the cdev series: > > "iommufd/device: Add iommufd_access_detach() API" > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > 097e2c9d9e26af4 > > [3] This adds iommufd_access_replace() in the replace series: > > "iommufd: Add iommufd_access_replace() API" > > > > https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9 > > bc2bf319b7654a8 > > > > Please check if they look okay, so that Yi can integrate them > > accordingly to the emulated/cdev series. > > Thanks. I'll start to integrate after ack from Kevin or Jason. btw. > Below is my latest code (rebased on top of rc-2).
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Wednesday, March 15, 2023 5:03 PM > > Hi, > > On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote: > > > > So, this preparatory series will add a pair of simple attach() > > > and detach() APIs. Then the cdev series will add the locking > > > and the ioas_unpin stuff as a rework of the detach() API. > > > > I think they can be something mingled... the sample code that > > > I sent previously could take care of those conditions. But, I > > > am also thinking a bit that maybe attach() does not need the > > > locking? I can do a separate replace() function in this case. > > > > > > > w/o locking then you need smp_store_release() and its pair. > > > > anyway it's not in perf critical path. Keeping lock for attach > > is simpler and safe. > > OK. Basically I followed what Jason suggested by having three > APIs and combined Kevin's inputs about the difference between > the attach/replace(). I also updated the replace changes, and > rebased all nesting (infrastructure, VT-d and SMMU): > https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting- > 03142023 > > The major three changes for those APIs: > [1] This adds iommufd_access_attach() in this series: > "iommufd: Create access in vfio_iommufd_emulated_bind()" > > https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcc > a5ceaeb40e22b5 WARN_ON(!vdev->iommufd_access) in vfio_iommufd_emulated_attach_ioas() > [2] This adds iommufd_access_detach() in the cdev series: > "iommufd/device: Add iommufd_access_detach() API" > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > 097e2c9d9e26af4 also add a check if old_ioas exists it must equal to the new_ioas in attach. > [3] This adds iommufd_access_replace() in the replace series: > "iommufd: Add iommufd_access_replace() API" > > https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9b > c2bf319b7654a8 lockdep_assert_held(&access->ioas_lock) in iommufd_access_change_pt() cur_ioas is uninitialized in iommufd_access_change_pt(). Actually we don't need an extra variable given only one reference to access->ioas. similarly directly refer to access->ioas in iommufd_access_replace(). > > Please check if they look okay, so that Yi can integrate them > accordingly to the emulated/cdev series. this split looks reasonable to me. Go ahead.
On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote: > > Please check if they look okay, so that Yi can integrate them > > accordingly to the emulated/cdev series. > > this split looks reasonable to me. Go ahead. Thanks for the review! I will address those comments and renew those commits by the end of the day. Thanks Nic
Hi Kevin, I've fixed the other two commits. Here is the one that I am not sure about: On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote: > > [2] This adds iommufd_access_detach() in the cdev series: > > "iommufd/device: Add iommufd_access_detach() API" > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > 097e2c9d9e26af4 > > also add a check if old_ioas exists it must equal to the new_ioas in attach. This is the commit adding detach(). And there's a check in it: if (WARN_ON(!access->ioas)) Do you mean having an "if (access->ioas) return -EBUSY;" line in the commit adding attach()? And, how should we check in the detach() if it equals to the new_ioas in attach? Isn't the WARN_ON(!access->ioas) enough? Thanks Nic
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Thursday, March 16, 2023 1:33 PM > > Hi Kevin, > > I've fixed the other two commits. Here is the one that I am > not sure about: > > On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote: > > > > [2] This adds iommufd_access_detach() in the cdev series: > > > "iommufd/device: Add iommufd_access_detach() API" > > > > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > > 097e2c9d9e26af4 > > > > also add a check if old_ioas exists it must equal to the new_ioas in attach. > > This is the commit adding detach(). And there's a check in it: > if (WARN_ON(!access->ioas)) > > Do you mean having an "if (access->ioas) return -EBUSY;" line > in the commit adding attach()? if (access->ioas && access->ioas != new_ioas) return -EBUSY; yes this is for attach. > > And, how should we check in the detach() if it equals to the > new_ioas in attach? Isn't the WARN_ON(!access->ioas) enough? > this is not required.
On Thu, Mar 16, 2023 at 05:38:41AM +0000, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Thursday, March 16, 2023 1:33 PM > > > > Hi Kevin, > > > > I've fixed the other two commits. Here is the one that I am > > not sure about: > > > > On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote: > > > > > > [2] This adds iommufd_access_detach() in the cdev series: > > > > "iommufd/device: Add iommufd_access_detach() API" > > > > > > > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > > > 097e2c9d9e26af4 > > > > > > also add a check if old_ioas exists it must equal to the new_ioas in attach. > > > > This is the commit adding detach(). And there's a check in it: > > if (WARN_ON(!access->ioas)) > > > > Do you mean having an "if (access->ioas) return -EBUSY;" line > > in the commit adding attach()? > > if (access->ioas && access->ioas != new_ioas) > return -EBUSY; > > yes this is for attach. OK. For attach(), the access->ioas shouldn't be !NULL, I think. At the point of adding attach(), the uAPI doesn't support the replacement use case yet. And later we have a separate API for that. So I think it'd be just: if (access->ioas) return -EBUSY; The reason why I didn't add it is actually because the caller vfio_iommufd_emulated_attach_ioas() has a check of "attached" already. Yet, it doesn't hurt to have one more in the API. Thanks Nic
> From: Nicolin Chen > Sent: Thursday, March 16, 2023 1:44 PM > > On Thu, Mar 16, 2023 at 05:38:41AM +0000, Tian, Kevin wrote: > > External email: Use caution opening links or attachments > > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > Sent: Thursday, March 16, 2023 1:33 PM > > > > > > Hi Kevin, > > > > > > I've fixed the other two commits. Here is the one that I am > > > not sure about: > > > > > > On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote: > > > > > > > > [2] This adds iommufd_access_detach() in the cdev series: > > > > > "iommufd/device: Add iommufd_access_detach() API" > > > > > > > > > > > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > > > > 097e2c9d9e26af4 > > > > > > > > also add a check if old_ioas exists it must equal to the new_ioas in > attach. > > > > > > This is the commit adding detach(). And there's a check in it: > > > if (WARN_ON(!access->ioas)) > > > > > > Do you mean having an "if (access->ioas) return -EBUSY;" line > > > in the commit adding attach()? > > > > if (access->ioas && access->ioas != new_ioas) > > return -EBUSY; > > > > yes this is for attach. > > OK. For attach(), the access->ioas shouldn't be !NULL, I think. > At the point of adding attach(), the uAPI doesn't support the > replacement use case yet. And later we have a separate API for > that. what about user calling attach twice in cdev? > > So I think it'd be just: > if (access->ioas) > return -EBUSY; > > The reason why I didn't add it is actually because the caller > vfio_iommufd_emulated_attach_ioas() has a check of "attached" > already. Yet, it doesn't hurt to have one more in the API. > but here the slight difference is that in physical path we allow attach twice to the same hwpt. they should be consistent: if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt) return -EINVAL;
On Thu, Mar 16, 2023 at 05:49:20AM +0000, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen > > Sent: Thursday, March 16, 2023 1:44 PM > > > > On Thu, Mar 16, 2023 at 05:38:41AM +0000, Tian, Kevin wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > Sent: Thursday, March 16, 2023 1:33 PM > > > > > > > > Hi Kevin, > > > > > > > > I've fixed the other two commits. Here is the one that I am > > > > not sure about: > > > > > > > > On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote: > > > > > > > > > > [2] This adds iommufd_access_detach() in the cdev series: > > > > > > "iommufd/device: Add iommufd_access_detach() API" > > > > > > > > > > > > > > > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > > > > > 097e2c9d9e26af4 > > > > > > > > > > also add a check if old_ioas exists it must equal to the new_ioas in > > attach. > > > > > > > > This is the commit adding detach(). And there's a check in it: > > > > if (WARN_ON(!access->ioas)) > > > > > > > > Do you mean having an "if (access->ioas) return -EBUSY;" line > > > > in the commit adding attach()? > > > > > > if (access->ioas && access->ioas != new_ioas) > > > return -EBUSY; > > > > > > yes this is for attach. > > > > OK. For attach(), the access->ioas shouldn't be !NULL, I think. > > At the point of adding attach(), the uAPI doesn't support the > > replacement use case yet. And later we have a separate API for > > that. > > what about user calling attach twice in cdev? > > > > > So I think it'd be just: > > if (access->ioas) > > return -EBUSY; > > > > The reason why I didn't add it is actually because the caller > > vfio_iommufd_emulated_attach_ioas() has a check of "attached" > > already. Yet, it doesn't hurt to have one more in the API. > > > > but here the slight difference is that in physical path we allow > attach twice to the same hwpt. they should be consistent: > > if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt) > return -EINVAL; I see. The point is to support duplicated calls: ATTACH (pt_id = ioas1) ATTACH (pt_id = ioas1) Then I will add this to keep the consistency: if (access->ioas != NULL && access->ioas != new_ioas) return -EINVAL; Thanks Nic
> From: Tian, Kevin <kevin.tian@intel.com> > Sent: Thursday, March 16, 2023 1:49 PM > > > From: Nicolin Chen > > Sent: Thursday, March 16, 2023 1:44 PM > > > > On Thu, Mar 16, 2023 at 05:38:41AM +0000, Tian, Kevin wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > Sent: Thursday, March 16, 2023 1:33 PM > > > > > > > > Hi Kevin, > > > > > > > > I've fixed the other two commits. Here is the one that I am > > > > not sure about: > > > > > > > > On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote: > > > > > > > > > > [2] This adds iommufd_access_detach() in the cdev series: > > > > > > "iommufd/device: Add iommufd_access_detach() API" > > > > > > > > > > > > > > > > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > > > > > 097e2c9d9e26af4 > > > > > > > > > > also add a check if old_ioas exists it must equal to the new_ioas in > > attach. > > > > > > > > This is the commit adding detach(). And there's a check in it: > > > > if (WARN_ON(!access->ioas)) > > > > > > > > Do you mean having an "if (access->ioas) return -EBUSY;" line > > > > in the commit adding attach()? > > > > > > if (access->ioas && access->ioas != new_ioas) > > > return -EBUSY; > > > > > > yes this is for attach. > > > > OK. For attach(), the access->ioas shouldn't be !NULL, I think. > > At the point of adding attach(), the uAPI doesn't support the > > replacement use case yet. And later we have a separate API for > > that. > > what about user calling attach twice in cdev? In the cdev series, VFIO only one attach is allowed so far. If vfio_device->iommufd_attached is set, would return -EBUSY to user. But this does not prevent the iommufd side to allow attach twice. Nic's replace series would relax it if user means to replace existing attached hwpt/ioas. Regards, Yi Liu
On Wed, Mar 15, 2023 at 02:03:09AM -0700, Nicolin Chen wrote: > Hi, > > On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote: > > > > So, this preparatory series will add a pair of simple attach() > > > and detach() APIs. Then the cdev series will add the locking > > > and the ioas_unpin stuff as a rework of the detach() API. > > > > I think they can be something mingled... the sample code that > > > I sent previously could take care of those conditions. But, I > > > am also thinking a bit that maybe attach() does not need the > > > locking? I can do a separate replace() function in this case. > > > > > > > w/o locking then you need smp_store_release() and its pair. > > > > anyway it's not in perf critical path. Keeping lock for attach > > is simpler and safe. > > OK. Basically I followed what Jason suggested by having three > APIs and combined Kevin's inputs about the difference between > the attach/replace(). I also updated the replace changes, and > rebased all nesting (infrastructure, VT-d and SMMU): > https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023 > > The major three changes for those APIs: > [1] This adds iommufd_access_attach() in this series: > "iommufd: Create access in vfio_iommufd_emulated_bind()" > https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcca5ceaeb40e22b5 > [2] This adds iommufd_access_detach() in the cdev series: > "iommufd/device: Add iommufd_access_detach() API" > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a097e2c9d9e26af4 > [3] This adds iommufd_access_replace() in the replace series: > "iommufd: Add iommufd_access_replace() API" > https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9bc2bf319b7654a8 > > Please check if they look okay, so that Yi can integrate them > accordingly to the emulated/cdev series. I don't understand why this is being put in front of the cdev series? Jason
On 2023/3/20 22:49, Jason Gunthorpe wrote: > On Wed, Mar 15, 2023 at 02:03:09AM -0700, Nicolin Chen wrote: >> Hi, >> >> On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote: >> >>>> So, this preparatory series will add a pair of simple attach() >>>> and detach() APIs. Then the cdev series will add the locking >>>> and the ioas_unpin stuff as a rework of the detach() API. >> >>>> I think they can be something mingled... the sample code that >>>> I sent previously could take care of those conditions. But, I >>>> am also thinking a bit that maybe attach() does not need the >>>> locking? I can do a separate replace() function in this case. >>>> >>> >>> w/o locking then you need smp_store_release() and its pair. >>> >>> anyway it's not in perf critical path. Keeping lock for attach >>> is simpler and safe. >> >> OK. Basically I followed what Jason suggested by having three >> APIs and combined Kevin's inputs about the difference between >> the attach/replace(). I also updated the replace changes, and >> rebased all nesting (infrastructure, VT-d and SMMU): >> https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023 >> >> The major three changes for those APIs: >> [1] This adds iommufd_access_attach() in this series: >> "iommufd: Create access in vfio_iommufd_emulated_bind()" >> https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcca5ceaeb40e22b5 >> [2] This adds iommufd_access_detach() in the cdev series: >> "iommufd/device: Add iommufd_access_detach() API" >> https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a097e2c9d9e26af4 >> [3] This adds iommufd_access_replace() in the replace series: >> "iommufd: Add iommufd_access_replace() API" >> https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9bc2bf319b7654a8 >> >> Please check if they look okay, so that Yi can integrate them >> accordingly to the emulated/cdev series. > > I don't understand why this is being put in front of the cdev series? because we want to make emulated devices have iommufd_access in the bind, then it can return iommufd_access->obj.id to userspace when adding cdev.
On Mon, Mar 20, 2023 at 11:11:51PM +0800, Yi Liu wrote: > > > On 2023/3/20 22:49, Jason Gunthorpe wrote: > > On Wed, Mar 15, 2023 at 02:03:09AM -0700, Nicolin Chen wrote: > > > Hi, > > > > > > On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote: > > > > > > > > So, this preparatory series will add a pair of simple attach() > > > > > and detach() APIs. Then the cdev series will add the locking > > > > > and the ioas_unpin stuff as a rework of the detach() API. > > > > > > > > I think they can be something mingled... the sample code that > > > > > I sent previously could take care of those conditions. But, I > > > > > am also thinking a bit that maybe attach() does not need the > > > > > locking? I can do a separate replace() function in this case. > > > > > > > > > > > > > w/o locking then you need smp_store_release() and its pair. > > > > > > > > anyway it's not in perf critical path. Keeping lock for attach > > > > is simpler and safe. > > > > > > OK. Basically I followed what Jason suggested by having three > > > APIs and combined Kevin's inputs about the difference between > > > the attach/replace(). I also updated the replace changes, and > > > rebased all nesting (infrastructure, VT-d and SMMU): > > > https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023 > > > > > > The major three changes for those APIs: > > > [1] This adds iommufd_access_attach() in this series: > > > "iommufd: Create access in vfio_iommufd_emulated_bind()" > > > https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcca5ceaeb40e22b5 > > > [2] This adds iommufd_access_detach() in the cdev series: > > > "iommufd/device: Add iommufd_access_detach() API" > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a097e2c9d9e26af4 > > > [3] This adds iommufd_access_replace() in the replace series: > > > "iommufd: Add iommufd_access_replace() API" > > > https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9bc2bf319b7654a8 > > > > > > Please check if they look okay, so that Yi can integrate them > > > accordingly to the emulated/cdev series. > > > > I don't understand why this is being put in front of the cdev series? > > because we want to make emulated devices have iommufd_access in the > bind, then it can return iommufd_access->obj.id to userspace when > adding cdev. Ah OK Jason
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index a0c66f47a65a..71c4d38994b3 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -412,9 +412,12 @@ void iommufd_access_destroy_object(struct iommufd_object *obj) struct iommufd_access *access = container_of(obj, struct iommufd_access, obj); - iopt_remove_access(&access->ioas->iopt, access); + if (access->ioas) { + iopt_remove_access(&access->ioas->iopt, access); + refcount_dec(&access->ioas->obj.users); + } iommufd_ctx_put(access->ictx); - refcount_dec(&access->ioas->obj.users); + mutex_destroy(&access->ioas_lock); } /** @@ -431,12 +434,10 @@ void iommufd_access_destroy_object(struct iommufd_object *obj) * The provided ops are required to use iommufd_access_pin_pages(). */ struct iommufd_access * -iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id, +iommufd_access_create(struct iommufd_ctx *ictx, const struct iommufd_access_ops *ops, void *data) { struct iommufd_access *access; - struct iommufd_object *obj; - int rc; /* * There is no uAPI for the access object, but to keep things symmetric @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id, access->data = data; access->ops = ops; - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); - if (IS_ERR(obj)) { - rc = PTR_ERR(obj); - goto out_abort; - } - access->ioas = container_of(obj, struct iommufd_ioas, obj); - iommufd_ref_to_users(obj); - if (ops->needs_pin_pages) access->iova_alignment = PAGE_SIZE; else access->iova_alignment = 1; - rc = iopt_add_access(&access->ioas->iopt, access); - if (rc) - goto out_put_ioas; /* The calling driver is a user until iommufd_access_destroy() */ refcount_inc(&access->obj.users); + mutex_init(&access->ioas_lock); access->ictx = ictx; iommufd_ctx_get(ictx); iommufd_object_finalize(ictx, &access->obj); return access; -out_put_ioas: - refcount_dec(&access->ioas->obj.users); -out_abort: - iommufd_object_abort(ictx, &access->obj); - return ERR_PTR(rc); } EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD); @@ -494,6 +480,50 @@ void iommufd_access_destroy(struct iommufd_access *access) } EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD); +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id) +{ + struct iommufd_ioas *new_ioas = NULL, *cur_ioas; + struct iommufd_ctx *ictx = access->ictx; + struct iommufd_object *obj; + int rc = 0; + + if (ioas_id) { + obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS); + if (IS_ERR(obj)) + return PTR_ERR(obj); + new_ioas = container_of(obj, struct iommufd_ioas, obj); + } + + mutex_lock(&access->ioas_lock); + cur_ioas = access->ioas; + if (cur_ioas == new_ioas) + goto out_unlock; + + if (new_ioas) { + rc = iopt_add_access(&new_ioas->iopt, access); + if (rc) + goto out_unlock; + iommufd_ref_to_users(obj); + } + + if (cur_ioas) { + iopt_remove_access(&cur_ioas->iopt, access); + refcount_dec(&cur_ioas->obj.users); + } + + access->ioas = new_ioas; + mutex_unlock(&access->ioas_lock); + + return 0; + +out_unlock: + mutex_unlock(&access->ioas_lock); + if (new_ioas) + iommufd_put_object(obj); + return rc; +} +EXPORT_SYMBOL_NS_GPL(iommufd_access_set_ioas, IOMMUFD); + /** * iommufd_access_notify_unmap - Notify users of an iopt to stop using it * @iopt: iopt to work on @@ -544,8 +574,8 @@ void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova, void iommufd_access_unpin_pages(struct iommufd_access *access, unsigned long iova, unsigned long length) { - struct io_pagetable *iopt = &access->ioas->iopt; struct iopt_area_contig_iter iter; + struct io_pagetable *iopt; unsigned long last_iova; struct iopt_area *area; @@ -553,6 +583,13 @@ void iommufd_access_unpin_pages(struct iommufd_access *access, WARN_ON(check_add_overflow(iova, length - 1, &last_iova))) return; + mutex_lock(&access->ioas_lock); + if (!access->ioas) { + mutex_unlock(&access->ioas_lock); + return; + } + iopt = &access->ioas->iopt; + down_read(&iopt->iova_rwsem); iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) iopt_area_remove_access( @@ -562,6 +599,7 @@ void iommufd_access_unpin_pages(struct iommufd_access *access, min(last_iova, iopt_area_last_iova(area)))); up_read(&iopt->iova_rwsem); WARN_ON(!iopt_area_contig_done(&iter)); + mutex_unlock(&access->ioas_lock); } EXPORT_SYMBOL_NS_GPL(iommufd_access_unpin_pages, IOMMUFD); @@ -607,8 +645,8 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, unsigned long length, struct page **out_pages, unsigned int flags) { - struct io_pagetable *iopt = &access->ioas->iopt; struct iopt_area_contig_iter iter; + struct io_pagetable *iopt; unsigned long last_iova; struct iopt_area *area; int rc; @@ -623,6 +661,13 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, if (check_add_overflow(iova, length - 1, &last_iova)) return -EOVERFLOW; + mutex_lock(&access->ioas_lock); + if (!access->ioas) { + mutex_unlock(&access->ioas_lock); + return -ENOENT; + } + iopt = &access->ioas->iopt; + down_read(&iopt->iova_rwsem); iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) { unsigned long last = min(last_iova, iopt_area_last_iova(area)); @@ -653,6 +698,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, } up_read(&iopt->iova_rwsem); + mutex_unlock(&access->ioas_lock); return 0; err_remove: @@ -667,6 +713,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, iopt_area_last_iova(area)))); } up_read(&iopt->iova_rwsem); + mutex_unlock(&access->ioas_lock); return rc; } EXPORT_SYMBOL_NS_GPL(iommufd_access_pin_pages, IOMMUFD); @@ -686,8 +733,8 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_pin_pages, IOMMUFD); int iommufd_access_rw(struct iommufd_access *access, unsigned long iova, void *data, size_t length, unsigned int flags) { - struct io_pagetable *iopt = &access->ioas->iopt; struct iopt_area_contig_iter iter; + struct io_pagetable *iopt; struct iopt_area *area; unsigned long last_iova; int rc; @@ -697,6 +744,13 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova, if (check_add_overflow(iova, length - 1, &last_iova)) return -EOVERFLOW; + mutex_lock(&access->ioas_lock); + if (!access->ioas) { + mutex_unlock(&access->ioas_lock); + return -ENOENT; + } + iopt = &access->ioas->iopt; + down_read(&iopt->iova_rwsem); iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) { unsigned long last = min(last_iova, iopt_area_last_iova(area)); @@ -723,6 +777,7 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova, rc = -ENOENT; err_out: up_read(&iopt->iova_rwsem); + mutex_unlock(&access->ioas_lock); return rc; } EXPORT_SYMBOL_NS_GPL(iommufd_access_rw, IOMMUFD); diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 9d7f71510ca1..820251b83ae4 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -263,6 +263,7 @@ struct iommufd_access { struct iommufd_object obj; struct iommufd_ctx *ictx; struct iommufd_ioas *ioas; + struct mutex ioas_lock; const struct iommufd_access_ops *ops; void *data; unsigned long iova_alignment; diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index cfb5fe9a5e0e..db4011bdc8a9 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -571,7 +571,7 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd, } access = iommufd_access_create( - ucmd->ictx, ioas_id, + ucmd->ictx, (flags & MOCK_FLAGS_ACCESS_CREATE_NEEDS_PIN_PAGES) ? &selftest_access_ops_pin : &selftest_access_ops, @@ -580,6 +580,9 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd, rc = PTR_ERR(access); goto out_put_fdno; } + rc = iommufd_access_set_ioas(access, ioas_id); + if (rc) + goto out_destroy; cmd->create_access.out_access_fd = fdno; rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); if (rc) diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c index db4efbd56042..1f87294c1931 100644 --- a/drivers/vfio/iommufd.c +++ b/drivers/vfio/iommufd.c @@ -138,10 +138,18 @@ static const struct iommufd_access_ops vfio_user_ops = { int vfio_iommufd_emulated_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx, u32 *out_device_id) { + struct iommufd_access *user; + lockdep_assert_held(&vdev->dev_set->lock); - vdev->iommufd_ictx = ictx; iommufd_ctx_get(ictx); + user = iommufd_access_create(ictx, &vfio_user_ops, vdev); + if (IS_ERR(user)) { + iommufd_ctx_put(ictx); + return PTR_ERR(user); + } + vdev->iommufd_access = user; + vdev->iommufd_ictx = ictx; return 0; } EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_bind); @@ -161,15 +169,12 @@ EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_unbind); int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id) { - struct iommufd_access *user; - lockdep_assert_held(&vdev->dev_set->lock); - user = iommufd_access_create(vdev->iommufd_ictx, *pt_id, &vfio_user_ops, - vdev); - if (IS_ERR(user)) - return PTR_ERR(user); - vdev->iommufd_access = user; - return 0; + if (!vdev->iommufd_ictx) + return -EINVAL; + if (!vdev->iommufd_access) + return -ENOENT; + return iommufd_access_set_ioas(vdev->iommufd_access, *pt_id); } EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_attach_ioas); diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index c0b5b3ac34f1..247b11609c79 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -40,9 +40,10 @@ enum { }; struct iommufd_access * -iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id, +iommufd_access_create(struct iommufd_ctx *ictx, const struct iommufd_access_ops *ops, void *data); void iommufd_access_destroy(struct iommufd_access *access); +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id); void iommufd_ctx_get(struct iommufd_ctx *ictx);