[Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i8259.c |   32 ++++++++++++++++++++++++++------
 1 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 40f8bee..6c8b5c5 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -25,6 +25,7 @@
 #include "pc.h"
 #include "isa.h"
 #include "monitor.h"
+#include "sysbus.h"
 
 /* debug PIC */
 //#define DEBUG_PIC
@@ -54,6 +55,7 @@ typedef struct PicState {
 } PicState;
 
 struct PicState2 {
+    SysBusDevice busdev;
     /* 0 is master pic, 1 is slave pic */
     /* XXX: better separation between the two pics */
     PicState pics[2];
@@ -182,7 +184,8 @@ int64_t irq_time[16];
 
 static void i8259_set_irq(void *opaque, int irq, int level)
 {
-    PicState2 *s = opaque;
+    DeviceState *dev = opaque;
+    PicState2 *s = FROM_SYSBUS(PicState2, sysbus_from_qdev(dev));
 
 #if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
     if (level != irq_level[irq]) {
@@ -546,20 +549,37 @@ void irq_info(Monitor *mon)
 #endif
 }
 
-qemu_irq *i8259_init(qemu_irq parent_irq)
+static void i8259_initfn(SysBusDevice *sysdev)
 {
     PicState2 *s;
 
-    s = qemu_mallocz(sizeof(PicState2));
+    s = FROM_SYSBUS(PicState2, sysdev);
     pic_init1(0x20, 0x4d0, &s->pics[0]);
     pic_init1(0xa0, 0x4d1, &s->pics[1]);
     s->pics[0].elcr_mask = 0xf8;
     s->pics[1].elcr_mask = 0xde;
-    s->parent_irq = parent_irq;
     s->pics[0].pics_state = s;
     s->pics[1].pics_state = s;
-    isa_pic = s;
-    return qemu_allocate_irqs(i8259_set_irq, s, 16);
+    qdev_init_gpio_out(&sysdev->qdev, &s->parent_irq, 1);
+    qdev_init_gpio_in(&sysdev->qdev, i8259_set_irq, 16);
+}
+
+static void i8259_register(void)
+{
+    sysbus_register_dev("i8259", sizeof(PicState2), i8259_initfn);
+}
+device_init(i8259_register);
+
+qemu_irq *i8259_init(qemu_irq parent_irq)
+{
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, "i8259");
+    qdev_init(dev);
+
+    isa_pic = FROM_SYSBUS(PicState2, sysbus_from_qdev(dev));
+    qdev_connect_gpio_out(dev, 0, parent_irq);
+    return dev->gpio_in;
 }
 
 void pic_set_alt_irq_func(PicState2 *s, SetIRQFunc *alt_irq_func,
-- 
1.6.2.2

[Qemu-devel] [PATCH 2/3] qdev: irq allocation in generic code.

[Gerd Hoffmann]

Almost all devices have a static number of irqs, so we can easily move
the setup into generic code by adding some fields to DeviceInfo.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c |    9 +++++++++
 hw/qdev.h |    4 ++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 9fd22cf..c70ae8f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -99,6 +99,15 @@ DeviceState *qdev_create(BusState *bus, const char *name)
     }
     dev->parent_bus = bus;
     LIST_INSERT_HEAD(&bus->children, dev, sibling);
+
+    if (t->info->num_gpio_in) {
+        qdev_init_gpio_in(dev, t->info->irq, t->info->num_gpio_in);
+    }
+    if (t->info->num_gpio_out) {
+        qemu_irq *irqs = qemu_mallocz(sizeof(irqs[0]) * t->info->num_gpio_out);
+        qdev_init_gpio_out(dev, irqs, t->info->num_gpio_out);
+    }
+
     return dev;
 }
 
diff --git a/hw/qdev.h b/hw/qdev.h
index 7b523b6..bed3d6a 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -82,6 +82,10 @@ struct DeviceInfo {
     const char *name;
     size_t size;
 
+    int num_gpio_in;
+    int num_gpio_out;
+    qemu_irq_handler irq;
+
     /* private to qdev / bus */
     qdev_initfn init;
     BusType bus_type;
-- 
1.6.2.2

[Qemu-devel] [PATCH 3/3] qdev: switch isa pic to generic irq allocation.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i8259.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 6c8b5c5..4f73acc 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -59,7 +59,6 @@ struct PicState2 {
     /* 0 is master pic, 1 is slave pic */
     /* XXX: better separation between the two pics */
     PicState pics[2];
-    qemu_irq parent_irq;
     void *irq_request_opaque;
     /* IOAPIC callback support */
     SetIRQFunc *alt_irq_func;
@@ -167,13 +166,13 @@ void pic_update_irq(PicState2 *s)
         }
         printf("pic: cpu_interrupt\n");
 #endif
-        qemu_irq_raise(s->parent_irq);
+        qemu_irq_raise(s->busdev.qdev.gpio_out[0]);
     }
 
 /* all targets should do this rather than acking the IRQ in the cpu */
 #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA)
     else {
-        qemu_irq_lower(s->parent_irq);
+        qemu_irq_lower(s->busdev.qdev.gpio_out[0]);
     }
 #endif
 }
