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

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,

On 2/7/20 9:26 PM, Peter Xu wrote:
> 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.
You're fully right :-(
> 
> To make this simpler, maybe we can destroy the mappings at [1] above.
> Then we can remove line [2] below too.
Yes I chose to destroy the mappings in the put_domain and remove [2].
> 
>> +            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.

Thank you for the careful review.

Eric
> 
> Thanks,
>