[Qemu-devel] [PATCH 0/2] Type-safe ioport callbacks

[Qemu-devel] [PATCH 0/2] Type-safe ioport callbacks

[Avi Kivity]

A recent qemu -> qemu-kvm merge broke cpu hotplug without the compiler
complaining because of the type-unsafeness of the ioport callbacks.  This
patchset adds a type-safe variant of ioport callbacks and coverts a sample
ioport.  Converting the other 300-odd registrations is left as an excercise
to the community.

Avi Kivity (2):
  Type-safe ioport callbacks
  piix4 acpi: convert io BAR to type-safe ioport callbacks

 hw/acpi_piix4.c |   30 +++++++++++++++----------
 ioport.c        |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ioport.h        |   16 +++++++++++++
 3 files changed, 98 insertions(+), 12 deletions(-)

-- 
1.7.3.1

[Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Avi Kivity]

The current ioport callbacks are not type-safe, in that they accept an "opaque"
pointer as an argument whose type must match the argument to the registration
function; this is not checked by the compiler.

This patch adds an alternative that is type-safe.  Instead of an opaque
argument, both registation and the callback use a new IOPort type.  The
callback then uses container_of() to access its main structures.

Currently the old and new methods exist side by side; once the old way is gone,
we can also save a bunch of memory since the new method requires one pointer
per ioport instead of 6.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 ioport.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ioport.h |   16 +++++++++++++++
 2 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/ioport.c b/ioport.c
index ec3dc65..47eafc3 100644
--- a/ioport.c
+++ b/ioport.c
@@ -174,6 +174,70 @@ int register_ioport_write(pio_addr_t start, int length, int size,
     return 0;
 }
 
