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

[Liu, Yi L]

> From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> Sent: Thursday, February 2, 2023 5:12 PM
> 
> 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()?

Yeah, doing iopt_table_add_domain() in hwpt_alloc suits well.
The only reason for current code is SMMU has drawback with it.
Great to see it is solved.

> > 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/85248e1c5f645e1eb701562e
> b078cf586af617fe
> (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.

Yes.

> 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.

I think reusing the device lock can simplify things to avoid bad
readability. However, I'll do a double-check.

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

Thanks, I'm now re-integrating the nesting patches.

Regards,
Yi Liu