Re: [PATCH v13 03/10] virtio-iommu: Implement attach/detach command

[Peter Xu]

On Mon, Feb 03, 2020 at 03:59:00PM +0100, Auger Eric wrote:

[...]

> >> +static void virtio_iommu_detach_endpoint_from_domain(VirtIOIOMMUEndpoint 
> >> *ep)
> >> +{
> >> +    QLIST_REMOVE(ep, next);
> >> +    g_tree_unref(ep->domain->mappings);
> > 
> > Here domain->mapping is unreferenced for each endpoint, while at [1]
> > below you only reference the domain->mappings if it's the first
> > endpoint.  Is that problematic?
> in [1] I take a ref to the domain->mappings if it is *not* the 1st
> endpoint. This aims at deleting the mappings gtree when the last EP is
> detached from the domain.
> 
> This fixes the issue reported by Jean in:
> https://patchwork.kernel.org/patch/11258267/#23046313

Ah OK. :)

However this is tricky.  How about do explicit g_tree_destroy() in
virtio_iommu_detach() when it's the last endpoint?  I see that you
have:

    /*
     * when the last EP is detached, simply remove the domain for
     * the domain list and destroy it. Note its mappings were already
     * freed by the ref count mechanism. Next operation involving
     * the same domain id will re-create one domain object.
     */
    if (QLIST_EMPTY(&domain->endpoint_list)) {
        g_tree_remove(s->domains, GUINT_TO_POINTER(domain->id));
    }

Then it becomes:

    if (QLIST_EMPTY(&domain->endpoint_list)) {
        g_tree_destroy(domain->mappings);
        g_tree_remove(s->domains, GUINT_TO_POINTER(domain->id));
    }

And also remove the trick in attach() so you take the domain ref
unconditionally.  Would that work?

Thanks,

-- 
Peter Xu