+static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
+{
+    IOPort *ioport = opaque;
+
+    return ioport->ops->readb(ioport, addr);
+}
+
+static uint32_t ioport_readw_thunk(void *opaque, uint32_t addr)
+{
+    IOPort *ioport = opaque;
+
+    return ioport->ops->readw(ioport, addr);
+}
+
+static uint32_t ioport_readl_thunk(void *opaque, uint32_t addr)
+{
+    IOPort *ioport = opaque;
+
+    return ioport->ops->readl(ioport, addr);
+}
+
+static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data)
+{
+    IOPort *ioport = opaque;
+
+    return ioport->ops->writeb(ioport, addr, data);
+}
+
+static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data)
+{
+    IOPort *ioport = opaque;
+
+    return ioport->ops->writew(ioport, addr, data);
+}
+
+static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data)
+{
+    IOPort *ioport = opaque;
+
+    return ioport->ops->writel(ioport, addr, data);
+}
+
+void ioport_register(IOPort *ioport, pio_addr_t start, pio_addr_t length)
+{
+    if (ioport->ops->readb) {
+        register_ioport_read(start, length, 1, ioport_readb_thunk, ioport);
+    }
+    if (ioport->ops->readw) {
+        register_ioport_read(start, length, 2, ioport_readw_thunk, ioport);
+    }
+    if (ioport->ops->readl) {
+        register_ioport_read(start, length, 4, ioport_readl_thunk, ioport);
+    }
+    if (ioport->ops->writeb) {
+        register_ioport_write(start, length, 1, ioport_writeb_thunk, ioport);
+    }
+    if (ioport->ops->writew) {
+        register_ioport_write(start, length, 2, ioport_writew_thunk, ioport);
+    }
+    if (ioport->ops->writel) {
+        register_ioport_write(start, length, 4, ioport_writel_thunk, ioport);
+    }
+}
+
 void isa_unassign_ioport(pio_addr_t start, int length)
 {
     int i;
diff --git a/ioport.h b/ioport.h
index 3d3c8a3..8036e59 100644
--- a/ioport.h
+++ b/ioport.h
@@ -36,6 +36,22 @@ typedef uint32_t pio_addr_t;
 typedef void (IOPortWriteFunc)(void *opaque, uint32_t address, uint32_t data);
 typedef uint32_t (IOPortReadFunc)(void *opaque, uint32_t address);
 
+struct IOPort;
+
+typedef struct IOPortOps {
+    uint32_t (*readb)(struct IOPort *ioport, pio_addr_t addr);
+    uint32_t (*readw)(struct IOPort *ioport, pio_addr_t addr);
+    uint32_t (*readl)(struct IOPort *ioport, pio_addr_t addr);
+    void (*writeb)(struct IOPort *ioport, pio_addr_t addr, uint32_t data);
+    void (*writew)(struct IOPort *ioport, pio_addr_t addr, uint32_t data);
+    void (*writel)(struct IOPort *ioport, pio_addr_t addr, uint32_t data);
+} IOPortOps;
+
+typedef struct IOPort {
+    IOPortOps *ops;
+} IOPort;
+
+void ioport_register(IOPort *ioport, pio_addr_t start, pio_addr_t length);
 int register_ioport_read(pio_addr_t start, int length, int size,
                          IOPortReadFunc *func, void *opaque);
 int register_ioport_write(pio_addr_t start, int length, int size,
-- 
1.7.3.1

[Qemu-devel] [PATCH 2/2] piix4 acpi: convert io BAR to type-safe ioport callbacks

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/acpi_piix4.c |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index f74f34c..5772667 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -52,6 +52,7 @@ struct pci_status {
 
 typedef struct PIIX4PMState {
     PCIDevice dev;
+    IOPort ioport;
     uint16_t pmsts;
     uint16_t pmen;
     uint16_t pmcntrl;
@@ -128,9 +129,9 @@ static void pm_tmr_timer(void *opaque)
     pm_update_sci(s);
 }
 
-static void pm_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
+static void pm_ioport_writew(IOPort *ioport, pio_addr_t addr, uint32_t val)
 {
-    PIIX4PMState *s = opaque;
+    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
     addr &= 0x3f;
     switch(addr) {
     case 0x00:
@@ -184,9 +185,9 @@ static void pm_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
     PIIX4_DPRINTF("PM writew port=0x%04x val=0x%04x\n", addr, val);
 }
 
-static uint32_t pm_ioport_readw(void *opaque, uint32_t addr)
+static uint32_t pm_ioport_readw(IOPort *ioport, pio_addr_t addr)
 {
-    PIIX4PMState *s = opaque;
+    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
     uint32_t val;
 
     addr &= 0x3f;
@@ -208,15 +209,15 @@ static uint32_t pm_ioport_readw(void *opaque, uint32_t addr)
     return val;
 }
 
-static void pm_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
+static void pm_ioport_writel(IOPort *ioport, pio_addr_t addr, uint32_t val)
 {
-    //    PIIX4PMState *s = opaque;
+    //    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
     PIIX4_DPRINTF("PM writel port=0x%04x val=0x%08x\n", addr & 0x3f, val);
 }
 
-static uint32_t pm_ioport_readl(void *opaque, uint32_t addr)
+static uint32_t pm_ioport_readl(IOPort *ioport, uint32_t addr)
 {
-    PIIX4PMState *s = opaque;
+    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
     uint32_t val;
 
     addr &= 0x3f;
@@ -265,10 +266,7 @@ static void pm_io_space_update(PIIX4PMState *s)
 
         /* XXX: need to improve memory and ioport allocation */
         PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base);
-        register_ioport_write(pm_io_base, 64, 2, pm_ioport_writew, s);
-        register_ioport_read(pm_io_base, 64, 2, pm_ioport_readw, s);
-        register_ioport_write(pm_io_base, 64, 4, pm_ioport_writel, s);
-        register_ioport_read(pm_io_base, 64, 4, pm_ioport_readl, s);
+        ioport_register(&s->ioport, pm_io_base, 64);
     }
 }
 
@@ -413,6 +411,13 @@ static int piix4_pm_initfn(PCIDevice *dev)
     return 0;
 }
 
+static IOPortOps pm_ioport_ops = {
+    .readw = pm_ioport_readw,
+    .readl = pm_ioport_readl,
+    .writew = pm_ioport_writew,
+    .writel = pm_ioport_writel,
+};
+
 i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                        qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
                        int kvm_enabled)
@@ -424,6 +429,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     qdev_prop_set_uint32(&dev->qdev, "smb_io_base", smb_io_base);
 
     s = DO_UPCAST(PIIX4PMState, dev, dev);
+    s->ioport.ops = &pm_ioport_ops;
     s->irq = sci_irq;
     s->cmos_s3 = cmos_s3;
     s->smi_irq = smi_irq;
-- 
1.7.3.1

