[Qemu-devel] [PATCH 1/2] pxa2xx_timer: separate irq for pxa27x handling

[Qemu-devel] [PATCH 1/2] pxa2xx_timer: separate irq for pxa27x handling

[Dmitry Eremin-Solenikov]

First, sysbus_init_irq shan't be called on on-stack variables. Indeed,
it only stores a passed pointer in qdev and the stored irq is later
populated, so we get a nice write-to-stack bug.
Second, irq for pxa27x should probably be handled in a more gentler way,
as we should check if we have events to raise this irq.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 hw/pxa2xx_timer.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/pxa2xx_timer.c b/hw/pxa2xx_timer.c
index 0fbd4a8..effe653 100644
--- a/hw/pxa2xx_timer.c
+++ b/hw/pxa2xx_timer.c
@@ -94,6 +94,7 @@ struct PXA2xxTimerInfo {
     uint32_t reset3;
     uint32_t snapshot;
 
+    qemu_irq irq4;
     PXA2xxTimer4 tm4[8];
 };
 
@@ -289,7 +290,7 @@ static void pxa2xx_timer_write(void *opaque, target_phys_addr_t offset,
                 if (s->tm4[i].tm.level && (value & 1))
                     s->tm4[i].tm.level = 0;
             if (!(s->events & 0xff0))
-                qemu_irq_lower(s->tm4->tm.irq);
+                qemu_irq_lower(s->irq4);
         }
         break;
     case OWER:	/* XXX: Reset on OSMR3 match? */
@@ -372,6 +373,8 @@ static void pxa2xx_timer_tick4(void *opaque)
         t->clock = 0;
     if (t->control & (1 << 6))
         pxa2xx_timer_update4(i, qemu_get_clock(vm_clock), t->tm.num - 4);
+    if (i->events & 0xff0)
+        qemu_irq_raise(i->irq4);
 }
 
 static int pxa25x_timer_post_load(void *opaque, int version_id)
@@ -395,7 +398,6 @@ static int pxa2xx_timer_init(SysBusDevice *dev)
     int i;
     int iomemtype;
     PXA2xxTimerInfo *s;
-    qemu_irq irq4;
 
     s = FROM_SYSBUS(PXA2xxTimerInfo, dev);
     s->irq_enabled = 0;
@@ -414,7 +416,7 @@ static int pxa2xx_timer_init(SysBusDevice *dev)
                         pxa2xx_timer_tick, &s->timer[i]);
     }
     if (s->flags & (1 << PXA2XX_TIMER_HAVE_TM4)) {
-        sysbus_init_irq(dev, &irq4);
+        sysbus_init_irq(dev, &s->irq4);
 
         for (i = 0; i < 8; i ++) {
             s->tm4[i].tm.value = 0;
@@ -425,7 +427,6 @@ static int pxa2xx_timer_init(SysBusDevice *dev)
             s->tm4[i].control = 0x0;
             s->tm4[i].tm.qtimer = qemu_new_timer(vm_clock,
                         pxa2xx_timer_tick4, &s->tm4[i]);
-            s->tm4[i].tm.irq = irq4;
         }
     }
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/2] pxa2xx_pic: fixup initialisation

[Dmitry Eremin-Solenikov]

Currently pxa2xx_pic init fails with error at sysbus_mmio_map(),
as there is no correspondent sysbus_init_mmio. Also move most of init to
pxa2xx_pic_initfn.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 hw/pxa2xx_pic.c |   37 +++++++++++++++++++------------------
 1 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/hw/pxa2xx_pic.c b/hw/pxa2xx_pic.c
index 86c0ee6..65edfd6 100644
--- a/hw/pxa2xx_pic.c
+++ b/hw/pxa2xx_pic.c
@@ -251,12 +251,25 @@ static int pxa2xx_pic_post_load(void *opaque, int version_id)
 
 DeviceState *pxa2xx_pic_init(target_phys_addr_t base, CPUState *env)
 {
-    DeviceState *dev = qdev_create(NULL, "pxa2xx_pic");
-    int iomemtype;
-    PXA2xxPICState *s = FROM_SYSBUS(PXA2xxPICState, sysbus_from_qdev(dev));
+    DeviceState *dev;
+    PXA2xxPICState *s;
+
+    dev = sysbus_create_varargs("pxa2xx_pic", base, NULL);
+    s = FROM_SYSBUS(PXA2xxPICState, sysbus_from_qdev(dev));
 
     s->cpu_env = env;
 
+    /* Enable IC coprocessor access.  */
+    cpu_arm_set_cp_io(env, 6, pxa2xx_pic_cp_read, pxa2xx_pic_cp_write, s);
+
+    return dev;
+}
+
+static int pxa2xx_pic_initfn(SysBusDevice *dev)
+{
+    PXA2xxPICState *s = FROM_SYSBUS(PXA2xxPICState, dev);
+    int iomemtype;
+
     s->int_pending[0] = 0;
     s->int_pending[1] = 0;
     s->int_enabled[0] = 0;
@@ -264,21 +277,14 @@ DeviceState *pxa2xx_pic_init(target_phys_addr_t base, CPUState *env)
     s->is_fiq[0] = 0;
     s->is_fiq[1] = 0;
 
-    qdev_init_nofail(dev);
-
-    qdev_init_gpio_in(dev, pxa2xx_pic_set_irq, PXA2XX_PIC_SRCS);
-
-    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
+    qdev_init_gpio_in(&dev->qdev, pxa2xx_pic_set_irq, PXA2XX_PIC_SRCS);
 
     /* Enable IC memory-mapped registers access.  */
     iomemtype = cpu_register_io_memory(pxa2xx_pic_readfn,
                     pxa2xx_pic_writefn, s, DEVICE_NATIVE_ENDIAN);
-    sysbus_init_mmio(sysbus_from_qdev(dev), 0x00100000, iomemtype);
-
-    /* Enable IC coprocessor access.  */
-    cpu_arm_set_cp_io(env, 6, pxa2xx_pic_cp_read, pxa2xx_pic_cp_write, s);
+    sysbus_init_mmio(dev, 0x00100000, iomemtype);
 
-    return dev;
+    return 0;
 }
 
 static VMStateDescription vmstate_pxa2xx_pic_regs = {
@@ -297,11 +303,6 @@ static VMStateDescription vmstate_pxa2xx_pic_regs = {
     },
 };
 
-static int pxa2xx_pic_initfn(SysBusDevice *dev)
-{
-    return 0;
-}
-
 static SysBusDeviceInfo pxa2xx_pic_info = {
     .init       = pxa2xx_pic_initfn,
     .qdev.name  = "pxa2xx_pic",
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/2] pxa2xx_timer: separate irq for pxa27x handling

[andrzej zaborowski]

On 4 March 2011 01:40, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote:
> First, sysbus_init_irq shan't be called on on-stack variables. Indeed,
> it only stores a passed pointer in qdev and the stored irq is later
> populated, so we get a nice write-to-stack bug.
> Second, irq for pxa27x should probably be handled in a more gentler way,
> as we should check if we have events to raise this irq.

Thanks, pushed this patch and sorry for breaking the code.  In the
second patch I decided against splitting the device initialisation
between pxa2xx_pic_init and pxa2xx_pic_initfn for clarity, it seems
the round way and just makes the code more tricky.

I also pushed the mainstone PCMCIA changes.

Cheers