Re: [PATCH v2 1/3] iommufd: Add devices_users to track the hw_pagetable usage by device

[Nicolin Chen <nicolinc@nvidia.com>]

On Tue, Jan 31, 2023 at 11:48:04PM -0800, Nicolin Chen wrote:
> On Mon, Jan 30, 2023 at 12:54:01PM -0800, Nicolin Chen wrote:
> > On Mon, Jan 30, 2023 at 04:35:35PM -0400, Jason Gunthorpe wrote:
> > > On Mon, Jan 30, 2023 at 12:04:33PM -0800, Nicolin Chen wrote:
> > > 
> > > > I recall we've discussed this that SMMU sets up domain when it
> > > > attaches the device to, so we made a compromise here...
> > > 
> > > The ARM driver has a problem that it doesn't know what SMMU instance
> > > will host the domain when it is allocated so it doesn't know if it
> > > should select a S1 or S2 page table format - which is determined by
> > > the capabilities of the specific SMMU HW block.
> > > 
> > > However, we don't have this problem when creating the S2. The S2
> > > should be created by a special alloc_domain_iommufd() asking for the
> > > S2 format. Not only does the new alloc_domain_iommufd API directly
> > > request a S2 format table, but it also specifies the struct device so
> > > any residual details can be determined directly.
> > > 
> > > Thus there is no need to do the two stage process when working with
> > > the S2.
> > 
> > Ah, right! Taking a quick look, we should be able to call that
> > arm_smmu_domain_finalise when handling alloc_domain_iommufd().
> > 
> > > So fixup the driver to create fully configured iommu_domain's
> > > immediately and get rid of this problem.
> > 
> > OK. I will draft a patch today.
> 
> @Yi
> Do you recall doing iopt_table_add_domain() in hwpt_alloc()?
> 
> Jason has a great point above. So even SMMU should be able to
> call the iopt_table_add_domain() after a kernel-manged hwpt
> allocation rather than after an iommu_attach_group(), except
> an auto_domain or a selftest mock_domain that still needs to
> attach the device first, otherwise the SMMU driver (currently)
> cannot finalize the domain aperture.

Some update today: I found ops->domain_alloc_user() is used
for all domain allocations inside IOMMUFD. So, without any
special case, we could entirely do iopt_table_add_domain()
in the __iommufd_hw_pagetable_alloc() and accordingly do
iopt_table_remove_domain() in the hw_pagetable_destroy():
https://github.com/nicolinc/iommufd/commit/85248e1c5f645e1eb701562eb078cf586af617fe
(We can also skip that "symmetric" patch for the list_add,
 moving the list_add/del calls directly to alloc/destroy.)

Without the complication of the add/remove_domain() calls
in the do_attach/detach() functions, there is no need for
the device_users counter any more.

I am not 100% sure if we still need the shared device lock,
though so far the sanity that I run doesn't show a problem.
We may discuss about it later when we converge our branches.
As before, I am also okay to do in the way with incremental
changes on top of your tree and to ask you to integrate,
once you have your branch ready.

My full wip branch:
https://github.com/nicolinc/iommufd/commits/wip/iommufd-v6.2-rc5-nesting

Thanks
Nic