Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

[Nicolin Chen <nicolinc@nvidia.com>]

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