Skip to contentSkip to conversation

Type-safe ioport callbacks

PATCH Applied Avi Kivity 2 participants 4 messages Nov 17, 2010 – Nov 21, 2010 KVM

lore
Share & tools
lore

Avi Kivity

author
#0 lore raw

v3 cover Type-safe ioport callbacks

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
Signature · 1 line
1.7.3.1
author
#1 lore raw

PATCH v3 1/2 Type-safe ioport callbacks

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 <[email protected]>
Signed-off-by Avi Kivity <[email protected]>
---
 ioport.c  |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ioport.h  |    2 +
 iorange.h |   30 ++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 0 deletions(-)
 create mode 100644 iorange.h
ioport.c +64 -0
index ec3dc65..aa4188a 100644
@@ -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;
ioport.h +2 -0
index 3d3c8a3..5ae62a3 100644
@@ -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,
iorange.h new file +30 -0
new file mode 100644
index 0000000..9783168
@@ -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
Signature · 1 line
1.7.3.1
author
#2 lore raw

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

In reply to Avi Kivity
Acked-by Anthony Liguori <[email protected]>
Signed-off-by Avi Kivity <[email protected]>
---
 hw/acpi_piix4.c |   55 ++++++++++++++++++++++---------------------------------
 1 files changed, 22 insertions(+), 33 deletions(-)
hw/acpi_piix4.c +22 -33
index f549089..173d781 100644
@@ -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);
     }
 }
Signature · 1 line
1.7.3.1
Applied
#3 lore raw
In reply to Avi Kivity
Quoted 6 lines · 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.

Applied all. Thanks.

Regards,

Anthony Liguori

Quoted 22 lines

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