| Message ID | e154494f9bc17c230b488cd8fbd6883c8e63529f.1400204799.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Fri, May 16, 2014 at 11:56 AM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > From: Andreas Färber <afaerber@suse.de> > > 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 <afaerber@suse.de> > --- > > 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
Am 19.05.2014 03:52, schrieb Peter Crosthwaite: > On Fri, May 16, 2014 at 11:56 AM, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> From: Andreas Färber <afaerber@suse.de> >> >> 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 <afaerber@suse.de> (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
On Mon, May 19, 2014 at 7:13 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 19.05.2014 03:52, schrieb Peter Crosthwaite: >> On Fri, May 16, 2014 at 11:56 AM, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> From: Andreas Färber <afaerber@suse.de> >>> >>> 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 <afaerber@suse.de> > > (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 >
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); + 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] = p; p++; @@ -72,7 +97,7 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n) { struct IRQState *irq; - irq = g_new(struct IRQState, 1); + irq = IRQ(object_new(TYPE_IRQ)); irq->handler = handler; irq->opaque = opaque; irq->n = n; @@ -82,13 +107,13 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n) void qemu_free_irqs(qemu_irq *s) { - g_free(s[0]); + object_unref(OBJECT(s[0])); g_free(s); } void qemu_free_irq(qemu_irq irq) { - g_free(irq); + object_unref(OBJECT(irq)); } static void qemu_notirq(void *opaque, int line, int level) @@ -150,3 +175,16 @@ void qemu_irq_intercept_out(qemu_irq **gpio_out, qemu_irq_handler handler, int n qemu_irq *old_irqs = *gpio_out; *gpio_out = qemu_allocate_irqs(handler, old_irqs, n); } + +static const TypeInfo irq_type_info = { + .name = TYPE_IRQ, + .parent = TYPE_OBJECT, + .instance_size = sizeof(struct IRQState), +}; + +static void irq_register_types(void) +{ + type_register_static(&irq_type_info); +} + +type_init(irq_register_types) diff --git a/include/hw/irq.h b/include/hw/irq.h index d08bc02..ac76ea7 100644 --- a/include/hw/irq.h +++ b/include/hw/irq.h @@ -3,6 +3,8 @@ /* Generic IRQ/GPIO pin infrastructure. */ +#define TYPE_IRQ "irq" + typedef struct IRQState *qemu_irq; typedef void (*qemu_irq_handler)(void *opaque, int n, int level);