Multiple entities (e.g. VMBus devices) can use the same SINT route.  To
make their lives easier in maintaining SINT route ownership, make it
reference-counted.  Adjust the respective API names accordingly.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 target/i386/hyperv.h     | 10 +++++-----
 hw/misc/hyperv_testdev.c |  4 ++--
 target/i386/hyperv.c     | 25 +++++++++++++++++++++----
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
index bc071da..ca5a32d 100644
--- a/target/i386/hyperv.h
+++ b/target/i386/hyperv.h
@@ -23,11 +23,11 @@ typedef void (*HvSintAckClb)(void *data);
 
 int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
 
-HvSintRoute *kvm_hv_sint_route_create(X86CPU *cpu, uint32_t sint,
-                                      HvSintAckClb sint_ack_clb,
-                                      void *sint_ack_clb_data);
-
-void kvm_hv_sint_route_destroy(HvSintRoute *sint_route);
+HvSintRoute *hyperv_sint_route_new(X86CPU *cpu, uint32_t sint,
+                                   HvSintAckClb sint_ack_clb,
+                                   void *sint_ack_clb_data);
+void hyperv_sint_route_ref(HvSintRoute *sint_route);
+void hyperv_sint_route_unref(HvSintRoute *sint_route);
 
 int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
 
diff --git a/hw/misc/hyperv_testdev.c b/hw/misc/hyperv_testdev.c
index 1b12295..929bf4f 100644
--- a/hw/misc/hyperv_testdev.c
+++ b/hw/misc/hyperv_testdev.c
@@ -55,7 +55,7 @@ static void sint_route_create(HypervTestDev *dev, X86CPU *cpu, uint8_t sint)
     sint_route->cpu = cpu;
     sint_route->sint = sint;
 
-    sint_route->sint_route = kvm_hv_sint_route_create(cpu, sint, NULL, NULL);
+    sint_route->sint_route = hyperv_sint_route_new(cpu, sint, NULL, NULL);
     assert(sint_route->sint_route);
 
     QLIST_INSERT_HEAD(&dev->sint_routes, sint_route, le);
@@ -81,7 +81,7 @@ static void sint_route_destroy(HypervTestDev *dev, X86CPU *cpu, uint8_t sint)
 
     sint_route = sint_route_find(dev, cpu, sint);
     QLIST_REMOVE(sint_route, le);
-    kvm_hv_sint_route_destroy(sint_route->sint_route);
+    hyperv_sint_route_unref(sint_route->sint_route);
     g_free(sint_route);
 }
 
diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
index a150401..ae67f82 100644
--- a/target/i386/hyperv.c
+++ b/target/i386/hyperv.c
@@ -24,6 +24,7 @@ struct HvSintRoute {
     EventNotifier sint_ack_notifier;
     HvSintAckClb sint_ack_clb;
     void *sint_ack_clb_data;
+    unsigned refcount;
 };
 
 uint32_t hyperv_vp_index(X86CPU *cpu)
@@ -90,9 +91,9 @@ static void kvm_hv_sint_ack_handler(EventNotifier *notifier)
     sint_route->sint_ack_clb(sint_route->sint_ack_clb_data);
 }
 
