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

[Magnus Kalland]

Hi Vasant,

> I went back to those reports and reviewed it again. It looks like
> pci_get_domain_bus_and_slot() takes spinlock which is causing the lockdep issue.

> I think V2 is good w/ some changes to get the pdev.
> Instead of pci_get_domain_bus_and_slot(), we should use dev_data (like we do in
> other places).

> >
> > However, we have a v4 ready doing 256 loop iterations instead, since aliases
> > are always on the same bus. I think that's a better approach.
> > What do you think?

> Its a performance sensitive code path. Also during init path, dma aliases are
> set properly. I think we should go w/ pci)fir_each_dma_alias -> flush irte path.


> I have added below fix on top of your v2 and did some sanity tests. So far it
> looks good. I have requested Dheeraj to rerun the tests.

> (https://lore.kernel.org/linux-iommu/20260205140059.11857-2-magnus@xxxxxxxxxxxxxx/)


> -Vasant


> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 050178cf388f..eb23b9e7bf03 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3118,12 +3118,10 @@ static void iommu_flush_irt_and_complete(struct
> amd_iommu *iommu, u16 devid)
>{
>	int ret;
>	u64 data;
> -	int domain = iommu->pci_seg->id;
> -	unsigned int bus = PCI_BUS_NUM(devid);
> -	unsigned int devfn = devid & 0xff;
>	unsigned long flags;
>	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;
> @@ -3131,11 +3129,12 @@ static void iommu_flush_irt_and_complete(struct
> amd_iommu *iommu, u16 devid)
>	data = atomic64_inc_return(&iommu->cmd_sem_val);
>	build_completion_wait(&cmd, iommu, data);
>
> -	pdev = pci_get_domain_bus_and_slot(domain, bus, devfn);
> +	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);
>	if (pdev) {
>		ret = pci_for_each_dma_alias(pdev, iommu_flush_dev_irt, iommu);
> -		pci_dev_put(pdev);
>	} else {
>		ret = iommu_flush_dev_irt(NULL, devid, iommu);
>	}

Thanks for the fix. I've tested it and it looks good on our end.

Currently the v2 does not apply to next. Should I send a v4 with
your changes that applies cleanly?

Magnus