Re: [PATCH v1 01/13] genirq/msi: Store the IOMMU IOVA directly in msi_desc instead of iommu_cookie

[Thomas Gleixner <tglx@linutronix.de>]

On Sat, Feb 08 2025 at 01:02, Nicolin Chen wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
>
> All the iommu cases simply want to override the MSI page's address with
> the IOVA that was mapped through the iommu. This doesn't need a cookie
> pointer, we just need to store the IOVA and its page size in the
> msi_desc.

We need to do nothing :)

> Instead provide msi_desc_set_iommu_msi_iova() which allows the IOMMU side
> to specify the IOVA that the MSI page is placed during
> iommu_dma_prepare_msi(). This is stored in the msi_desc and then
> iommu_dma_compose_msi_msg() is a simple inline that sets address_hi/lo.
>
> The next patch will correct the naming.
>
> This is done because we cannot correctly lock access to group->domain
> in

Now this gets really confusing. How is the next patch which corrects the
naming related the locking problem?

> the atomic context that iommu_dma_compose_msi_msg() is called under. Today
> the locking miss is tolerable because dma_iommu.c operates under an
> assumption that the domain does not change while a driver is probed.
>
> However iommufd now permits the domain to change while the driver is
> probed and VFIO userspace can create races with IRQ changes calling
> iommu_dma_prepare_msi/compose_msi_msg() and changing/freeing the
> iommu_domain.
>
> Removing the pointer, and critically, the call to
> iommu_get_domain_for_dev() during compose resolves this race.

So this change log really fails to follow the basic structure:

   The context, the problem and the solution

Something like this:

   The IOMMU translation for MSI message addresses is a two stage
   process:

     1) A cookie containing the IOVA address is stored in the MSI
        descriptor, when a MSI interrupt is allocated

     2) The compose callback uses this cookie to apply the translation
        to the message address.

   This worked so far because .......

   With the iommufd rework this becomes racy, because ...

   Fix this by storing ... instead of ... ....

Hmm?

        tglx