Re: [Qemu-devel] [RFC v1 11/25] irq: Slim conversion of qemu_irq to QOM [WIP]

[Peter Crosthwaite]

On Fri, May 16, 2014 at 11:56 AM, Peter Crosthwaite
<address@hidden> wrote:
> From: Andreas Färber <address@hidden>
>
> As a prequel to any big Pin refactoring plans, do an in-place conversion
> of qemu_irq to an Object, so that we can reference it in link<> properties.
>
> Signed-off-by: Andreas Färber <address@hidden>
> ---
>
>  hw/core/irq.c    | 44 +++++++++++++++++++++++++++++++++++++++++---
>  include/hw/irq.h |  2 ++
>  2 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/irq.c b/hw/core/irq.c
> index 03c8cb3..0bcd27b 100644
> --- a/hw/core/irq.c
> +++ b/hw/core/irq.c
> @@ -23,8 +23,13 @@
>   */
>  #include "qemu-common.h"
>  #include "hw/irq.h"
> +#include "qom/object.h"
> +
> +#define IRQ(obj) OBJECT_CHECK(struct IRQState, (obj), TYPE_IRQ)
>
>  struct IRQState {
> +    Object parent_obj;
> +
>      qemu_irq_handler handler;
>      void *opaque;
>      int n;
> @@ -38,6 +43,14 @@ void qemu_set_irq(qemu_irq irq, int level)
>      irq->handler(irq->opaque, irq->n, level);
>  }
>
> +static void irq_nonfirst_free(void *obj)
> +{
> +    struct IRQState *s = obj;
> +
> +    /* Unreference the first IRQ in this array */
> +    object_unref(OBJECT(s - s->n));
> +}
> +
>  qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler 
> handler,
>                             void *opaque, int n)
>  {
> @@ -51,11 +64,23 @@ qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, 
> qemu_irq_handler handler,
>      s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
>      p = old ? g_renew(struct IRQState, s[0], n + n_old) :
>                  g_new(struct IRQState, n);

So using g_renew on the actual object storage is very fragile, as it
means you cannot reliably take pointers to the objects. I think
however this whole issue could be simplified by de-arrayifying the
whole thing, and treating IRQs individually (interdiff):

 qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
                            void *opaque, int n)
 {
     qemu_irq *s;
-    struct IRQState *p;
     int i;

     if (!old) {
         n_old = 0;
     }
     s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
-    p = old ? g_renew(struct IRQState, s[0], n + n_old) :
-                g_new(struct IRQState, n);
-    memset(p + n_old, 0, n * sizeof(*p));
     for (i = 0; i < n + n_old; i++) {
         if (i >= n_old) {
-            Object *obj;
-
-            object_initialize(p, sizeof(*p), TYPE_IRQ);
-            p->handler = handler;
-            p->opaque = opaque;
-            p->n = i;
-            obj = OBJECT(p);
-            /* Let the first IRQ clean them all up */
-            if (unlikely(i == 0)) {
-                obj->free = g_free;
-            } else {
-                object_ref(OBJECT(s[0]));
-                obj->free = irq_nonfirst_free;
-            }
+            s[i] = qemu_allocate_irq(handler, opaque, i);
         }
-        s[i] = p;
-        p++;
     }
     return s;

The system for freeing may need some thought, and I wonder if their
are API clients out there assuming the IRQ storage elements are
contiguous (if there are they have too much internal knowledge and
need to be fixed).

But with this change these objects are now usable with links and canon
paths etc.

Will roll into V2 of this patch.

Regards,
Peter