@@ -299,7 +298,7 @@ static void pic_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             /* init */
             pic_reset(s);
             /* deassert a pending interrupt */
-            qemu_irq_lower(s->pics_state->parent_irq);
+            qemu_irq_lower(s->pics_state->busdev.qdev.gpio_out[0]);
             s->init_state = 1;
             s->init4 = val & 1;
             s->single_mode = val & 2;
@@ -560,13 +559,22 @@ static void i8259_initfn(SysBusDevice *sysdev)
     s->pics[1].elcr_mask = 0xde;
     s->pics[0].pics_state = s;
     s->pics[1].pics_state = s;
-    qdev_init_gpio_out(&sysdev->qdev, &s->parent_irq, 1);
-    qdev_init_gpio_in(&sysdev->qdev, i8259_set_irq, 16);
 }
 
+static SysBusDeviceInfo i8259_info = {
+    .qdev.name = "i8259",
+    .qdev.size = sizeof(PicState2),
+
+    .qdev.num_gpio_in  = 16,
+    .qdev.num_gpio_out = 1,
+    .qdev.irq          = i8259_set_irq,
+
+    .init      = i8259_initfn,
+};
+
 static void i8259_register(void)
 {
-    sysbus_register_dev("i8259", sizeof(PicState2), i8259_initfn);
+    sysbus_register_withprop(&i8259_info);
 }
 device_init(i8259_register);
 
-- 
1.6.2.2

Re: [Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Paul Brook]

> +    qdev_init_gpio_out(&sysdev->qdev, &s->parent_irq, 1);

This should be using sysbus_init_irq.

Paul

Re: [Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Gerd Hoffmann]

On 06/09/09 17:01, Paul Brook wrote:
>> +    qdev_init_gpio_out(&sysdev->qdev,&s->parent_irq, 1);
>
> This should be using sysbus_init_irq.
>
> Paul

Patch 3/3 kills that line anyway ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Paul Brook]

On Tuesday 09 June 2009, Gerd Hoffmann wrote:
> On 06/09/09 17:01, Paul Brook wrote:
> >> +    qdev_init_gpio_out(&sysdev->qdev,&s->parent_irq, 1);
> >
> > This should be using sysbus_init_irq.
> >
> > Paul
>
> Patch 3/3 kills that line anyway ...

It's still wrong.

Paul

Re: [Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Gerd Hoffmann]

On 06/09/09 17:32, Paul Brook wrote:
> On Tuesday 09 June 2009, Gerd Hoffmann wrote:
>> On 06/09/09 17:01, Paul Brook wrote:
>>>> +    qdev_init_gpio_out(&sysdev->qdev,&s->parent_irq, 1);
>>> This should be using sysbus_init_irq.
>>>
>>> Paul
>> Patch 3/3 kills that line anyway ...
>
> It's still wrong.

Why doesn't sysbus_init_irq() call qdev_init_gpio_out()?

What is the point in maintaining qemu_irqs in SysBusDevice directly and 
ignoring the qemu_irqs in DeviceState (aka SysBusDevice.qdev)?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Paul Brook]

On Tuesday 09 June 2009, Gerd Hoffmann wrote:
> On 06/09/09 17:32, Paul Brook wrote:
> > On Tuesday 09 June 2009, Gerd Hoffmann wrote:
> >> On 06/09/09 17:01, Paul Brook wrote:
> >>>> +    qdev_init_gpio_out(&sysdev->qdev,&s->parent_irq, 1);
> >>>
> >>> This should be using sysbus_init_irq.
> >>>
> >>> Paul
> >>
> >> Patch 3/3 kills that line anyway ...
> >
> > It's still wrong.
>
> Why doesn't sysbus_init_irq() call qdev_init_gpio_out()?

Because some devices have both IRQs and GPIO outputs.

Paul

Re: [Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Gerd Hoffmann]

On 06/09/09 17:58, Paul Brook wrote:

>> Why doesn't sysbus_init_irq() call qdev_init_gpio_out()?
>
> Because some devices have both IRQs and GPIO outputs.

What is the point of this commit here ...

http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=067a3ddc8876cee8451d6f690a051e413a593fdc

then?

In general I think we should handle as much as possible at DeviceState / 
DeviceInfo level.  Stuff which devices commonly have should live there: 
  IRQs, mmio, ioports, ... in DeviceState.  name, init and other generic 