-HvSintRoute *kvm_hv_sint_route_create(X86CPU *cpu, uint32_t sint,
-                                      HvSintAckClb sint_ack_clb,
-                                      void *sint_ack_clb_data)
+HvSintRoute *hyperv_sint_route_new(X86CPU *cpu, uint32_t sint,
+                                   HvSintAckClb sint_ack_clb,
+                                   void *sint_ack_clb_data)
 {
     HvSintRoute *sint_route;
     EventNotifier *ack_notifier;
@@ -130,6 +131,7 @@ HvSintRoute *kvm_hv_sint_route_create(X86CPU *cpu, uint32_t sint,
     sint_route->sint_ack_clb_data = sint_ack_clb_data;
     sint_route->cpu = cpu;
     sint_route->sint = sint;
+    sint_route->refcount = 1;
 
     return sint_route;
 
@@ -148,8 +150,23 @@ err:
     return NULL;
 }
 
-void kvm_hv_sint_route_destroy(HvSintRoute *sint_route)
+void hyperv_sint_route_ref(HvSintRoute *sint_route)
 {
+    sint_route->refcount++;
+}
+
+void hyperv_sint_route_unref(HvSintRoute *sint_route)
+{
+    if (!sint_route) {
+        return;
+    }
+
+    assert(sint_route->refcount > 0);
+
+    if (--sint_route->refcount) {
+        return;
+    }
+
     kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state,
                                           &sint_route->sint_set_notifier,
                                           sint_route->gsi);
-- 
2.9.4

On Tue, Jun 06, 2017 at 09:19:37PM +0300, Roman Kagan wrote:
> Multiple entities (e.g. VMBus devices) can use the same SINT route.  To
> make their lives easier in maintaining SINT route ownership, make it
> reference-counted.  Adjust the respective API names accordingly.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>

Isn't it easier to reuse existing refcounting infrastructure
here?  Is it overkill to make it a QOM object? (struct Object has
40 bytes)

-- 
Eduardo

On Wed, Jun 14, 2017 at 10:53:25AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 06, 2017 at 09:19:37PM +0300, Roman Kagan wrote:
> > Multiple entities (e.g. VMBus devices) can use the same SINT route.  To
> > make their lives easier in maintaining SINT route ownership, make it
> > reference-counted.  Adjust the respective API names accordingly.
> > 
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> 
> Isn't it easier to reuse existing refcounting infrastructure
> here?  Is it overkill to make it a QOM object? (struct Object has
> 40 bytes)

Normally the guests use a sint route per cpu or less.  So no, the space
overhead is not an issue.

I also wanted to reuse regular QOM refcounting so I QOM-ified it at
first.  However, while hammering out the design, I found no appropriate
place in the QOM hierachy where to stick these objects so we dropped the
idea.

If I get your proposal right you suggest to leave it unattached instead.
That should probably work; however, looking at all the boilerplate code
this would entail, including OBJECT casts, I'm not sure it would save
anything.  Do you think it's worth reworking into QOM?

Thanks,
Roman.

On Wed, Jun 14, 2017 at 07:23:56PM +0300, Roman Kagan wrote:
> On Wed, Jun 14, 2017 at 10:53:25AM -0300, Eduardo Habkost wrote:
> > On Tue, Jun 06, 2017 at 09:19:37PM +0300, Roman Kagan wrote:
> > > Multiple entities (e.g. VMBus devices) can use the same SINT route.  To
> > > make their lives easier in maintaining SINT route ownership, make it
> > > reference-counted.  Adjust the respective API names accordingly.
> > > 
> > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > 
> > Isn't it easier to reuse existing refcounting infrastructure
> > here?  Is it overkill to make it a QOM object? (struct Object has
> > 40 bytes)
> 
> Normally the guests use a sint route per cpu or less.  So no, the space
> overhead is not an issue.
> 
> I also wanted to reuse regular QOM refcounting so I QOM-ified it at
> first.  However, while hammering out the design, I found no appropriate
> place in the QOM hierachy where to stick these objects so we dropped the
> idea.
> 
> If I get your proposal right you suggest to leave it unattached instead.
> That should probably work; however, looking at all the boilerplate code
> this would entail, including OBJECT casts, I'm not sure it would save
> anything.  Do you think it's worth reworking into QOM?

I just noticed I didn't reply to this, sorry.  I'm also not sure it is
worth reworking into QOM, so I guess it's up to you.

About placing the object in the QOM hierarchy, I don't think that would
be a problem.  Objects without a parent are safer, as long as reference
counting is properly tracked.

About the OBJECT casts, I guess that's a common issue.  I'm considering
proposing making object_ref()/object_unref() macros that use OBJECT()
automatically.

-- 
Eduardo