Re: [PATCH v14 03/11] virtio-iommu: Implement attach/detach command

[Peter Xu <peterx@redhat.com>]

On Fri, Feb 07, 2020 at 10:31:55AM +0100, Eric Auger wrote:

[...]

> v13 -> v14:
> - in virtio_iommu_put_endpoint, if the EP is attached to a
>   domain, call virtio_iommu_detach_endpoint_from_domain()
> - remove domain ref counting and simply delete the mappings
>   gtree when the last EP is detached from the domain

Yeh this looks a good optimization!  Ref counting protects the domain
from being gone when there's still EP in the domain, but since we've
got the ep_list in domain after all so it seems to be safe and clearer.

> - in virtio_iommu_detach_endpoint_from_domain(), return if the
>   ep's domain is unset.

[...]

> +static void virtio_iommu_put_domain(gpointer data)
> +{
> +    VirtIOIOMMUDomain *domain = (VirtIOIOMMUDomain *)data;
> +    VirtIOIOMMUEndpoint *iter, *tmp;
> +
> +    QLIST_FOREACH_SAFE(iter, &domain->endpoint_list, next, tmp) {
> +        virtio_iommu_detach_endpoint_from_domain(iter);
> +    }
> +    trace_virtio_iommu_put_domain(domain->id);

[1]

> +    g_free(domain);
> +}

[...]

>  static int virtio_iommu_attach(VirtIOIOMMU *s,
>                                 struct virtio_iommu_req_attach *req)
>  {
>      uint32_t domain_id = le32_to_cpu(req->domain);
>      uint32_t ep_id = le32_to_cpu(req->endpoint);
> +    VirtIOIOMMUDomain *domain;
> +    VirtIOIOMMUEndpoint *ep;
>  
>      trace_virtio_iommu_attach(domain_id, ep_id);
>  
> -    return VIRTIO_IOMMU_S_UNSUPP;
> +    ep = virtio_iommu_get_endpoint(s, ep_id);
> +    if (!ep) {
> +        return VIRTIO_IOMMU_S_NOENT;
> +    }
> +
> +    if (ep->domain) {
> +        VirtIOIOMMUDomain *previous_domain = ep->domain;
> +        /*
> +         * the device is already attached to a domain,
> +         * detach it first
> +         */
> +        virtio_iommu_detach_endpoint_from_domain(ep);
> +        if (QLIST_EMPTY(&previous_domain->endpoint_list)) {

I feel like we still need:

               g_tree_destroy(previous_domain->mappings);

Or the mappings will be leaked.

To make this simpler, maybe we can destroy the mappings at [1] above.
Then we can remove line [2] below too.

> +            g_tree_remove(s->domains, GUINT_TO_POINTER(previous_domain->id));
> +        }
> +    }
> +
> +    domain = virtio_iommu_get_domain(s, domain_id);
> +    QLIST_INSERT_HEAD(&domain->endpoint_list, ep, next);
> +
> +    ep->domain = domain;
> +
> +    return VIRTIO_IOMMU_S_OK;
>  }
>  
>  static int virtio_iommu_detach(VirtIOIOMMU *s,
> @@ -50,10 +268,29 @@ static int virtio_iommu_detach(VirtIOIOMMU *s,
>  {
>      uint32_t domain_id = le32_to_cpu(req->domain);
>      uint32_t ep_id = le32_to_cpu(req->endpoint);
> +    VirtIOIOMMUDomain *domain;
> +    VirtIOIOMMUEndpoint *ep;
>  
>      trace_virtio_iommu_detach(domain_id, ep_id);
>  
> -    return VIRTIO_IOMMU_S_UNSUPP;
> +    ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id));
> +    if (!ep) {
> +        return VIRTIO_IOMMU_S_NOENT;
> +    }
> +
> +    domain = ep->domain;
> +
> +    if (!domain || domain->id != domain_id) {
> +        return VIRTIO_IOMMU_S_INVAL;
> +    }
> +
> +    virtio_iommu_detach_endpoint_from_domain(ep);
> +
> +    if (QLIST_EMPTY(&domain->endpoint_list)) {
> +        g_tree_destroy(domain->mappings);

[2]

> +        g_tree_remove(s->domains, GUINT_TO_POINTER(domain->id));
> +    }
> +    return VIRTIO_IOMMU_S_OK;
>  }
>  
>  static int virtio_iommu_map(VirtIOIOMMU *s,
> @@ -172,6 +409,27 @@ out:
>      }
>  }

Other than that, the whole patch looks good to me.

Thanks,

-- 
Peter Xu