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

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

On 8/19/2026 5:18 PM, guanghuifeng@linux.alibaba.com wrote:
> 
> 在 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;
>> +
>>
>> ....
>> 
>> @@ -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.

I'll update the comment in v5.

Thanks,
Suravee