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

Jason Gunthorpe
2023-03-10T15:56:36Z

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