| Message ID | 20191122182943.4656-7-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU device | expand |
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 >
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,
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(-)