[Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Nicolin Chen <nicolinc at 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.