Re: [PATCH 23/24] iommu/amd: Assign per-vIOMMU translate device ID

["guanghuifeng@linux.alibaba.com" <guanghuifeng@linux.alibaba.com>]

在 2026/7/27 21:29, Suravee Suthikulpanit 写道:
> Allocate one translate-device-id per IOMMUFD vIOMMU instance from the
> per-segment pool on init.  Program translation DTE and VFctrl TransDevID
> after MMIO reset; clear both on init error and destroy.
>
> Add per-vIOMMU trans_devid_lock to serialize DTE and VFctrl updates
> during teardown.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   drivers/iommu/amd/amd_iommu_types.h |  7 ++++++
>   drivers/iommu/amd/iommufd.c         | 35 +++++++++++++++++++++++++++++
>   drivers/iommu/amd/viommu.c          |  2 ++
>   3 files changed, 44 insertions(+)
>
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index f288a7b384d0..39cf2c588106 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -559,6 +559,13 @@ struct amd_iommu_viommu {
>   	u64 *domid_table;
>   	u16 trans_devid;
>   
> +	/*
> +	 * Serializes translate-device-id hardware changes (DTE, VFctrl) and
> +	 * coordinates with pool relocation during PCI attach.  Lock ordering:
> +	 * pci_seg->trans_devid_mutex, then trans_devid_lock.
> +	 */
> +	struct mutex trans_devid_lock;
> +
>   	/* Offset for mmap() of guest VF MMIO; set after iommufd_viommu_alloc_mmap(). */
>   	unsigned long vfmmio_mmap_offset;
>   };
> diff --git a/drivers/iommu/amd/iommufd.c b/drivers/iommu/amd/iommufd.c
> index 7e070f72f02e..43575af59bba 100644
> --- a/drivers/iommu/amd/iommufd.c
> +++ b/drivers/iommu/amd/iommufd.c
> @@ -48,11 +48,15 @@ int amd_iommufd_viommu_init(struct iommufd_viommu *viommu, struct iommu_domain *
>   	int ret;
>   	phys_addr_t page_base;
>   	unsigned long flags;
> +	u16 trans_devid;
> +	bool trans_devid_allocated = false;
> +	bool trans_dte_set = false;
>   	struct iommu_viommu_amd data = {};
>   	struct protection_domain *pdom = to_pdomain(parent);
>   	struct amd_iommu *iommu = container_of(viommu->iommu_dev, struct amd_iommu, iommu);
>   	struct amd_iommu_viommu *aviommu = container_of(viommu, struct amd_iommu_viommu, core);
>   
> +	mutex_init(&aviommu->trans_devid_lock);
>   	xa_init_flags(&aviommu->gdomid_array, XA_FLAGS_ALLOC1);
>   	aviommu->parent = pdom;
>   
> @@ -81,9 +85,22 @@ int amd_iommufd_viommu_init(struct iommufd_viommu *viommu, struct iommu_domain *
>   
>   	data.out_vfmmio_mmap_offset = aviommu->vfmmio_mmap_offset;
>   
> +	ret = amd_iommu_trans_devid_alloc(iommu->pci_seg, aviommu);
> +	if (ret < 0)
> +		goto err_trans_devid;
> +	trans_devid = ret;
> +	trans_devid_allocated = true;
> +	aviommu->trans_devid = trans_devid;
> +
>   	/* Reset vIOMMU MMIOs to initialize the vIOMMU */
>   	iommu_reset_vmmio(iommu, aviommu->gid);
>   
> +	ret = amd_iommu_set_translate_dte(viommu);
> +	if (ret)
> +		goto err_init;
> +	trans_dte_set = true;
> +	amd_iommu_update_vfctrl_mmio_translate_devid(iommu, aviommu->gid, trans_devid);
> +
>   	ret = amd_viommu_init_one(iommu, aviommu);
>   	if (ret)
>   		goto err_init;
> @@ -102,6 +119,18 @@ int amd_iommufd_viommu_init(struct iommufd_viommu *viommu, struct iommu_domain *
>   
>   	return 0;
>   err_init:
> +	if (trans_dte_set) {
> +		mutex_lock(&aviommu->trans_devid_lock);
> +		amd_iommu_update_vfctrl_mmio_translate_devid(iommu, aviommu->gid, 0);
> +		amd_iommu_clear_translate_dte(iommu, trans_devid);
> +		mutex_unlock(&aviommu->trans_devid_lock);
> +	}
> +	if (trans_devid_allocated) {
> +		mutex_lock(&aviommu->trans_devid_lock);
> +		amd_iommu_trans_devid_free(iommu->pci_seg, trans_devid, aviommu);
> +		mutex_unlock(&aviommu->trans_devid_lock);
> +	}
> +err_trans_devid:
>   	iommufd_viommu_destroy_mmap(&aviommu->core, aviommu->vfmmio_mmap_offset);
>   err_mmap:
>   	amd_iommu_gid_free(iommu, aviommu->gid);
> @@ -124,6 +153,12 @@ static void amd_iommufd_viommu_destroy(struct iommufd_viommu *viommu)
>   	xa_destroy(&aviommu->gdomid_array);
>   	iommufd_viommu_destroy_mmap(&aviommu->core, aviommu->vfmmio_mmap_offset);
>   	amd_viommu_uninit_one(iommu, aviommu);
> +
> +	mutex_lock(&aviommu->trans_devid_lock);
> +	amd_iommu_update_vfctrl_mmio_translate_devid(iommu, aviommu->gid, 0);
> +	amd_iommu_clear_translate_dte(iommu, aviommu->trans_devid);
> +	amd_iommu_trans_devid_free(iommu->pci_seg, aviommu->trans_devid, aviommu);
> +	mutex_unlock(&aviommu->trans_devid_lock);
>   	amd_iommu_gid_free(iommu, aviommu->gid);
>   }

The comment states "pci_seg->trans_devid_mutex, then trans_devid_lock"
as the lock ordering. However, examining the actual code paths:

In amd_iommufd_viommu_destroy():
   mutex_lock(&aviommu->trans_devid_lock);          /* viommu lock first */
   ...
   amd_iommu_trans_devid_free(...);                  /* takes seg_mutex 
inside */
   mutex_unlock(&aviommu->trans_devid_lock);

In amd_iommu_trans_devid_free():
   mutex_lock(&pci_seg->trans_devid_mutex);          /* seg_mutex nested */

In trans_devid_relocate() (Patch 24):
   mutex_lock(&aviommu->trans_devid_lock);          /* viommu lock first */
   mutex_lock(&pci_seg->trans_devid_mutex);          /* seg_mutex second */

All paths consistently take trans_devid_lock first, then
trans_devid_mutex (nested). The actual ordering is the opposite of
what the comment describes:

   Actual:   trans_devid_lock -> trans_devid_mutex
   Comment:  trans_devid_mutex -> trans_devid_lock

The code is consistent and there is no deadlock risk, but the
comment should be corrected to avoid confusing future readers:

   * Lock ordering: trans_devid_lock, then pci_seg->trans_devid_mutex.



>   
> diff --git a/drivers/iommu/amd/viommu.c b/drivers/iommu/amd/viommu.c
> index eb3f5217d856..ae4a6a2cae39 100644
> --- a/drivers/iommu/amd/viommu.c
> +++ b/drivers/iommu/amd/viommu.c
> @@ -501,6 +501,8 @@ void amd_viommu_uninit_one(struct amd_iommu *iommu, struct amd_iommu_viommu *avi
>   			       VIOMMU_DOMID_MAPPING_BASE,
>   			       VIOMMU_DOMID_MAPPING_ENTRY_SIZE,
>   			       aviommu->gid);
> +
> +	amd_iommu_update_vfctrl_mmio_translate_devid(iommu, aviommu->gid, 0);
>   	viommu_clear_mapping(iommu, aviommu);
>   }
>