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

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

[Avi Kivity]

A not-so-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.

v3:
 - define a common IORange that can also be used for mmio
 - move start/length into IORange
 - make access width a parameter of the access functions instead of
   having a callback per access size

v2:
 - const correctness
 - avoid return void

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

 hw/acpi_piix4.c |   55 +++++++++++++++++++----------------------------
 ioport.c        |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ioport.h        |    2 +
 iorange.h       |   30 +++++++++++++++++++++++++
 4 files changed, 118 insertions(+), 33 deletions(-)
 create mode 100644 iorange.h

-- 
1.7.3.1

[Qemu-devel] [PATCH v3 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.

Acked-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
---
 ioport.c  |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ioport.h  |    2 +
 iorange.h |   30 ++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 0 deletions(-)
 create mode 100644 iorange.h

diff --git a/ioport.c b/ioport.c
index ec3dc65..aa4188a 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)
+{
+    IORange *ioport = opaque;
+    uint64_t data;
+
+    ioport->ops->read(ioport, addr - ioport->base, 1, &data);
+    return data;
+}
+
+static uint32_t ioport_readw_thunk(void *opaque, uint32_t addr)
+{
+    IORange *ioport = opaque;
+    uint64_t data;
+
+    ioport->ops->read(ioport, addr - ioport->base, 2, &data);
+    return data;
+}
+
+static uint32_t ioport_readl_thunk(void *opaque, uint32_t addr)
+{
+    IORange *ioport = opaque;
+    uint64_t data;
+
+    ioport->ops->read(ioport, addr - ioport->base, 4, &data);
+    return data;
+}
+
+static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data)
+{
+    IORange *ioport = opaque;
+
+    ioport->ops->write(ioport, addr - ioport->base, 1, data);
+}
+
+static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data)
+{
+    IORange *ioport = opaque;
+
+    ioport->ops->write(ioport, addr - ioport->base, 2, data);
+}
+
+static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data)
+{
+    IORange *ioport = opaque;
+
+    ioport->ops->write(ioport, addr - ioport->base, 4, data);
+}
+
+void ioport_register(IORange *ioport)
+{
+    register_ioport_read(ioport->base, ioport->len, 1,
+                         ioport_readb_thunk, ioport);
+    register_ioport_read(ioport->base, ioport->len, 2,
+                         ioport_readw_thunk, ioport);
+    register_ioport_read(ioport->base, ioport->len, 4,
+                         ioport_readl_thunk, ioport);
+    register_ioport_write(ioport->base, ioport->len, 1,
+                          ioport_writeb_thunk, ioport);
+    register_ioport_write(ioport->base, ioport->len, 2,
+                          ioport_writew_thunk, ioport);
+    register_ioport_write(ioport->base, ioport->len, 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..5ae62a3 100644
--- a/ioport.h
+++ b/ioport.h
@@ -25,6 +25,7 @@
 #define IOPORT_H
 
 #include "qemu-common.h"
+#include "iorange.h"
 
 typedef uint32_t pio_addr_t;
 #define FMT_pioaddr     PRIx32
@@ -36,6 +37,7 @@ 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);
 
+void ioport_register(IORange *iorange);
 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,
diff --git a/iorange.h b/iorange.h
new file mode 100644
index 0000000..9783168
--- /dev/null
+++ b/iorange.h
@@ -0,0 +1,30 @@
+#ifndef IORANGE_H
+#define IORANGE_H
+
+#include <stdint.h>
+
+typedef struct IORange IORange;
+typedef struct IORangeOps IORangeOps;
+
+struct IORangeOps {
+    void (*read)(IORange *iorange, uint64_t offset, unsigned width,
+                 uint64_t *data);
+    void (*write)(IORange *iorange, uint64_t offset, unsigned width,
+                  uint64_t data);
+};
+
+struct IORange {
+    const IORangeOps *ops;
+    uint64_t base;
+    uint64_t len;
+};
+
+static inline void iorange_init(IORange *iorange, const IORangeOps *ops,
+                                uint64_t base, uint64_t len)
+{
+    iorange->ops = ops;
+    iorange->base = base;
+    iorange->len = len;
+}
+
+#endif
-- 
1.7.3.1

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

[Avi Kivity]

Acked-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/acpi_piix4.c |   55 ++++++++++++++++++++++---------------------------------
 1 files changed, 22 insertions(+), 33 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index f549089..173d781 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -52,6 +52,7 @@ struct pci_status {
 
 typedef struct PIIX4PMState {
     PCIDevice dev;
+    IORange ioport;
     uint16_t pmsts;
     uint16_t pmen;
     uint16_t pmcntrl;
@@ -128,10 +129,16 @@ 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_write(IORange *ioport, uint64_t addr, unsigned width,
+                            uint64_t val)
 {
-    PIIX4PMState *s = opaque;
-    addr &= 0x3f;
+    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
+
+    if (width != 2) {
+        PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n",
+                      (unsigned)addr, width, (unsigned)val);
+    }
+
     switch(addr) {
     case 0x00:
         {
@@ -184,12 +191,12 @@ 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 void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width,
+                            uint64_t *data)
 {
-    PIIX4PMState *s = opaque;
+    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
     uint32_t val;
 
-    addr &= 0x3f;
     switch(addr) {
     case 0x00:
         val = get_pmsts(s);
@@ -200,27 +207,6 @@ static uint32_t pm_ioport_readw(void *opaque, uint32_t addr)
     case 0x04:
         val = s->pmcntrl;
         break;
-    default:
-        val = 0;
-        break;
-    }
-    PIIX4_DPRINTF("PM readw port=0x%04x val=0x%04x\n", addr, val);
-    return val;
-}
-
-static void pm_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
-{
-    //    PIIX4PMState *s = opaque;
-    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)
-{
-    PIIX4PMState *s = opaque;
-    uint32_t val;
-
-    addr &= 0x3f;
-    switch(addr) {
     case 0x08:
         val = get_pmtmr(s);
         break;
@@ -228,10 +214,15 @@ static uint32_t pm_ioport_readl(void *opaque, uint32_t addr)
         val = 0;
         break;
     }
-    PIIX4_DPRINTF("PM readl port=0x%04x val=0x%08x\n", addr, val);
-    return val;
+    PIIX4_DPRINTF("PM readw port=0x%04x val=0x%04x\n", addr, val);
+    *data = val;
 }
 
+static const IORangeOps pm_iorange_ops = {
+    .read = pm_ioport_read,
+    .write = pm_ioport_write,
+};
+
 static void apm_ctrl_changed(uint32_t val, void *arg)
 {
     PIIX4PMState *s = arg;
@@ -265,10 +256,8 @@ 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);
+        iorange_init(&s->ioport, &pm_iorange_ops, pm_io_base, 64);
+        ioport_register(&s->ioport);
     }
 }
 
-- 
1.7.3.1

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

[Anthony Liguori]

On 11/17/2010 03:50 AM, Avi Kivity wrote:
> A not-so-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.
>    

Applied all.  Thanks.

Regards,

Anthony Liguori

> v3:
>   - define a common IORange that can also be used for mmio
>   - move start/length into IORange
>   - make access width a parameter of the access functions instead of
>     having a callback per access size
>
> v2:
>   - const correctness
>   - avoid return void
>
> Avi Kivity (2):
>    Type-safe ioport callbacks
>    piix4 acpi: convert io BAR to type-safe ioport callbacks
>
>   hw/acpi_piix4.c |   55 +++++++++++++++++++----------------------------
>   ioport.c        |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   ioport.h        |    2 +
>   iorange.h       |   30 +++++++++++++++++++++++++
>   4 files changed, 118 insertions(+), 33 deletions(-)
>   create mode 100644 iorange.h
>
>