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

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

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 |   30 +++++++++++++++----------
 ioport.c        |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ioport.h        |   16 +++++++++++++
 3 files changed, 98 insertions(+), 12 deletions(-)

-- 
1.7.3.1

[Qemu-devel] [PATCH v2 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 |   16 +++++++++++++++
 2 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/ioport.c b/ioport.c
index ec3dc65..b9e74b8 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;
+
+    ioport->ops->writeb(ioport, addr, data);
+}
+
+static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data)
+{
+    IOPort *ioport = opaque;
+
+    ioport->ops->writew(ioport, addr, data);
+}
+
+static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data)
+{
+    IOPort *ioport = opaque;
+
+    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..6a7742b 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 {
+    const 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 v2 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 |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index f74f34c..c01c2ee 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 const 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