Re: [PATCH for-5.0 v11 06/20] virtio-iommu: Implement attach/detach command

[Auger Eric]

Hi Jean,

On 12/10/19 5:41 PM, Jean-Philippe Brucker wrote:
> On Fri, Nov 22, 2019 at 07:29:29PM +0100, Eric Auger wrote:
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 235bde2203..138d5b2a9c 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -77,11 +77,12 @@ static gint interval_cmp(gconstpointer a, gconstpointer 
>> b, gpointer user_data)
>>  static void virtio_iommu_detach_endpoint_from_domain(viommu_endpoint *ep)
>>  {
>>      QLIST_REMOVE(ep, next);
>> +    g_tree_unref(ep->domain->mappings);
>>      ep->domain = NULL;
>>  }
>>  
>> -viommu_endpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s, uint32_t ep_id);
>> -viommu_endpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s, uint32_t ep_id)
>> +static viommu_endpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s,
>> +                                                  uint32_t ep_id)
>>  {
>>      viommu_endpoint *ep;
>>  
>> @@ -102,15 +103,14 @@ static void virtio_iommu_put_endpoint(gpointer data)
>>  
>>      if (ep->domain) {
>>          virtio_iommu_detach_endpoint_from_domain(ep);
>> -        g_tree_unref(ep->domain->mappings);
>>      }
>>  
>>      trace_virtio_iommu_put_endpoint(ep->id);
>>      g_free(ep);
>>  }
>>  
>> -viommu_domain *virtio_iommu_get_domain(VirtIOIOMMU *s, uint32_t domain_id);
>> -viommu_domain *virtio_iommu_get_domain(VirtIOIOMMU *s, uint32_t domain_id)
>> +static viommu_domain *virtio_iommu_get_domain(VirtIOIOMMU *s,
>> +                                              uint32_t domain_id)
> 
> Looks like the above change belong to patch 5?
virtio_iommu_get_domain was not used yet in last patch. I turn it into
static now it gets used.
> 
>>  {
>>      viommu_domain *domain;
>>  
>> @@ -137,7 +137,6 @@ static void virtio_iommu_put_domain(gpointer data)
>>      QLIST_FOREACH_SAFE(iter, &domain->endpoint_list, next, tmp) {
>>          virtio_iommu_detach_endpoint_from_domain(iter);
>>      }
>> -    g_tree_destroy(domain->mappings);
> 
> When created by virtio_iommu_get_domain(), mappings has one reference.
> Then for each attach (including the first one) an additional reference is
> taken, and freed by virtio_iommu_detach_endpoint_from_domain(). So I think
> there are two problems:
> 
> * virtio_iommu_put_domain() drops one ref for each endpoint, but we still
>   have one reference to mappings, so they're not freed. We do need this
>   g_tree_destroy()
> 
> * After detaching all the endpoints, the guest may reuse the domain ID for
>   another domain, but the previous mappings haven't been erased. Not sure
>   how to fix this using the g_tree refs, because dropping all the
>   references will free the internal tree data and it won't be reusable.

You're perfectly right, mappings were not destroyed and I missed that.
So I made 2 modifications:
- do not increment the ref count on the first EP addition
- destroy the domain when its EP list get empty.

Thanks

Eric
> 
> Thanks,
> Jean
>