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,
>
