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

[Andreas Färber]

Am 19.05.2014 03:52, schrieb 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>

(Note that you forgot to sign off here.)

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

Negative!

I was well aware of the two g_renew()s, one of which affects the
objects. However I figured, as long as no one has a pointer to those
objects it should just continue work (otherwise the pre-QOM pointers
would've broken, too -> separate issue) - and so far I haven't found a
test case that doesn't work as is.

So while I agree that we should refactor this, your series does iirc not
include my genuine qemu_irq* fixes either, and I reported at least two
callers of qemu_free_irqs() remaining, in serial-pci.c among others, so
your diff above is not yet okay - it would leak any non-first IRQState.
Which is why I do these odd object_{ref,unref}() tricks -
qemu_free_irqs() cannot tell how many IRQs there are to be freed. And
while your use of qemu_allocate_irq() gives them the normal oc->free =
g_free for freeing them individually, you do not unref them anywhere, so
it will never happen.

Instead of squashing this into my patch I suggest you build upon master
plus my first two cleanup patches and get rid of the remaining
qemu_free_irqs()s altogether, then we can much more sanely refactor this
code and get it in shortly.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg