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
