[Qemu-devel] [PATCH 0/2] IDE: combine portio r/w functions

[Qemu-devel] [PATCH 0/2] IDE: combine portio r/w functions

[John Snow]

Mark, here's a quick sketch for you. There are two things I don't like,
but didn't care enough to fix:

(1) Restricting nbytes to 2 or 4 means some extra boilerplate
    to quiet compilers who don't know it will only ever be 2 or 4
(2) the address value is all-but-ignored, it carries over from the
    portio signature and is useful primarily for tracing, but it's
    a little ugly/deceiving to take a parameter and not use it.

Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

John Snow (2):
  ide: generic ide_data_read
  ide: generic ide_data_write

 hw/ide/core.c             | 185 +++++++++++++++++++++-------------------------
 hw/ide/trace-events       |   7 +-
 include/hw/ide/internal.h |   4 +-
 3 files changed, 89 insertions(+), 107 deletions(-)

-- 
2.9.5

[Qemu-devel] [PATCH 1/2] ide: generic ide_data_read

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c             | 99 +++++++++++++++++++++--------------------------
 hw/ide/trace-events       |  4 +-
 include/hw/ide/internal.h |  3 +-
 3 files changed, 49 insertions(+), 57 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index a19bd90..393f523 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2266,6 +2266,49 @@ static bool ide_is_pio_out(IDEState *s)
     abort();
 }
 
+uint32_t ide_data_read(void *opaque, uint32_t addr, short nbytes)
+{
+    IDEBus *bus = opaque;
+    IDEState *s = idebus_active_if(bus);
+    uint8_t *p;
+    int ret;
+
+    g_assert(nbytes == 2 || nbytes == 4);
+
+    /* PIO data access allowed only when DRQ bit is set. The result of a read
+     * during PIO in is indeterminate, return 0 and don't move forward. */
+    if (!(s->status & DRQ_STAT) || !ide_is_pio_out(s)) {
+        ret = 0;
+        goto out;
+    }
+
+    p = s->data_ptr;
+    if (p + nbytes > s->data_end) {
+        ret = 0;
+        goto out;
+    }
+
+    if (nbytes == 2) {
+        ret = cpu_to_le16(*(uint16_t *)p);
+    } else if (nbytes == 4) {
+        ret = cpu_to_le32(*(uint32_t *)p);
+    } else {
+        ret = 0;
+        goto out;
+    }
+
+    p += nbytes;
+    s->data_ptr = p;
+    if (p >= s->data_end) {
+        s->status &= ~DRQ_STAT;
+        s->end_transfer_func(s);
+    }
+
+ out:
+    trace_ide_data_read(addr, nbytes, ret, bus, s);
+    return ret;
+}
+
 void ide_data_writew(void *opaque, uint32_t addr, uint32_t val)
 {
     IDEBus *bus = opaque;
@@ -2296,32 +2339,7 @@ void ide_data_writew(void *opaque, uint32_t addr, uint32_t val)
 
 uint32_t ide_data_readw(void *opaque, uint32_t addr)
 {
-    IDEBus *bus = opaque;
-    IDEState *s = idebus_active_if(bus);
-    uint8_t *p;
-    int ret;
-
-    /* PIO data access allowed only when DRQ bit is set. The result of a read
-     * during PIO in is indeterminate, return 0 and don't move forward. */
-    if (!(s->status & DRQ_STAT) || !ide_is_pio_out(s)) {
-        return 0;
-    }
-
-    p = s->data_ptr;
-    if (p + 2 > s->data_end) {
-        return 0;
-    }
-
-    ret = cpu_to_le16(*(uint16_t *)p);
-    p += 2;
-    s->data_ptr = p;
-    if (p >= s->data_end) {
-        s->status &= ~DRQ_STAT;
-        s->end_transfer_func(s);
-    }
-
-    trace_ide_data_readw(addr, ret, bus, s);
-    return ret;
+    return ide_data_read(opaque, addr, 2);
 }
 
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val)
@@ -2354,34 +2372,7 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val)
 
 uint32_t ide_data_readl(void *opaque, uint32_t addr)
 {
-    IDEBus *bus = opaque;
-    IDEState *s = idebus_active_if(bus);
-    uint8_t *p;
-    int ret;
-
-    /* PIO data access allowed only when DRQ bit is set. The result of a read
-     * during PIO in is indeterminate, return 0 and don't move forward. */
-    if (!(s->status & DRQ_STAT) || !ide_is_pio_out(s)) {
-        ret = 0;
-        goto out;
-    }
-
-    p = s->data_ptr;
-    if (p + 4 > s->data_end) {
-        return 0;
-    }
-
-    ret = cpu_to_le32(*(uint32_t *)p);
-    p += 4;
-    s->data_ptr = p;
-    if (p >= s->data_end) {
-        s->status &= ~DRQ_STAT;
-        s->end_transfer_func(s);
-    }
-
-out:
-    trace_ide_data_readl(addr, ret, bus, s);
-    return ret;
+    return ide_data_read(opaque, addr, 4);
 }
 
 static void ide_dummy_transfer_stop(IDEState *s)
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 601bd97..e42c428 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -7,10 +7,10 @@ ide_ioport_write(uint32_t addr, const char *reg, uint32_t val, void *bus, void *
 ide_status_read(uint32_t addr, uint32_t val, void *bus, void *s)                   "IDE PIO rd @ 0x%"PRIx32" (Alt Status); val 0x%02"PRIx32"; bus %p; IDEState %p"
 ide_cmd_write(uint32_t addr, uint32_t val, void *bus)                              "IDE PIO wr @ 0x%"PRIx32" (Device Control); val 0x%02"PRIx32"; bus %p"
 # Warning: verbose
-ide_data_readw(uint32_t addr, uint32_t val, void *bus, void *s)                    "IDE PIO rd @ 0x%"PRIx32" (Data: Word); val 0x%04"PRIx32"; bus %p; IDEState %p"
 ide_data_writew(uint32_t addr, uint32_t val, void *bus, void *s)                   "IDE PIO wr @ 0x%"PRIx32" (Data: Word); val 0x%04"PRIx32"; bus %p; IDEState %p"
-ide_data_readl(uint32_t addr, uint32_t val, void *bus, void *s)                    "IDE PIO rd @ 0x%"PRIx32" (Data: Long); val 0x%08"PRIx32"; bus %p; IDEState %p"
 ide_data_writel(uint32_t addr, uint32_t val, void *bus, void *s)                   "IDE PIO wr @ 0x%"PRIx32" (Data: Long); val 0x%08"PRIx32"; bus %p; IDEState %p"
+ide_data_read(uint32_t addr, short nbytes, uint32_t val, void *bus, void *s)       "IDE PIO rd @ 0x%"PRIx32" (Data: %d bytes); val 0x%08"PRIx32"; bus %p; IDEState %p"
+
 # misc
 ide_exec_cmd(void *bus, void *state, uint32_t cmd) "IDE exec cmd: bus %p; state %p; cmd 0x%02x"
 ide_cancel_dma_sync_buffered(void *fn, void *req) "invoking cb %p of buffered request %p with -ECANCELED"
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 180e00e..3159c66 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -599,8 +599,9 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1);
 uint32_t ide_status_read(void *opaque, uint32_t addr);
 void ide_cmd_write(void *opaque, uint32_t addr, uint32_t val);
 void ide_data_writew(void *opaque, uint32_t addr, uint32_t val);
-uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
+uint32_t ide_data_read(void *opaque, uint32_t addr, short nbytes);
+uint32_t ide_data_readw(void *opaque, uint32_t addr);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
 int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
-- 
2.9.5

[Qemu-devel] [PATCH 2/2] ide: generic ide_data_write

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c             | 86 +++++++++++++++++++++--------------------------
 hw/ide/trace-events       |  3 +-
 include/hw/ide/internal.h |  1 +
 3 files changed, 40 insertions(+), 50 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 393f523..af49de5 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2266,6 +2266,42 @@ static bool ide_is_pio_out(IDEState *s)
     abort();
 }
 
+void ide_data_write(void *opaque, uint32_t addr, short nbytes, uint32_t val)
+{
+    IDEBus *bus = opaque;
+    IDEState *s = idebus_active_if(bus);
+    uint8_t *p;
+
+    trace_ide_data_write(addr, nbytes, val, bus, s);
+
+    /* PIO data access allowed only when DRQ bit is set. The result of a write
+     * during PIO out is indeterminate, just ignore it. */
+    if (!(s->status & DRQ_STAT) || ide_is_pio_out(s)) {
+        return;
+    }
+
+    if (nbytes != 2 && nbytes != 4) {
+        return;
+    }
+
+    p = s->data_ptr;
+    if (p + nbytes > s->data_end) {
+        return;
+    }
+
+    if (nbytes == 2) {
+        *(uint16_t *)p = le16_to_cpu(val);
+    } else if (nbytes == 4) {
+        *(uint32_t *)p = le32_to_cpu(val);
+    }
+    p += nbytes;
+    s->data_ptr = p;
+    if (p >= s->data_end) {
+        s->status &= ~DRQ_STAT;
+        s->end_transfer_func(s);
+    }
+}
+
 uint32_t ide_data_read(void *opaque, uint32_t addr, short nbytes)
 {
     IDEBus *bus = opaque;
@@ -2311,30 +2347,7 @@ uint32_t ide_data_read(void *opaque, uint32_t addr, short nbytes)
 
 void ide_data_writew(void *opaque, uint32_t addr, uint32_t val)
 {
-    IDEBus *bus = opaque;
-    IDEState *s = idebus_active_if(bus);
-    uint8_t *p;
-
-    trace_ide_data_writew(addr, val, bus, s);
-
-    /* PIO data access allowed only when DRQ bit is set. The result of a write
-     * during PIO out is indeterminate, just ignore it. */
-    if (!(s->status & DRQ_STAT) || ide_is_pio_out(s)) {
-        return;
-    }
-
-    p = s->data_ptr;
-    if (p + 2 > s->data_end) {
-        return;
-    }
-
-    *(uint16_t *)p = le16_to_cpu(val);
-    p += 2;
-    s->data_ptr = p;
-    if (p >= s->data_end) {
-        s->status &= ~DRQ_STAT;
-        s->end_transfer_func(s);
-    }
+    return ide_data_write(opaque, addr, 2, val);
 }
 
 uint32_t ide_data_readw(void *opaque, uint32_t addr)
@@ -2344,30 +2357,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr)
 
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val)
 {
-    IDEBus *bus = opaque;
-    IDEState *s = idebus_active_if(bus);
-    uint8_t *p;
-
-    trace_ide_data_writel(addr, val, bus, s);
-
-    /* PIO data access allowed only when DRQ bit is set. The result of a write
-     * during PIO out is indeterminate, just ignore it. */
-    if (!(s->status & DRQ_STAT) || ide_is_pio_out(s)) {
-        return;
-    }
-
-    p = s->data_ptr;
-    if (p + 4 > s->data_end) {
-        return;
-    }
-
-    *(uint32_t *)p = le32_to_cpu(val);
-    p += 4;
-    s->data_ptr = p;
-    if (p >= s->data_end) {
-        s->status &= ~DRQ_STAT;
-        s->end_transfer_func(s);
-    }
+    return ide_data_write(opaque, addr, 4, val);
 }
 
 uint32_t ide_data_readl(void *opaque, uint32_t addr)
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index e42c428..e92c0bb 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -7,9 +7,8 @@ ide_ioport_write(uint32_t addr, const char *reg, uint32_t val, void *bus, void *
 ide_status_read(uint32_t addr, uint32_t val, void *bus, void *s)                   "IDE PIO rd @ 0x%"PRIx32" (Alt Status); val 0x%02"PRIx32"; bus %p; IDEState %p"
 ide_cmd_write(uint32_t addr, uint32_t val, void *bus)                              "IDE PIO wr @ 0x%"PRIx32" (Device Control); val 0x%02"PRIx32"; bus %p"
 # Warning: verbose
-ide_data_writew(uint32_t addr, uint32_t val, void *bus, void *s)                   "IDE PIO wr @ 0x%"PRIx32" (Data: Word); val 0x%04"PRIx32"; bus %p; IDEState %p"
-ide_data_writel(uint32_t addr, uint32_t val, void *bus, void *s)                   "IDE PIO wr @ 0x%"PRIx32" (Data: Long); val 0x%08"PRIx32"; bus %p; IDEState %p"
 ide_data_read(uint32_t addr, short nbytes, uint32_t val, void *bus, void *s)       "IDE PIO rd @ 0x%"PRIx32" (Data: %d bytes); val 0x%08"PRIx32"; bus %p; IDEState %p"
+ide_data_write(uint32_t addr, short nbytes, uint32_t val, void *bus, void *s)      "IDE PIO wr @ 0x%"PRIx32" (Data: %d bytes); val 0x%08"PRIx32"; bus %p; IDEState %p"
 
 # misc
 ide_exec_cmd(void *bus, void *state, uint32_t cmd) "IDE exec cmd: bus %p; state %p; cmd 0x%02x"
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 3159c66..deb592d 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -598,6 +598,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_ioport_read(void *opaque, uint32_t addr1);
 uint32_t ide_status_read(void *opaque, uint32_t addr);
 void ide_cmd_write(void *opaque, uint32_t addr, uint32_t val);
+void ide_data_write(void *opaque, uint32_t addr, short nbytes, uint32_t val);
 void ide_data_writew(void *opaque, uint32_t addr, uint32_t val);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_read(void *opaque, uint32_t addr, short nbytes);
-- 
2.9.5