Re: [PATCH for-5.0 v11 06/20] virtio-iommu: Implement attach/detach command
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
>
Re: [PATCH for-5.0 v11 06/20] virtio-iommu: Implement attach/detach command
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
[PATCH for-5.0 v11 06/20] virtio-iommu: Implement attach/detach command
This patch implements the endpoint attach/detach to/from
a domain.
Signed-off-by: Eric Auger
---
---
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