[Qemu-devel] Re: [PATCH 1/2] Type-safe ioport callbacks

[Avi Kivity]

  On 10/24/2010 05:34 PM, Avi Kivity wrote:
> Currently the old and new methods exist side by side; once the old way is gone,
> we can also save a bunch of memory since the new method requires one pointer
> per ioport instead of 6.

Actually, 1:7, we replace 3 read callbacks, 3 write callbacks, and 1 
opaque pointer with a single IOPort pointer.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/2] Type-safe ioport callbacks

[Paolo Bonzini]

On 10/24/2010 05:34 PM, Avi Kivity wrote:
> A recent qemu ->  qemu-kvm merge broke cpu hotplug without the compiler
> complaining because of the type-unsafeness of the ioport callbacks.  This
> patchset adds a type-safe variant of ioport callbacks and coverts a sample
> ioport.  Converting the other 300-odd registrations is left as an excercise
> to the community.

Should we create a Documentation/ file with incomplete transitions and 
the commit(s) that introduced them, for volunteers who wish to do some 
dirty work or to learn Coccinelle?

Paolo

[Qemu-devel] Re: [PATCH 0/2] Type-safe ioport callbacks

[Avi Kivity]

  On 10/24/2010 07:35 PM, Paolo Bonzini wrote:
> On 10/24/2010 05:34 PM, Avi Kivity wrote:
>> A recent qemu ->  qemu-kvm merge broke cpu hotplug without the compiler
>> complaining because of the type-unsafeness of the ioport callbacks.  
>> This
>> patchset adds a type-safe variant of ioport callbacks and coverts a 
>> sample
>> ioport.  Converting the other 300-odd registrations is left as an 
>> excercise
>> to the community.
>
> Should we create a Documentation/ file with incomplete transitions and 
> the commit(s) that introduced them, for volunteers who wish to do some 
> dirty work or to learn Coccinelle?

If we have a TODO, we could add a janitor section there.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Blue Swirl]

On Sun, Oct 24, 2010 at 3:34 PM, Avi Kivity <avi@redhat.com> wrote:
> The current ioport callbacks are not type-safe, in that they accept an "opaque"
> pointer as an argument whose type must match the argument to the registration
> function; this is not checked by the compiler.
>
> This patch adds an alternative that is type-safe.  Instead of an opaque
> argument, both registation and the callback use a new IOPort type.  The
> callback then uses container_of() to access its main structures.
>
> Currently the old and new methods exist side by side; once the old way is gone,
> we can also save a bunch of memory since the new method requires one pointer
> per ioport instead of 6.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>

If we are going to change the interface, let's do it so that it's
useful for other uses too:
http://article.gmane.org/gmane.comp.emulators.qemu/76984

