Messages in this thread | | | From | "Liu, Yi L" <> | Subject | RE: [PATCH v2 1/3] iommufd: Add devices_users to track the hw_pagetable usage by device | Date | Tue, 7 Feb 2023 04:19:29 +0000 |
| |
> From: Nicolin Chen <nicolinc@nvidia.com> > 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
| |