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
>