> ---
>  ioport.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ioport.h |   16 +++++++++++++++
>  2 files changed, 80 insertions(+), 0 deletions(-)
>
> diff --git a/ioport.c b/ioport.c
> index ec3dc65..47eafc3 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -174,6 +174,70 @@ int register_ioport_write(pio_addr_t start, int length, int size,
>     return 0;
>  }
>
> +static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
> +{
> +    IOPort *ioport = opaque;
> +
> +    return ioport->ops->readb(ioport, addr);
> +}
> +
> +static uint32_t ioport_readw_thunk(void *opaque, uint32_t addr)
> +{
> +    IOPort *ioport = opaque;
> +
> +    return ioport->ops->readw(ioport, addr);
> +}
> +
> +static uint32_t ioport_readl_thunk(void *opaque, uint32_t addr)
> +{
> +    IOPort *ioport = opaque;
> +
> +    return ioport->ops->readl(ioport, addr);
> +}
> +
> +static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    IOPort *ioport = opaque;
> +
> +    return ioport->ops->writeb(ioport, addr, data);
> +}
> +
> +static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    IOPort *ioport = opaque;
> +
> +    return ioport->ops->writew(ioport, addr, data);
> +}
> +
> +static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    IOPort *ioport = opaque;
> +
> +    return ioport->ops->writel(ioport, addr, data);
> +}
> +
> +void ioport_register(IOPort *ioport, pio_addr_t start, pio_addr_t length)
> +{
> +    if (ioport->ops->readb) {
> +        register_ioport_read(start, length, 1, ioport_readb_thunk, ioport);
> +    }
> +    if (ioport->ops->readw) {
> +        register_ioport_read(start, length, 2, ioport_readw_thunk, ioport);
> +    }
> +    if (ioport->ops->readl) {
> +        register_ioport_read(start, length, 4, ioport_readl_thunk, ioport);
> +    }
> +    if (ioport->ops->writeb) {
> +        register_ioport_write(start, length, 1, ioport_writeb_thunk, ioport);
> +    }
> +    if (ioport->ops->writew) {
> +        register_ioport_write(start, length, 2, ioport_writew_thunk, ioport);
> +    }
> +    if (ioport->ops->writel) {
> +        register_ioport_write(start, length, 4, ioport_writel_thunk, ioport);
> +    }
> +}
> +
>  void isa_unassign_ioport(pio_addr_t start, int length)
>  {
>     int i;
> diff --git a/ioport.h b/ioport.h
> index 3d3c8a3..8036e59 100644
> --- a/ioport.h
> +++ b/ioport.h
> @@ -36,6 +36,22 @@ typedef uint32_t pio_addr_t;
>  typedef void (IOPortWriteFunc)(void *opaque, uint32_t address, uint32_t data);
>  typedef uint32_t (IOPortReadFunc)(void *opaque, uint32_t address);
>
> +struct IOPort;
> +
> +typedef struct IOPortOps {
> +    uint32_t (*readb)(struct IOPort *ioport, pio_addr_t addr);
> +    uint32_t (*readw)(struct IOPort *ioport, pio_addr_t addr);
> +    uint32_t (*readl)(struct IOPort *ioport, pio_addr_t addr);
> +    void (*writeb)(struct IOPort *ioport, pio_addr_t addr, uint32_t data);
> +    void (*writew)(struct IOPort *ioport, pio_addr_t addr, uint32_t data);
> +    void (*writel)(struct IOPort *ioport, pio_addr_t addr, uint32_t data);
> +} IOPortOps;
> +
> +typedef struct IOPort {
> +    IOPortOps *ops;

const

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Avi Kivity]

  On 10/24/2010 08:14 PM, Blue Swirl wrote:
> On Sun, Oct 24, 2010 at 3:34 PM, Avi Kivity<avi@redhat.com>  wrote:
> >  The current ioport callbacks are not type-safe, in that they accept an "opaque"
> >  pointer as an argument whose type must match the argument to the registration
> >  function; this is not checked by the compiler.
> >
> >  This patch adds an alternative that is type-safe.  Instead of an opaque
> >  argument, both registation and the callback use a new IOPort type.  The
> >  callback then uses container_of() to access its main structures.
> >
> >  Currently the old and new methods exist side by side; once the old way is gone,
> >  we can also save a bunch of memory since the new method requires one pointer
> >  per ioport instead of 6.
> >
> >  Signed-off-by: Avi Kivity<avi@redhat.com>
>
> If we are going to change the interface, let's do it so that it's
> useful for other uses too:
> http://article.gmane.org/gmane.comp.emulators.qemu/76984

I don't really see why we need registration; cpu_register_io() takes 
function pointers, a size, and an opaque, and gives an integer handle in 
return.  With the IOPort object approach, you set up the IOPort with 
function pointers, size is implied, and the opaque is derived using 
container_of(); the handle is simply the address of the object.

> >  +typedef struct IOPort {
> >  +    IOPortOps *ops;
>
> const

Yup, will fix.  Will repost once we agree on the approach.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 1/2] Type-safe ioport callbacks

[Juan Quintela]

Avi Kivity <avi@redhat.com> wrote:

> +static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    IOPort *ioport = opaque;
> +
> +    return ioport->ops->writeb(ioport, addr, data);
> +}
> +
> +static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    IOPort *ioport = opaque;
> +
> +    return ioport->ops->writew(ioport, addr, data);
> +}
> +
> +static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data)
> +{
> +    IOPort *ioport = opaque;
> +
> +    return ioport->ops->writel(ioport, addr, data);
> +}

"return" is not needed on this three functions.

Rest of aproach looks good.

Later, Juan.

[Qemu-devel] Re: [PATCH 1/2] Type-safe ioport callbacks

[Avi Kivity]

  On 10/25/2010 02:54 PM, Juan Quintela wrote:
