This patch implements the endpoint attach/detach to/from
a domain.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---
---
 hw/virtio/virtio-iommu.c | 43 ++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

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)
 {
     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);
     trace_virtio_iommu_put_domain(domain->id);
     g_free(domain);
 }
@@ -186,10 +185,27 @@ static int virtio_iommu_attach(VirtIOIOMMU *s,
 {
     uint32_t domain_id = le32_to_cpu(req->domain);
     uint32_t ep_id = le32_to_cpu(req->endpoint);
+    viommu_domain *domain;
+    viommu_endpoint *ep;
 
     trace_virtio_iommu_attach(domain_id, ep_id);
 
-    return VIRTIO_IOMMU_S_UNSUPP;
+    ep = virtio_iommu_get_endpoint(s, ep_id);
+    if (ep->domain) {
+        /*
+         * the device is already attached to a domain,
+         * detach it first
+         */
+        virtio_iommu_detach_endpoint_from_domain(ep);
+    }
+
+    domain = virtio_iommu_get_domain(s, domain_id);
+    QLIST_INSERT_HEAD(&domain->endpoint_list, ep, next);
+
+    ep->domain = domain;
+    g_tree_ref(domain->mappings);
+
+    return VIRTIO_IOMMU_S_OK;
 }
 
 static int virtio_iommu_detach(VirtIOIOMMU *s,
@@ -197,10 +213,21 @@ 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);
+    viommu_endpoint *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;
+    }
+
+    if (!ep->domain) {
+        return VIRTIO_IOMMU_S_INVAL;
+    }
+
+    virtio_iommu_detach_endpoint_from_domain(ep);
+    return VIRTIO_IOMMU_S_OK;
 }
 
 static int virtio_iommu_map(VirtIOIOMMU *s,
-- 
2.20.1

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?

>  {
>      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.

Thanks,
Jean

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
>