Messages in this thread | | | Date | Wed, 15 Mar 2023 18:21:50 -0700 | From | Nicolin Chen <> | Subject | Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper |
| |
Hi Robin,
How do you think about Jason's proposal below? I'd like to see us come to an agreement on an acceptable solution...
Thanks Nic
On Fri, Mar 10, 2023 at 11:55:07AM -0400, Jason Gunthorpe wrote: > On Fri, Mar 10, 2023 at 09:41:01AM +0100, Eric Auger wrote: > > > I tend to agree with Robin here. This was first introduced by > > > > [PATCH v7 21/22] iommu/dma: Add support for mapping MSIs <https://lore.kernel.org/all/2273af20d844bd618c6a90b57e639700328ebf7f.1473695704.git.robin.murphy@arm.com/#r> > > https://lore.kernel.org/all/2273af20d844bd618c6a90b57e639700328ebf7f.1473695704.git.robin.murphy@arm.com/ > > Presumably it had to use the iommu_get_domain_for_dev() instead of > iommu_get_dma_domain() to support ARM 32 arm-iommu. Ie it is poking > into the arm-iommu owned domain as well. VFIO just ended being the > same flow > > > even before the support un VFIO use case which came later on. So > > using the "unmanaged" terminology sounds improper to me, at least. > > Couldn't we use a parent/child terminology as used in the past in > > No objection to a better name... > > Actually how about if we write it like this? Robin would you be > happier? I think it much more clearly explains why this function is > special within our single domain attachment model. > > "get_unmanaged_msi_domain" seems like a much more narrowly specific to > the purpose name. > > int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr) > { > struct device *dev = msi_desc_to_dev(desc); > struct iommu_domain *domain = iommu_get_domain_for_dev(dev); > struct iommu_dma_msi_page *msi_page; > static DEFINE_MUTEX(msi_prepare_lock); /* see below */ > > desc->iommu_cookie = NULL; > > /* > * This probably shouldn't happen as the ARM32 systems should only have > * NULL if arm-iommu has been disconnected during setup/destruction. > * Assume it is an identity domain. > */ > if (!domain) > return 0; > > /* Caller is expected to use msi_addr for the page */ > if (domain->type == IOMMU_DOMAIN_IDENTITY) > return 0; > > /* > * The current domain is some driver opaque thing. We assume the > * driver/user knows what it is doing regarding ARM ITS MSI pages and we > * want to try to install the page into some kind of kernel owned > * unmanaged domain. Eg for nesting this will install the ITS page into > * the S2 domain and then we assume that the S1 domain has independently > * made it mapped at the same address. > */ > // FIXME wrap in a function > if (domain->type != IOMMU_DOMAIN_UNMANAGED && > domain->ops->get_unmanged_msi_domain) > domain = domain->ops->get_unmanged_msi_domain(domain); > > if (!domain || domain->type != IOMMU_DOMAIN_UNMANAGED) > return -EINVAL; > > // ??? > if (!domain->iova_cookie) > return 0; > > /* > * In fact the whole prepare operation should already be serialised by > * irq_domain_mutex further up the callchain, but that's pretty subtle > * on its own, so consider this locking as failsafe documentation... > */ > mutex_lock(&msi_prepare_lock); > msi_page = iommu_dma_get_msi_page(dev, msi_addr, domain); > mutex_unlock(&msi_prepare_lock); > > msi_desc_set_iommu_cookie(desc, msi_page); > > if (!msi_page) > return -ENOMEM; > return 0; > } > > Jason
| |