> Avi Kivity<avi@redhat.com>  wrote:
>
> >  +static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data)
> >  +{
> >  +    IOPort *ioport = opaque;
> >  +
> >  +    return ioport->ops->writeb(ioport, addr, data);
> >  +}
> >  +
> >  +static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data)
> >  +{
> >  +    IOPort *ioport = opaque;
> >  +
> >  +    return ioport->ops->writew(ioport, addr, data);
> >  +}
> >  +
> >  +static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data)
> >  +{
> >  +    IOPort *ioport = opaque;
> >  +
> >  +    return ioport->ops->writel(ioport, addr, data);
> >  +}
>
> "return" is not needed on this three functions.
>

Fixed.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 0/2] Type-safe ioport callbacks

[Anthony Liguori]

On 10/24/2010 10:34 AM, Avi Kivity wrote:
> A recent qemu ->  qemu-kvm merge broke cpu hotplug without the compiler
> complaining because of the type-unsafeness of the ioport callbacks.  This
> patchset adds a type-safe variant of ioport callbacks and coverts a sample
> ioport.  Converting the other 300-odd registrations is left as an excercise
> to the community.
>
> Avi Kivity (2):
>    Type-safe ioport callbacks
>    piix4 acpi: convert io BAR to type-safe ioport callbacks
>    

Makes sense.

Acked-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>   hw/acpi_piix4.c |   30 +++++++++++++++----------
>   ioport.c        |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   ioport.h        |   16 +++++++++++++
>   3 files changed, 98 insertions(+), 12 deletions(-)
>
>    

Re: [Qemu-devel] Re: [PATCH 0/2] Type-safe ioport callbacks

[Anthony Liguori]

On 10/24/2010 12:38 PM, Avi Kivity wrote:
>  On 10/24/2010 07:35 PM, Paolo Bonzini wrote:
>> On 10/24/2010 05:34 PM, Avi Kivity wrote:
>>> A recent qemu ->  qemu-kvm merge broke cpu hotplug without the compiler
>>> complaining because of the type-unsafeness of the ioport callbacks.  
>>> This
>>> patchset adds a type-safe variant of ioport callbacks and coverts a 
>>> sample
>>> ioport.  Converting the other 300-odd registrations is left as an 
>>> excercise
>>> to the community.
>>
>> Should we create a Documentation/ file with incomplete transitions 
>> and the commit(s) that introduced them, for volunteers who wish to do 
>> some dirty work or to learn Coccinelle?
>
> If we have a TODO, we could add a janitor section there.

That's unnecessary work.  The vast majority of the callers of 
register_ioport* should be using a bus specific interface instead.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 0/2] Type-safe ioport callbacks

[Paolo Bonzini]

On 10/25/2010 03:46 PM, Anthony Liguori wrote:
>>>
>>> Should we create a Documentation/ file with incomplete transitions
>>> and the commit(s) that introduced them, for volunteers who wish to do
>>> some dirty work or to learn Coccinelle?
>>
>> If we have a TODO, we could add a janitor section there.
>
> That's unnecessary work.  The vast majority of the callers of
> register_ioport* should be using a bus specific interface instead.

It's still an incomplete transition.

Paolo

Re: [Qemu-devel] Re: [PATCH 0/2] Type-safe ioport callbacks

[Anthony Liguori]

On 10/25/2010 08:52 AM, Paolo Bonzini wrote:
> On 10/25/2010 03:46 PM, Anthony Liguori wrote:
>>>>
>>>> Should we create a Documentation/ file with incomplete transitions
>>>> and the commit(s) that introduced them, for volunteers who wish to do
>>>> some dirty work or to learn Coccinelle?
>>>
>>> If we have a TODO, we could add a janitor section there.
>>
>> That's unnecessary work.  The vast majority of the callers of
>> register_ioport* should be using a bus specific interface instead.
>
> It's still an incomplete transition.

Right, but what I meant was, there's not a lot of benefit to switching 
callers to ioport_register() if they should be using 
isa_ioport_register() or whatever is the appropriate interface.

Regards,

Anthony Liguori

> Paolo

Re: [Qemu-devel] [PATCH 0/2] Type-safe ioport callbacks

[Markus Armbruster]

Avi Kivity <avi@redhat.com> writes:

