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

[Suthikulpanit, Suravee]

On 8/19/2026 5:18 PM, guanghuifeng@xxxxxxxxxxxxxxxxx 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@xxxxxxx>
> > ---
> >   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