Re: [PATCH v2] iommu/dma: Restore locking around msi_page_list

[Jason Gunthorpe]

> @@ -2204,6 +2204,19 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
>  	dma_addr_t iova;
>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>  	size_t size = cookie_msi_granule(domain);
> +	static DEFINE_MUTEX(msi_prepare_lock);
> +
> +	/*
> +	 * Normally a device's default domain is only ever attached to that
> +	 * device's own group, and the group mutex held by
> +	 * iommu_group_mutex_assert()'s callers is enough on its own. A VFIO
> +	 * type1 container is the one case that breaks that assumption: it
> +	 * can merge devices from different groups onto one domain, so two
> +	 * devices' group mutexes don't serialize each other here. A static
> +	 * lock is sufficient due to the expectation that this is a corner
> +	 * case that will never be contended in practice.
> +	 */
> +	guard(mutex)(&msi_prepare_lock);

It looks OK to me

Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>

But I wondered what the original comment ment:

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

I sicced AI on the question and it pointed out that when this was originally
done in commit c18647900ec8 ("iommu/dma: Relax locking in
iommu_dma_prepare_msi()") there was in fact a global irq_domain_mutex being
held in this callchain.

But that was changed by commit 9dbb8e3452ab ("irqdomain: Switch to per-domain
locking") and it became a per-domain lock

And then this case probably has multiple irq domains for all the different
VFIO devices.

So the comment became out of date and I didn't double check my self it was
still accurate when I removed it in 288683c92b1a ..

-- 
Jason