> A recent qemu -> qemu-kvm merge broke cpu hotplug without the compiler
> complaining because of the type-unsafeness of the ioport callbacks.  This
> patchset adds a type-safe variant of ioport callbacks and coverts a sample
> ioport.  Converting the other 300-odd registrations is left as an excercise
> to the community.
>
> Avi Kivity (2):
>   Type-safe ioport callbacks
>   piix4 acpi: convert io BAR to type-safe ioport callbacks
>
>  hw/acpi_piix4.c |   30 +++++++++++++++----------
>  ioport.c        |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ioport.h        |   16 +++++++++++++
>  3 files changed, 98 insertions(+), 12 deletions(-)

Related: proper error handling
http://www.mail-archive.com/qemu-devel@nongnu.org/msg43710.html

I think that could be built nicely on top of this.

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Blue Swirl]

On Mon, Oct 25, 2010 at 10:00 AM, Avi Kivity <avi@redhat.com> wrote:
>  On 10/24/2010 08:14 PM, Blue Swirl wrote:
>>
>> On Sun, Oct 24, 2010 at 3:34 PM, Avi Kivity<avi@redhat.com>  wrote:
>> >  The current ioport callbacks are not type-safe, in that they accept an
>> > "opaque"
>> >  pointer as an argument whose type must match the argument to the
>> > registration
>> >  function; this is not checked by the compiler.
>> >
>> >  This patch adds an alternative that is type-safe.  Instead of an opaque
>> >  argument, both registation and the callback use a new IOPort type.  The
>> >  callback then uses container_of() to access its main structures.
>> >
>> >  Currently the old and new methods exist side by side; once the old way
>> > is gone,
>> >  we can also save a bunch of memory since the new method requires one
>> > pointer
>> >  per ioport instead of 6.
>> >
>> >  Signed-off-by: Avi Kivity<avi@redhat.com>
>>
>> If we are going to change the interface, let's do it so that it's
>> useful for other uses too:
>> http://article.gmane.org/gmane.comp.emulators.qemu/76984
>
> I don't really see why we need registration; cpu_register_io() takes
> function pointers, a size, and an opaque, and gives an integer handle in
> return.  With the IOPort object approach, you set up the IOPort with
> function pointers, size is implied, and the opaque is derived using
> container_of(); the handle is simply the address of the object.

With the handle, we can separate setting up the structures at device
level, and mapping the object using only the handle at bus or other
higher level. Can this be done with the object approach?

The purpose of that patch series was to perform the separation for PCI
BARs. I wasn't so happy with the series, so I never pushed.

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Avi Kivity]

  On 10/25/2010 08:38 PM, Blue Swirl wrote:
> >
> >  I don't really see why we need registration; cpu_register_io() takes
> >  function pointers, a size, and an opaque, and gives an integer handle in
> >  return.  With the IOPort object approach, you set up the IOPort with
> >  function pointers, size is implied, and the opaque is derived using
> >  container_of(); the handle is simply the address of the object.
>
> With the handle, we can separate setting up the structures at device
> level, and mapping the object using only the handle at bus or other
> higher level. Can this be done with the object approach?

I believe so.  The handle is simply an indirect pointer, no?

> The purpose of that patch series was to perform the separation for PCI
> BARs. I wasn't so happy with the series, so I never pushed.

In fact I think an IOPort is even more suitable; if we need additional 
attributes we can use a derived object:

struct PCIIOPort {
     IOPort ioport;
     /* additional fields */
};



-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Blue Swirl]

On Tue, Oct 26, 2010 at 8:05 AM, Avi Kivity <avi@redhat.com> wrote:
>  On 10/25/2010 08:38 PM, Blue Swirl wrote:
>>
>> >
>> >  I don't really see why we need registration; cpu_register_io() takes
>> >  function pointers, a size, and an opaque, and gives an integer handle
>> > in
>> >  return.  With the IOPort object approach, you set up the IOPort with
>> >  function pointers, size is implied, and the opaque is derived using
>> >  container_of(); the handle is simply the address of the object.
>>
>> With the handle, we can separate setting up the structures at device
>> level, and mapping the object using only the handle at bus or other
>> higher level. Can this be done with the object approach?
>
> I believe so.  The handle is simply an indirect pointer, no?

Yes, but then the object should also contain size information. That
should not be needed for mapping at higher level.

