qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [RFC v1 11/25] irq: Slim conversion of qemu_irq to QOM [WIP]
Date: Mon, 19 May 2014 20:22:47 +1000

On Mon, May 19, 2014 at 7:13 PM, Andreas Färber <address@hidden> wrote:
> 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

Well I introduce the issue 13-15 of this series. They are dependant on
pointer stability.

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

Understand. I have seen your predecessor patches in your branch. I
have left out such patches from this RFC (there are others like it for
other features) to avoid a monster series - this is really just for
comments on the whole idea. I probably already have a TLDR problem
here so i'm trying to not make it worse!

 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.

There are so few call sites, they can probably just keep track of
numbers and pass the count to the freer. That or removal as you have
started in your short series.

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

Working on it. Left that issue alone just to get something working
today. By v2 I mean a v2 RFC :)

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

Ok. Thanks for the review.

Regards,
Peter

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]