| Newsgroups |
dev.linux.lists.iommu,org.kernel.vger.linux-kernel |
| Message-ID |
<[email protected]> |
在 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 <[email protected]>
> ---
> 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);
> }
>