>> The purpose of that patch series was to perform the separation for PCI
>> BARs. I wasn't so happy with the series, so I never pushed.
>
> In fact I think an IOPort is even more suitable; if we need additional
> attributes we can use a derived object:
>
> struct PCIIOPort {
>    IOPort ioport;
>    /* additional fields */
> };

One issue with my series was that it would be great if the devices
just had some BAR structures (used by PCI layer to map the devices)
inside PCI/qdev structures, but I invented that too late. Maybe this
can be addressed in your design?

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Avi Kivity]

  On 10/26/2010 05:09 PM, Blue Swirl wrote:
> On Tue, Oct 26, 2010 at 8:05 AM, Avi Kivity<avi@redhat.com>  wrote:
> >    On 10/25/2010 08:38 PM, Blue Swirl wrote:
> >>
> >>  >
> >>  >    I don't really see why we need registration; cpu_register_io() takes
> >>  >    function pointers, a size, and an opaque, and gives an integer handle
> >>  >  in
> >>  >    return.  With the IOPort object approach, you set up the IOPort with
> >>  >    function pointers, size is implied, and the opaque is derived using
> >>  >    container_of(); the handle is simply the address of the object.
> >>
> >>  With the handle, we can separate setting up the structures at device
> >>  level, and mapping the object using only the handle at bus or other
> >>  higher level. Can this be done with the object approach?
> >
> >  I believe so.  The handle is simply an indirect pointer, no?
>
> Yes, but then the object should also contain size information. That
> should not be needed for mapping at higher level.

Sorry, I don't follow your meaning.

When I said "size is implied" I meant that the IOPort object has a 
separate function pointer for sizes 1, 2, and 4, so it ioport_register() 
doesn't need a size parameter.  But I don't see how that relates to your 
comment.

> >>  The purpose of that patch series was to perform the separation for PCI
> >>  BARs. I wasn't so happy with the series, so I never pushed.
> >
> >  In fact I think an IOPort is even more suitable; if we need additional
> >  attributes we can use a derived object:
> >
> >  struct PCIIOPort {
> >      IOPort ioport;
> >      /* additional fields */
> >  };
>
> One issue with my series was that it would be great if the devices
> just had some BAR structures (used by PCI layer to map the devices)
> inside PCI/qdev structures, but I invented that too late. Maybe this
> can be addressed in your design?

It looks to be orthogonal.  It would be great to have a BAR object; that 
object can then use your API, my API, or the existing API.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Anthony Liguori]

On 10/26/2010 12:18 PM, Avi Kivity wrote:
>  On 10/26/2010 05:09 PM, Blue Swirl wrote:
>> On Tue, Oct 26, 2010 at 8:05 AM, Avi Kivity<avi@redhat.com>  wrote:
>> >    On 10/25/2010 08:38 PM, Blue Swirl wrote:
>> >>
>> >> >
>> >> >    I don't really see why we need registration; 
>> cpu_register_io() takes
>> >> >    function pointers, a size, and an opaque, and gives an 
>> integer handle
>> >> >  in
>> >> >    return.  With the IOPort object approach, you set up the 
>> IOPort with
>> >> >    function pointers, size is implied, and the opaque is derived 
>> using
>> >> >    container_of(); the handle is simply the address of the object.
>> >>
>> >>  With the handle, we can separate setting up the structures at device
>> >>  level, and mapping the object using only the handle at bus or other
>> >>  higher level. Can this be done with the object approach?
>> >
>> >  I believe so.  The handle is simply an indirect pointer, no?
>>
>> Yes, but then the object should also contain size information. That
>> should not be needed for mapping at higher level.
>
> Sorry, I don't follow your meaning.
>
> When I said "size is implied" I meant that the IOPort object has a 
> separate function pointer for sizes 1, 2, and 4, so it 
> ioport_register() doesn't need a size parameter.  But I don't see how 
> that relates to your comment.

Yeah, I don't think it makes sense to combine "this is how to dispatch 
I/O" with "this is a region of I/O address space".

I think an IORegion should contain an IOPort structure though.  I think 
the name needs rethinking.

Maybe:

struct PortIOHandler;
struct MemoryIOHandler;

And it would be good to add a memory callback to this series too.

Regards,

Anthony Liguori

