On Tue, 15 Jul 2025 10:01:21 -0700
Nicolin Chen <nicol...@nvidia.com> wrote:
> On Tue, Jul 15, 2025 at 11:29:41AM +0100, Jonathan Cameron wrote:
> > > + if (!iommufd_backend_alloc_viommu(idev->iommufd, idev->devid,
> > > + IOMMU_VIOMMU_TYPE_ARM_SMMUV3,
> > > + s2_hwpt_id, &viommu_id, errp)) {
> > > + return false;
> > > + }
>
> [...]
>
> > > +free_abort_hwpt:
> > > + iommufd_backend_free_id(idev->iommufd, viommu->abort_hwpt_id);
> > > +free_viommu:
> > > + iommufd_backend_free_id(idev->iommufd, viommu->core.viommu_id);
> > > + g_free(viommu);
> >
> > No unwinding of iommufd_backened_alloc_viommu?
> > Looks like we just leak it until destruction of the fd.
> >
> > Maybe add a comment for those like me who aren't all that familiar with
> > this stuff and see an alloc with no matching free.
>
> Those iommufd_backend_free_id calls are the reverts. An iommufd
> object is free-ed using its object id, i.e. the "viommu_id" and
> "abort_hwpt_id" in the lines.
Ah. I confused IDs and thought this was unwinding the two
iommufd_backend_alloc_hwpt() calls but of course the second one doesn't
need unwinding in the error path as it is the last call so if it fails
nothing to unwind.
So feel free to ignore this comment.
>
> Adding comments to every single iommufd_backened_free_id() call
> isn't optimal, IMHO, as that function would be invoked across
> different vIOMMU files and even the vfio/iommufd core files.
>
> Perhaps QEMU should wrap it up with a helper, E.g.
>
> static inline void iommufd_backend_free(int iommufd, int obj_id)
> {
> iommufd_backend_free_id(iommufd, obj_id);
> }
>
> if it helps readability?
That would then leave us with iommufd_backend_alloc_hwpt() being
unwound by a direct iommufd_backend_free_id() which is equally
odd - so let's just keep them all being odd.
Jonathan
>
> Nicolin