callbacks, ... in DeviceInfo.

The bus structs should only hold stuff which is actually specific to 
that bus.  That is probably almost nothing for sysbus.  i2c has the xfer 
callbacks in I2CSlaveInfo.  Likewise pci can have the config space 
read/write callbacks in PCIDeviceInfo.

With more and more stuff coming to DeviceInfo it probably becomes more 
useful to have the device register functions just pass a DeviceInfo 
struct (or a bus-specific one like PCIDeviceInfo with DeviceInfo as 
element) to the register function, otherwise we'll end up with an 
ever-growing list of arguments ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Filip Navara]

On Wed, Jun 10, 2009 at 9:32 AM, Gerd Hoffmann<kraxel@redhat.com> wrote:
[snip]>
> In general I think we should handle as much as possible at DeviceState /
> DeviceInfo level.  Stuff which devices commonly have should live there:
>  IRQs, mmio, ioports, ... in DeviceState.  name, init and other generic
> callbacks, ... in DeviceInfo.
>
> The bus structs should only hold stuff which is actually specific to that
> bus.  That is probably almost nothing for sysbus.  i2c has the xfer
> callbacks in I2CSlaveInfo.  Likewise pci can have the config space
> read/write callbacks in PCIDeviceInfo.

This is definitely based on wrong assumptions. I've GPIO devices
modelled on top of qdev and they don't know anything about IRQs, MMIO
or stuff like that. All they know about is that there are few in/out
GPIO pins, which are connected to the GPIO controller in the emulated
microcontroller.

Best regards,
Filip Navara

Re: [Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Gerd Hoffmann]

On 06/10/09 11:43, Filip Navara wrote:
> On Wed, Jun 10, 2009 at 9:32 AM, Gerd Hoffmann<kraxel@redhat.com>  wrote:
> [snip]>
>> In general I think we should handle as much as possible at DeviceState /
>> DeviceInfo level.  Stuff which devices commonly have should live there:
>>   IRQs, mmio, ioports, ... in DeviceState.  name, init and other generic
>> callbacks, ... in DeviceInfo.
>>
>> The bus structs should only hold stuff which is actually specific to that
>> bus.  That is probably almost nothing for sysbus.  i2c has the xfer
>> callbacks in I2CSlaveInfo.  Likewise pci can have the config space
>> read/write callbacks in PCIDeviceInfo.
>
> This is definitely based on wrong assumptions. I've GPIO devices
> modelled on top of qdev and they don't know anything about IRQs, MMIO
> or stuff like that. All they know about is that there are few in/out
> GPIO pins, which are connected to the GPIO controller in the emulated
> microcontroller.

Sure, not every device has IRQs.  Nevertheless almost every bus out 
there supports IRQs.  Thus it is IMHO pointless to have a common thing 
duplicated in each end every bus implementation, it should be in the 
most basic type instead.

That of course doesn't imply that every device must actually use them.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/3] qdev-ify isa pic

[Paul Brook]

On Wednesday 10 June 2009, Gerd Hoffmann wrote:
> On 06/10/09 11:43, Filip Navara wrote:
> > On Wed, Jun 10, 2009 at 9:32 AM, Gerd Hoffmann<kraxel@redhat.com>  wrote:
> > [snip]>
> >
> >> In general I think we should handle as much as possible at DeviceState /
> >> DeviceInfo level.  Stuff which devices commonly have should live there:
> >>   IRQs, mmio, ioports, ... in DeviceState.  name, init and other generic
> >> callbacks, ... in DeviceInfo.
> >>
> >> The bus structs should only hold stuff which is actually specific to
> >> that bus.  That is probably almost nothing for sysbus.  i2c has the xfer
> >> callbacks in I2CSlaveInfo.  Likewise pci can have the config space
> >> read/write callbacks in PCIDeviceInfo.
> >
> > This is definitely based on wrong assumptions. I've GPIO devices
> > modelled on top of qdev and they don't know anything about IRQs, MMIO
> > or stuff like that. All they know about is that there are few in/out
> > GPIO pins, which are connected to the GPIO controller in the emulated
> > microcontroller.
>
> Sure, not every device has IRQs.  Nevertheless almost every bus out
> there supports IRQs.  Thus it is IMHO pointless to have a common thing
> duplicated in each end every bus implementation, it should be in the
> most basic type instead.

My initial implementation tried to push IRQ and MMIO handling into 
DeviceState, and it didn't fit well.

PCI v.s. SysBus is a good example here.  Both have IRQs and MMIO regions. 
However the way these are configured and exposed to devices is very different. 
In practice you end up needing per-bus wrappers/hooks, and there's very little 
useful common code.

For things that are truly bus agnostic (or independent of the primary bus, 
e.g. GPIO pins) pushing up to the qdev level makes sense.

Paul