>> >>  The purpose of that patch series was to perform the separation 
>> for PCI
>> >>  BARs. I wasn't so happy with the series, so I never pushed.
>> >
>> >  In fact I think an IOPort is even more suitable; if we need 
>> additional
>> >  attributes we can use a derived object:
>> >
>> >  struct PCIIOPort {
>> >      IOPort ioport;
>> >      /* additional fields */
>> >  };
>>
>> One issue with my series was that it would be great if the devices
>> just had some BAR structures (used by PCI layer to map the devices)
>> inside PCI/qdev structures, but I invented that too late. Maybe this
>> can be addressed in your design?
>
> It looks to be orthogonal.  It would be great to have a BAR object; 
> that object can then use your API, my API, or the existing API.
>

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Avi Kivity]

  On 10/26/2010 07:27 PM, Anthony Liguori wrote:
>> Sorry, I don't follow your meaning.
>>
>> When I said "size is implied" I meant that the IOPort object has a 
>> separate function pointer for sizes 1, 2, and 4, so it 
>> ioport_register() doesn't need a size parameter.  But I don't see how 
>> that relates to your comment.
>
>
> Yeah, I don't think it makes sense to combine "this is how to dispatch 
> I/O" with "this is a region of I/O address space".

Oh, so Blue meant the size of the region in ports, not the size of the 
individual ports.  I think that putting the range length (but not base 
address) in the IOPort structure may make sense.

>
> I think an IORegion should contain an IOPort structure though.  I 
> think the name needs rethinking.
>
> Maybe:
>
> struct PortIOHandler;
> struct MemoryIOHandler;

Why two types?  I think some devices use PIO on a PC and MMIO on other 
architectures.  Sharing the type would allow sharing code.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Blue Swirl]

On Tue, Oct 26, 2010 at 5:35 PM, Avi Kivity <avi@redhat.com> wrote:
>  On 10/26/2010 07:27 PM, Anthony Liguori wrote:
>>>
>>> Sorry, I don't follow your meaning.
>>>
>>> When I said "size is implied" I meant that the IOPort object has a
>>> separate function pointer for sizes 1, 2, and 4, so it ioport_register()
>>> doesn't need a size parameter.  But I don't see how that relates to your
>>> comment.
>>
>>
>> Yeah, I don't think it makes sense to combine "this is how to dispatch
>> I/O" with "this is a region of I/O address space".
>
> Oh, so Blue meant the size of the region in ports, not the size of the
> individual ports.  I think that putting the range length (but not base
> address) in the IOPort structure may make sense.

Yes, that's what I meant. Consider for example the handlers: they
expect that the port is within some range.

>>
>> I think an IORegion should contain an IOPort structure though.  I think
>> the name needs rethinking.
>>
>> Maybe:
>>
>> struct PortIOHandler;
>> struct MemoryIOHandler;
>
> Why two types?  I think some devices use PIO on a PC and MMIO on other
> architectures.  Sharing the type would allow sharing code.

Then there are the functions provided by rwhandler.c. I think that
interface makes even more sense compared to 8/16/32 (and 64?) bit
handlers in many cases.

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Avi Kivity]

  On 10/26/2010 08:33 PM, Blue Swirl wrote:
> >
> >  Why two types?  I think some devices use PIO on a PC and MMIO on other
> >  architectures.  Sharing the type would allow sharing code.
>
> Then there are the functions provided by rwhandler.c. I think that
> interface makes even more sense compared to 8/16/32 (and 64?) bit
> handlers in many cases.

On the other hand, that makes the transition harder.

Perhaps we can have a type with {read,write}(addr, width), and another 
built on top that provides the traditional {read,write}[bwl](addr) to 
ease the transition.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Blue Swirl]

On Wed, Oct 27, 2010 at 9:26 AM, Avi Kivity <avi@redhat.com> wrote:
>  On 10/26/2010 08:33 PM, Blue Swirl wrote:
>>
>> >
>> >  Why two types?  I think some devices use PIO on a PC and MMIO on other
>> >  architectures.  Sharing the type would allow sharing code.
>>
>> Then there are the functions provided by rwhandler.c. I think that
>> interface makes even more sense compared to 8/16/32 (and 64?) bit
>> handlers in many cases.
>
> On the other hand, that makes the transition harder.
>
> Perhaps we can have a type with {read,write}(addr, width), and another built
> on top that provides the traditional {read,write}[bwl](addr) to ease the
> transition.

That should work.