Re: [PATCH v4] iommu/amd: Invalidate IRT cache for DMA aliases

[Vasant Hegde]

Magnus,


On 4/1/2026 4:49 PM, Magnus Kalland wrote:
> DMA aliasing causes interrupt remapping table entries (IRTEs) to be shared
> between multiple device IDs. See commit 3c124435e8dd
> ("iommu/amd: Support multiple PCI DMA aliases in IRQ Remapping") for more
> information on this. However, the AMD IOMMU driver currently invalidates
> IRTE cache entries on a per-device basis whenever an IRTE is updated, not
> for each alias.
> 
> This approach leaves stale IRTE cache entries when an IRTE is cached under
> one DMA alias but later updated and invalidated through a different alias.
> In such cases, the original device ID is never invalidated, since it is
> programmed via aliasing.
> 
> This incoherency bug has been observed when IRTEs are cached for one
> Non-Transparent Bridge (NTB) DMA alias, later updated via another.
> 
> Fix this by invalidating the interrupt remapping table cache for all DMA
> aliases when updating an IRTE.
> 
> Changes since v3:
>  - Revert back to using pci_for_each_dma_alias (v2), but look up pdev with
>    search_dev_data instead of pci_get_domain_bus_and_slot (lockdep). This
>    avoids changing any locks
>  - Rebased on iommu next
>  - Tested atop
> https://lore.kernel.org/linux-iommu/20260401080017.117549-1-vasant.hegde@xxxxxxx/
> 
> Link to v2:
> https://lore.kernel.org/linux-iommu/26cfa307-6c33-41f9-a7a0-fbf202b38a00@xxxxxxx/

Next time please move the changelog after "---" so that it gets discarded
automatically when applying patches.

> 
> Co-developed-by: Lars B. Kristiansen <larsk@xxxxxxxxxxxxxx>
> Signed-off-by: Lars B. Kristiansen <larsk@xxxxxxxxxxxxxx>
> Co-developed-by: Jonas Markussen <jonas@xxxxxxxxxxxxxx>
> Signed-off-by: Jonas Markussen <jonas@xxxxxxxxxxxxxx>
> Co-developed-by: Tore H. Larsen <torel@xxxxxxxxx>
> Signed-off-by: Tore H. Larsen <torel@xxxxxxxxx>
> Signed-off-by: Magnus Kalland <magnus@xxxxxxxxxxxxxx>
> Link: https://lore.kernel.org/linux-iommu/ea4cfd4d-9ad2-4451-a896-9f0435e72a06@xxxxxxx/

Overall it looks good. One minor comment (see below).

Reviewed-by: Vasant Hegde <vasant.hegde@xxxxxxx>


> 
> ---
>  drivers/iommu/amd/iommu.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index f22a7e9ecfdb..217da630d847 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3186,26 +3186,42 @@ const struct iommu_ops amd_iommu_ops = {
>  static struct irq_chip amd_ir_chip;
>  static DEFINE_SPINLOCK(iommu_table_lock);
>  
> +static int iommu_flush_dev_irt(struct pci_dev *unused, u16 devid, void *data)
> +{
> +	int ret;
> +	struct iommu_cmd cmd;
> +	struct amd_iommu *iommu = data;
> +
> +	build_inv_irt(&cmd, devid);
> +	ret = __iommu_queue_command_sync(iommu, &cmd, true);
> +	return ret;
> +}
> +
>  static void iommu_flush_irt_and_complete(struct amd_iommu *iommu, u16 devid)
>  {
>  	int ret;
>  	u64 data;
>  	unsigned long flags;
> -	struct iommu_cmd cmd, cmd2;
> +	struct iommu_cmd cmd;
> +	struct pci_dev *pdev = NULL;
> +	struct iommu_dev_data *dev_data = search_dev_data(iommu, devid);
>  
>  	if (iommu->irtcachedis_enabled)
>  		return;
>  
> -	build_inv_irt(&cmd, devid);
> +	if (dev_data && dev_data->dev && dev_is_pci(dev_data->dev))
> +		pdev = to_pci_dev(dev_data->dev);
>  
>  	raw_spin_lock_irqsave(&iommu->lock, flags);
>  	data = get_cmdsem_val(iommu);
> -	build_completion_wait(&cmd2, iommu, data);
> +	build_completion_wait(&cmd, iommu, data);
>  
> -	ret = __iommu_queue_command_sync(iommu, &cmd, true);
> -	if (ret)
> -		goto out_err;
> -	ret = __iommu_queue_command_sync(iommu, &cmd2, false);
> +	if (pdev)
> +		ret = pci_for_each_dma_alias(pdev, iommu_flush_dev_irt, iommu);
> +	else
> +		ret = iommu_flush_dev_irt(NULL, devid, iommu);
> +

if (ret)
	goto out_err;

-Vasant