[Qemu-devel] [PATCH] 64 bit I/O support v7

[Qemu-devel] [PATCH] 64 bit I/O support v7

[Robert Reif]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

This patch is a rebase on current svn of my previous patch.

This patch adds support for 64 bit I/O by essentially replacing an
array of 4 function pointers with a structure containing 4 function
pointers. The current array contains 4 pointers, one for 8, 16 and 32
bit function pointers which really support only 32 bits and a
forth pointer which is unused but I assume is a place holder for a
64 bit function pointer.  The current trick of passing 8, 16 or 32 bits
in a 32 bit variable doesn't extend to passing 64 bits in a 32 bit
bit variable and creating a 64 bit function would require casts to
allow being saved in the array so a sutrcture is used that can take
different function pointers.

Here are the relevant parts of the patch:

+typedef void CPUWriteMemoryFunc64(void *opaque, target_phys_addr_t 
addr, uint64_t value);
+typedef uint64_t CPUReadMemoryFunc64(void *opaque, target_phys_addr_t 
addr);
+
+typedef struct CPUWriteMemoryFuncs {
+    CPUWriteMemoryFunc   *b;
+    CPUWriteMemoryFunc   *w;
+    CPUWriteMemoryFunc   *l;
+    CPUWriteMemoryFunc64 *q;
+} CPUWriteMemoryFuncs;
+
+typedef struct CPUReadMemoryFuncs {
+    CPUReadMemoryFunc   *b;
+    CPUReadMemoryFunc   *w;
+    CPUReadMemoryFunc   *l;
+    CPUReadMemoryFunc64 *q;
+} CPUReadMemoryFuncs;

-extern CPUWriteMemoryFunc *io_mem_write[IO_MEM_NB_ENTRIES][4];
-extern CPUReadMemoryFunc *io_mem_read[IO_MEM_NB_ENTRIES][4];
+extern CPUWriteMemoryFuncs io_mem_write[IO_MEM_NB_ENTRIES];
+extern CPUReadMemoryFuncs io_mem_read[IO_MEM_NB_ENTRIES];

The rest of the patch just does the array to structure conversion and
adds the 64 bit functions.

The old array function call is supported so no existing drivers need
to be modified.  They can continue to do 2 32 bit accesses because 2
helper functions have been added to break up the accesses automatically.
However, the helper functions should only be used until all drivers are
converted to use the structure and then can be removed along
with the old array functions api.  The replacement of the arrays with
structures in the drivers is very straightforward for drivers that don't
do 64 bit I/O and the few that do can be cleaned up to remove the
work arounds for the lack of true 64 bit I/O by their maintainers.

Signed-off-by: Robert Reif <reif@earthlink.net>

[-- Attachment #2: io64-v7.diff.txt --]
[-- Type: text/plain, Size: 24805 bytes --]

Index: softmmu_template.h
===================================================================
--- softmmu_template.h	(revision 7190)
+++ softmmu_template.h	(working copy)
@@ -65,17 +65,7 @@
     }
 
     env->mem_io_vaddr = addr;
-#if SHIFT <= 2
-    res = io_mem_read[index][SHIFT](io_mem_opaque[index], physaddr);
-#else
-#ifdef TARGET_WORDS_BIGENDIAN
-    res = (uint64_t)io_mem_read[index][2](io_mem_opaque[index], physaddr) << 32;
-    res |= io_mem_read[index][2](io_mem_opaque[index], physaddr + 4);
-#else
-    res = io_mem_read[index][2](io_mem_opaque[index], physaddr);
-    res |= (uint64_t)io_mem_read[index][2](io_mem_opaque[index], physaddr + 4) << 32;
-#endif
-#endif /* SHIFT > 2 */
+    res = io_mem_read[index].SUFFIX(io_mem_opaque[index], physaddr);
 #ifdef CONFIG_KQEMU
     env->last_io_time = cpu_get_time_fast();
 #endif
@@ -210,17 +200,7 @@
 
     env->mem_io_vaddr = addr;
     env->mem_io_pc = (unsigned long)retaddr;
-#if SHIFT <= 2
-    io_mem_write[index][SHIFT](io_mem_opaque[index], physaddr, val);
-#else
-#ifdef TARGET_WORDS_BIGENDIAN
-    io_mem_write[index][2](io_mem_opaque[index], physaddr, val >> 32);
-    io_mem_write[index][2](io_mem_opaque[index], physaddr + 4, val);
-#else
-    io_mem_write[index][2](io_mem_opaque[index], physaddr, val);
-    io_mem_write[index][2](io_mem_opaque[index], physaddr + 4, val >> 32);
-#endif
-#endif /* SHIFT > 2 */
+    io_mem_write[index].SUFFIX(io_mem_opaque[index], physaddr, val);
 #ifdef CONFIG_KQEMU
     env->last_io_time = cpu_get_time_fast();
 #endif
Index: exec.c
===================================================================
--- exec.c	(revision 7190)
+++ exec.c	(working copy)
@@ -47,6 +47,7 @@
 //#define DEBUG_FLUSH
 //#define DEBUG_TLB
 //#define DEBUG_UNASSIGNED
+//#define DEBUG_MMIO64
 
 /* make various TB consistency checks */
 //#define DEBUG_TB_CHECK
@@ -182,8 +183,8 @@
 static void io_mem_init(void);
 
 /* io memory support */
-CPUWriteMemoryFunc *io_mem_write[IO_MEM_NB_ENTRIES][4];
-CPUReadMemoryFunc *io_mem_read[IO_MEM_NB_ENTRIES][4];
+CPUWriteMemoryFuncs io_mem_write[IO_MEM_NB_ENTRIES];
+CPUReadMemoryFuncs io_mem_read[IO_MEM_NB_ENTRIES];
 void *io_mem_opaque[IO_MEM_NB_ENTRIES];
 static char io_mem_used[IO_MEM_NB_ENTRIES];
 static int io_mem_watch;
@@ -2588,6 +2589,17 @@
     return 0;
 }
 
+static uint64_t unassigned_mem_readq(void *opaque, target_phys_addr_t addr)
+{
+#ifdef DEBUG_UNASSIGNED
+    printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
+#endif
+#if defined(TARGET_SPARC)
+    do_unassigned_access(addr, 0, 0, 0, 8);
+#endif
+    return 0;
+}
+
 static void unassigned_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
 #ifdef DEBUG_UNASSIGNED
@@ -2618,16 +2630,28 @@
 #endif
 }
 
-static CPUReadMemoryFunc *unassigned_mem_read[3] = {
+static void unassigned_mem_writeq(void *opaque, target_phys_addr_t addr, uint64_t val)
+{
+#ifdef DEBUG_UNASSIGNED
+    printf("Unassigned mem write " TARGET_FMT_plx " = 0x%016llx\n", addr, val);
+#endif
+#if defined(TARGET_SPARC)
+    do_unassigned_access(addr, 1, 0, 0, 8);
+#endif
+}
+
+static CPUReadMemoryFuncs unassigned_mem_read = {
     unassigned_mem_readb,
     unassigned_mem_readw,
     unassigned_mem_readl,
+    unassigned_mem_readq,
 };
 
-static CPUWriteMemoryFunc *unassigned_mem_write[3] = {
+static CPUWriteMemoryFuncs unassigned_mem_write = {
     unassigned_mem_writeb,
     unassigned_mem_writew,
     unassigned_mem_writel,
+    unassigned_mem_writeq,
 };
 
 static void notdirty_mem_writeb(void *opaque, target_phys_addr_t ram_addr,
@@ -2705,16 +2729,43 @@
         tlb_set_dirty(cpu_single_env, cpu_single_env->mem_io_vaddr);
 }
 
-static CPUReadMemoryFunc *error_mem_read[3] = {
+static void notdirty_mem_writeq(void *opaque, target_phys_addr_t ram_addr,
+                                uint64_t val)
+{
+    int dirty_flags;
+    dirty_flags = phys_ram_dirty[ram_addr >> TARGET_PAGE_BITS];
+    if (!(dirty_flags & CODE_DIRTY_FLAG)) {
+#if !defined(CONFIG_USER_ONLY)
+        tb_invalidate_phys_page_fast(ram_addr, 8);
+        dirty_flags = phys_ram_dirty[ram_addr >> TARGET_PAGE_BITS];
+#endif
+    }
+    stq_p(qemu_get_ram_ptr(ram_addr), val);
+#ifdef USE_KQEMU
+    if (cpu_single_env->kqemu_enabled &&
+        (dirty_flags & KQEMU_MODIFY_PAGE_MASK) != KQEMU_MODIFY_PAGE_MASK)
+        kqemu_modify_page(cpu_single_env, ram_addr);
+#endif
+    dirty_flags |= (0xff & ~CODE_DIRTY_FLAG);
+    phys_ram_dirty[ram_addr >> TARGET_PAGE_BITS] = dirty_flags;
+    /* we remove the notdirty callback only if the code has been
+       flushed */
+    if (dirty_flags == 0xff)
+        tlb_set_dirty(cpu_single_env, cpu_single_env->mem_io_vaddr);
+}
+
+static CPUReadMemoryFuncs error_mem_read = {
     NULL, /* never used */
     NULL, /* never used */
     NULL, /* never used */
+    NULL, /* never used */
 };
 
-static CPUWriteMemoryFunc *notdirty_mem_write[3] = {
+static CPUWriteMemoryFuncs notdirty_mem_write = {
     notdirty_mem_writeb,
     notdirty_mem_writew,
     notdirty_mem_writel,
+    notdirty_mem_writeq,
 };
 
 /* Generate a debug exception if a watchpoint has been hit.  */
@@ -2783,6 +2834,12 @@
     return ldl_phys(addr);
 }
 
+static uint64_t watch_mem_readq(void *opaque, target_phys_addr_t addr)
+{
+    check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x7, BP_MEM_READ);
+    return ldq_phys(addr);
+}
+
 static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
                              uint32_t val)
 {
@@ -2804,16 +2861,25 @@
     stl_phys(addr, val);
 }
 
-static CPUReadMemoryFunc *watch_mem_read[3] = {
+static void watch_mem_writeq(void *opaque, target_phys_addr_t addr,
+                             uint64_t val)
+{
+    check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x7, BP_MEM_WRITE);
+    stq_phys(addr, val);
+}
+
+static CPUReadMemoryFuncs watch_mem_read = {
     watch_mem_readb,
     watch_mem_readw,
     watch_mem_readl,
+    watch_mem_readq,
 };
 
-static CPUWriteMemoryFunc *watch_mem_write[3] = {
+static CPUWriteMemoryFuncs watch_mem_write = {
     watch_mem_writeb,
     watch_mem_writew,
     watch_mem_writel,
+    watch_mem_writeq,
 };
 
 static inline uint32_t subpage_readlen (subpage_t *mmio, target_phys_addr_t addr,
@@ -2902,23 +2968,54 @@
     subpage_writelen(opaque, addr, value, 2);
 }
 
-static CPUReadMemoryFunc *subpage_read[] = {
-    &subpage_readb,
-    &subpage_readw,
-    &subpage_readl,
+static uint64_t subpage_readq (void *opaque, target_phys_addr_t addr)
+{
+    subpage_t *mmio = (subpage_t *)opaque;
+    unsigned int idx;
+
+    idx = SUBPAGE_IDX(addr);
+#if defined(DEBUG_SUBPAGE)
+    printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d\n", __func__,
+           mmio, 3, addr, idx);
+#endif
+    return ((CPUReadMemoryFunc64*)(*mmio->mem_read[idx][3]))(mmio->opaque[idx][0][3],
+                                       addr + mmio->region_offset[idx][0][3]);
+}
+
+static void subpage_writeq (void *opaque,
+                         target_phys_addr_t addr, uint64_t value)
+{
+    subpage_t *mmio = (subpage_t *)opaque;
+    unsigned int idx;
+
+    idx = SUBPAGE_IDX(addr);
+#if defined(DEBUG_SUBPAGE)
+    printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d value %016llx\n", __func__,
+           mmio, 3, addr, idx, value);
+#endif
+    ((CPUWriteMemoryFunc64*)(*mmio->mem_write[idx][3]))(mmio->opaque[idx][1][3],
+                                  addr + mmio->region_offset[idx][1][3],
+                                  value);
+}
+
+static CPUReadMemoryFuncs subpage_read = {
+    subpage_readb,
+    subpage_readw,
+    subpage_readl,
+    subpage_readq,
 };
 
-static CPUWriteMemoryFunc *subpage_write[] = {
-    &subpage_writeb,
-    &subpage_writew,
-    &subpage_writel,
+static CPUWriteMemoryFuncs subpage_write = {
+    subpage_writeb,
+    subpage_writew,
+    subpage_writel,
+    subpage_writeq,
 };
 
 static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
                              ram_addr_t memory, ram_addr_t region_offset)
 {
     int idx, eidx;
-    unsigned int i;
 
     if (start >= TARGET_PAGE_SIZE || end >= TARGET_PAGE_SIZE)
         return -1;
@@ -2930,18 +3027,46 @@
 #endif
     memory >>= IO_MEM_SHIFT;
     for (; idx <= eidx; idx++) {
-        for (i = 0; i < 4; i++) {
-            if (io_mem_read[memory][i]) {
-                mmio->mem_read[idx][i] = &io_mem_read[memory][i];
-                mmio->opaque[idx][0][i] = io_mem_opaque[memory];
-                mmio->region_offset[idx][0][i] = region_offset;
-            }
-            if (io_mem_write[memory][i]) {
-                mmio->mem_write[idx][i] = &io_mem_write[memory][i];
-                mmio->opaque[idx][1][i] = io_mem_opaque[memory];
-                mmio->region_offset[idx][1][i] = region_offset;
-            }
+        if (io_mem_read[memory].b) {
+            mmio->mem_read[idx][0] = &io_mem_read[memory].b;
+            mmio->opaque[idx][0][0] = io_mem_opaque[memory];
+            mmio->region_offset[idx][0][0] = region_offset;
         }
+        if (io_mem_write[memory].b) {
+            mmio->mem_write[idx][0] = &io_mem_write[memory].b;
+            mmio->opaque[idx][1][0] = io_mem_opaque[memory];
+            mmio->region_offset[idx][1][0] = region_offset;
+        }
+        if (io_mem_read[memory].w) {
+            mmio->mem_read[idx][1] = &io_mem_read[memory].w;
+            mmio->opaque[idx][0][1] = io_mem_opaque[memory];
+            mmio->region_offset[idx][0][1] = region_offset;
+        }
+        if (io_mem_write[memory].w) {
+            mmio->mem_write[idx][1] = &io_mem_write[memory].w;
+            mmio->opaque[idx][1][1] = io_mem_opaque[memory];
+            mmio->region_offset[idx][1][1] = region_offset;
+        }
+        if (io_mem_read[memory].l) {
+            mmio->mem_read[idx][2] = &io_mem_read[memory].l;
+            mmio->opaque[idx][0][2] = io_mem_opaque[memory];
+            mmio->region_offset[idx][0][2] = region_offset;
+        }
+        if (io_mem_write[memory].l) {
+            mmio->mem_write[idx][2] = &io_mem_write[memory].l;
+            mmio->opaque[idx][1][2] = io_mem_opaque[memory];
+            mmio->region_offset[idx][1][2] = region_offset;
+        }
+        if (io_mem_read[memory].q) {
+            mmio->mem_read[idx][3] = (CPUReadMemoryFunc**)&io_mem_read[memory].q;
+            mmio->opaque[idx][0][3] = io_mem_opaque[memory];
+            mmio->region_offset[idx][0][3] = region_offset;
+        }
+        if (io_mem_write[memory].q) {
+            mmio->mem_write[idx][3] = (CPUWriteMemoryFunc**)&io_mem_write[memory].q;
+            mmio->opaque[idx][1][3] = io_mem_opaque[memory];
+            mmio->region_offset[idx][1][3] = region_offset;
+        }
     }
 
     return 0;
@@ -2956,7 +3081,7 @@
     mmio = qemu_mallocz(sizeof(subpage_t));
 
     mmio->base = base;
-    subpage_memory = cpu_register_io_memory(0, subpage_read, subpage_write, mmio);
+    subpage_memory = cpu_register_io_memory64(0, &subpage_read, &subpage_write, mmio);
 #if defined(DEBUG_SUBPAGE)
     printf("%s: %p base " TARGET_FMT_plx " len %08x %d\n", __func__,
            mmio, base, TARGET_PAGE_SIZE, subpage_memory);
@@ -2985,14 +3110,14 @@
 {
     int i;
 
-    cpu_register_io_memory(IO_MEM_ROM >> IO_MEM_SHIFT, error_mem_read, unassigned_mem_write, NULL);
-    cpu_register_io_memory(IO_MEM_UNASSIGNED >> IO_MEM_SHIFT, unassigned_mem_read, unassigned_mem_write, NULL);
-    cpu_register_io_memory(IO_MEM_NOTDIRTY >> IO_MEM_SHIFT, error_mem_read, notdirty_mem_write, NULL);
+    cpu_register_io_memory64(IO_MEM_ROM >> IO_MEM_SHIFT, &error_mem_read, &unassigned_mem_write, NULL);
+    cpu_register_io_memory64(IO_MEM_UNASSIGNED >> IO_MEM_SHIFT, &unassigned_mem_read, &unassigned_mem_write, NULL);
+    cpu_register_io_memory64(IO_MEM_NOTDIRTY >> IO_MEM_SHIFT, &error_mem_read, &notdirty_mem_write, NULL);
     for (i=0; i<5; i++)
         io_mem_used[i] = 1;
 
-    io_mem_watch = cpu_register_io_memory(0, watch_mem_read,
-                                          watch_mem_write, NULL);
+    io_mem_watch = cpu_register_io_memory64(0, &watch_mem_read,
+                                            &watch_mem_write, NULL);
 #ifdef CONFIG_KQEMU
     if (kqemu_phys_ram_base) {
         /* alloc dirty bits array */
@@ -3002,6 +3127,58 @@
 #endif
 }
 
+/* mem_readq and mem_writeq are helper functions that are
+   used to emulate 64 bit accesses for hardware devices
+   that are doing 64 bit mmio but have not been converted
+   to use cpu_register_io_memory64.  These functions should
+   only be used until all hardware devices are converted to
+   the new api and then removed because they are not efficient.  */
+static void mem_writeq(void *opaque, target_phys_addr_t addr, uint64_t val)
+{
+    int i;
+
+    for (i = 0; i < IO_MEM_NB_ENTRIES; i++) {
+        if (io_mem_opaque[i] == opaque && io_mem_used[i]) {
+#ifdef TARGET_WORDS_BIGENDIAN
+            io_mem_write[i].l(opaque, addr, val >> 32);
+            io_mem_write[i].l(opaque, addr + 4, val);
+#else
+            io_mem_write[i].l(opaque, addr, val);
+            io_mem_write[i].l(opaque, addr + 4, val >> 32);
+#endif
+#ifdef DEBUG_MMIO64
+            printf("FIXME: emulating 64 bit I/O using 32 bit I/O.\n");
+#endif
+            return;
+        }
+    }
+}
+
+static uint64_t mem_readq(void *opaque, target_phys_addr_t addr)
+{
+    int i;
+
+    for (i = 0; i < IO_MEM_NB_ENTRIES; i++) {
+        if (io_mem_opaque[i] == opaque && io_mem_used[i]) {
+            uint64_t val;
+
+#ifdef TARGET_WORDS_BIGENDIAN
+            val = (uint64_t)io_mem_read[i].l(opaque, addr) << 32;
+            val |= io_mem_read[i].l(opaque, addr + 4);
+#else
+            val = io_mem_read[i].l(opaque, addr);
+            val |= (uint64_t)io_mem_read[i].l(opaque, addr + 4) << 32;
+#endif
+#ifdef DEBUG_MMIO64
+            printf("FIXME: emulating 64 bit I/O using 32 bit I/O.\n");
+#endif
+            return val;
+        }
+    }
+
+    return 0;
+}
+
 /* mem_read and mem_write are arrays of functions containing the
    function to access byte (index 0), word (index 1) and dword (index
    2). Functions can be omitted with a NULL function pointer. The
@@ -3015,8 +3192,38 @@
                            CPUWriteMemoryFunc **mem_write,
                            void *opaque)
 {
-    int i, subwidth = 0;
+    if (io_index <= 0) {
+        io_index = get_free_io_mem_idx();
+        if (io_index == -1)
+            return io_index;
+    } else {
+        if (io_index >= IO_MEM_NB_ENTRIES)
+            return -1;
+    }
 
+    io_mem_read[io_index].b = mem_read[0];
+    io_mem_write[io_index].b = mem_write[0];
+
+    io_mem_read[io_index].w = mem_read[1];
+    io_mem_write[io_index].w = mem_write[1];
+
+    io_mem_read[io_index].l = mem_read[2];
+    io_mem_write[io_index].l = mem_write[2];
+
+    io_mem_read[io_index].q = mem_readq;
+    io_mem_write[io_index].q = mem_writeq;
+
+    io_mem_opaque[io_index] = opaque;
+    return (io_index << IO_MEM_SHIFT) | IO_MEM_SUBWIDTH;
+}
+
+int cpu_register_io_memory64(int io_index,
+                             CPUReadMemoryFuncs *mem_read,
+                             CPUWriteMemoryFuncs *mem_write,
+                             void *opaque)
+{
+    int subwidth = 0;
+
     if (io_index <= 0) {
         io_index = get_free_io_mem_idx();
         if (io_index == -1)
@@ -3026,37 +3233,36 @@
             return -1;
     }
 
-    for(i = 0;i < 3; i++) {
-        if (!mem_read[i] || !mem_write[i])
-            subwidth = IO_MEM_SUBWIDTH;
-        io_mem_read[io_index][i] = mem_read[i];
-        io_mem_write[io_index][i] = mem_write[i];
-    }
+    if (!mem_read->b || !mem_write->b ||
+        !mem_read->w || !mem_write->w ||
+        !mem_read->l || !mem_write->l ||
+        !mem_read->q || !mem_write->q)
+        subwidth = IO_MEM_SUBWIDTH;
+    
+    io_mem_read[io_index] = *mem_read;
+    io_mem_write[io_index] = *mem_write;
     io_mem_opaque[io_index] = opaque;
     return (io_index << IO_MEM_SHIFT) | subwidth;
 }
 
 void cpu_unregister_io_memory(int io_table_address)
 {
-    int i;
     int io_index = io_table_address >> IO_MEM_SHIFT;
 
-    for (i=0;i < 3; i++) {
-        io_mem_read[io_index][i] = unassigned_mem_read[i];
-        io_mem_write[io_index][i] = unassigned_mem_write[i];
-    }
+    io_mem_read[io_index] = unassigned_mem_read;
+    io_mem_write[io_index] = unassigned_mem_write;
     io_mem_opaque[io_index] = NULL;
     io_mem_used[io_index] = 0;
 }
 
 CPUWriteMemoryFunc **cpu_get_io_memory_write(int io_index)
 {
-    return io_mem_write[io_index >> IO_MEM_SHIFT];
+    return &io_mem_write[io_index >> IO_MEM_SHIFT].b;
 }
 
 CPUReadMemoryFunc **cpu_get_io_memory_read(int io_index)
 {
-    return io_mem_read[io_index >> IO_MEM_SHIFT];
+    return &io_mem_read[io_index >> IO_MEM_SHIFT].b;
 }
 
 #endif /* !defined(CONFIG_USER_ONLY) */
@@ -3134,20 +3340,25 @@
                     addr1 = (addr & ~TARGET_PAGE_MASK) + p->region_offset;
                 /* XXX: could force cpu_single_env to NULL to avoid
                    potential bugs */
-                if (l >= 4 && ((addr1 & 3) == 0)) {
+                if (l >= 8 && ((addr1 & 7) == 0)) {
+                    /* 64 bit write access */
+                    uint64_t val64 = ldq_p(buf);
+                    io_mem_write[io_index].q(io_mem_opaque[io_index], addr1, val64);
+                    l = 8;
+                } else if (l >= 4 && ((addr1 & 3) == 0)) {
                     /* 32 bit write access */
                     val = ldl_p(buf);
-                    io_mem_write[io_index][2](io_mem_opaque[io_index], addr1, val);
+                    io_mem_write[io_index].l(io_mem_opaque[io_index], addr1, val);
                     l = 4;
                 } else if (l >= 2 && ((addr1 & 1) == 0)) {
                     /* 16 bit write access */
                     val = lduw_p(buf);
-                    io_mem_write[io_index][1](io_mem_opaque[io_index], addr1, val);
+                    io_mem_write[io_index].w(io_mem_opaque[io_index], addr1, val);
                     l = 2;
                 } else {
                     /* 8 bit write access */
                     val = ldub_p(buf);
-                    io_mem_write[io_index][0](io_mem_opaque[io_index], addr1, val);
+                    io_mem_write[io_index].b(io_mem_opaque[io_index], addr1, val);
                     l = 1;
                 }
             } else {
@@ -3172,19 +3383,24 @@
                 io_index = (pd >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
                 if (p)
                     addr1 = (addr & ~TARGET_PAGE_MASK) + p->region_offset;
-                if (l >= 4 && ((addr1 & 3) == 0)) {
+                if (l >= 8 && ((addr1 & 7) == 0)) {
+                    /* 64 bit read access */
+                    uint64_t val64 = io_mem_read[io_index].q(io_mem_opaque[io_index], addr1);
+                    stq_p(buf, val64);
+                    l = 8;
+                } else if (l >= 4 && ((addr1 & 3) == 0)) {
                     /* 32 bit read access */
-                    val = io_mem_read[io_index][2](io_mem_opaque[io_index], addr1);
+                    val = io_mem_read[io_index].l(io_mem_opaque[io_index], addr1);
                     stl_p(buf, val);
                     l = 4;
                 } else if (l >= 2 && ((addr1 & 1) == 0)) {
                     /* 16 bit read access */
-                    val = io_mem_read[io_index][1](io_mem_opaque[io_index], addr1);
+                    val = io_mem_read[io_index].w(io_mem_opaque[io_index], addr1);
                     stw_p(buf, val);
                     l = 2;
                 } else {
                     /* 8 bit read access */
-                    val = io_mem_read[io_index][0](io_mem_opaque[io_index], addr1);
+                    val = io_mem_read[io_index].b(io_mem_opaque[io_index], addr1);
                     stb_p(buf, val);
                     l = 1;
                 }
@@ -3405,7 +3621,7 @@
         io_index = (pd >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
         if (p)
             addr = (addr & ~TARGET_PAGE_MASK) + p->region_offset;
-        val = io_mem_read[io_index][2](io_mem_opaque[io_index], addr);
+        val = io_mem_read[io_index].l(io_mem_opaque[io_index], addr);
     } else {
         /* RAM case */
         ptr = qemu_get_ram_ptr(pd & TARGET_PAGE_MASK) +
@@ -3437,13 +3653,7 @@
         io_index = (pd >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
         if (p)
             addr = (addr & ~TARGET_PAGE_MASK) + p->region_offset;
-#ifdef TARGET_WORDS_BIGENDIAN
-        val = (uint64_t)io_mem_read[io_index][2](io_mem_opaque[io_index], addr) << 32;
-        val |= io_mem_read[io_index][2](io_mem_opaque[io_index], addr + 4);
-#else
-        val = io_mem_read[io_index][2](io_mem_opaque[io_index], addr);
-        val |= (uint64_t)io_mem_read[io_index][2](io_mem_opaque[io_index], addr + 4) << 32;
-#endif
+        val = io_mem_read[io_index].q(io_mem_opaque[io_index], addr);
     } else {
         /* RAM case */
         ptr = qemu_get_ram_ptr(pd & TARGET_PAGE_MASK) +
@@ -3490,7 +3700,7 @@
         io_index = (pd >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
         if (p)
             addr = (addr & ~TARGET_PAGE_MASK) + p->region_offset;
-        io_mem_write[io_index][2](io_mem_opaque[io_index], addr, val);
+        io_mem_write[io_index].l(io_mem_opaque[io_index], addr, val);
     } else {
         unsigned long addr1 = (pd & TARGET_PAGE_MASK) + (addr & ~TARGET_PAGE_MASK);
         ptr = qemu_get_ram_ptr(addr1);
@@ -3526,13 +3736,7 @@
         io_index = (pd >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
         if (p)
             addr = (addr & ~TARGET_PAGE_MASK) + p->region_offset;
-#ifdef TARGET_WORDS_BIGENDIAN
-        io_mem_write[io_index][2](io_mem_opaque[io_index], addr, val >> 32);
-        io_mem_write[io_index][2](io_mem_opaque[io_index], addr + 4, val);
-#else
-        io_mem_write[io_index][2](io_mem_opaque[io_index], addr, val);
-        io_mem_write[io_index][2](io_mem_opaque[io_index], addr + 4, val >> 32);
-#endif
+        io_mem_write[io_index].q(io_mem_opaque[io_index], addr, val);
     } else {
         ptr = qemu_get_ram_ptr(pd & TARGET_PAGE_MASK) +
             (addr & ~TARGET_PAGE_MASK);
@@ -3559,7 +3763,7 @@
         io_index = (pd >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
         if (p)
             addr = (addr & ~TARGET_PAGE_MASK) + p->region_offset;
-        io_mem_write[io_index][2](io_mem_opaque[io_index], addr, val);
+        io_mem_write[io_index].l(io_mem_opaque[io_index], addr, val);
     } else {
         unsigned long addr1;
         addr1 = (pd & TARGET_PAGE_MASK) + (addr & ~TARGET_PAGE_MASK);
Index: exec-all.h
===================================================================
--- exec-all.h	(revision 7190)
+++ exec-all.h	(working copy)
@@ -265,8 +265,8 @@
 
 TranslationBlock *tb_find_pc(unsigned long pc_ptr);
 
-extern CPUWriteMemoryFunc *io_mem_write[IO_MEM_NB_ENTRIES][4];
-extern CPUReadMemoryFunc *io_mem_read[IO_MEM_NB_ENTRIES][4];
+extern CPUWriteMemoryFuncs io_mem_write[IO_MEM_NB_ENTRIES];
+extern CPUReadMemoryFuncs io_mem_read[IO_MEM_NB_ENTRIES];
 extern void *io_mem_opaque[IO_MEM_NB_ENTRIES];
 
 #include "qemu-lock.h"
Index: cpu-all.h
===================================================================
--- cpu-all.h	(revision 7190)
+++ cpu-all.h	(working copy)
@@ -891,6 +891,23 @@
 typedef void CPUWriteMemoryFunc(void *opaque, target_phys_addr_t addr, uint32_t value);
 typedef uint32_t CPUReadMemoryFunc(void *opaque, target_phys_addr_t addr);
 
+typedef void CPUWriteMemoryFunc64(void *opaque, target_phys_addr_t addr, uint64_t value);
+typedef uint64_t CPUReadMemoryFunc64(void *opaque, target_phys_addr_t addr);
+
+typedef struct CPUWriteMemoryFuncs {
+    CPUWriteMemoryFunc   *b;
+    CPUWriteMemoryFunc   *w;
+    CPUWriteMemoryFunc   *l;
+    CPUWriteMemoryFunc64 *q;
+} CPUWriteMemoryFuncs;
+
+typedef struct CPUReadMemoryFuncs {
+    CPUReadMemoryFunc   *b;
+    CPUReadMemoryFunc   *w;
+    CPUReadMemoryFunc   *l;
+    CPUReadMemoryFunc64 *q;
+} CPUReadMemoryFuncs;
+
 void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,
                                          ram_addr_t size,
                                          ram_addr_t phys_offset,
@@ -914,6 +931,10 @@
                            CPUReadMemoryFunc **mem_read,
                            CPUWriteMemoryFunc **mem_write,
                            void *opaque);
+int cpu_register_io_memory64(int io_index,
+                             CPUReadMemoryFuncs *mem_read,
+                             CPUWriteMemoryFuncs *mem_write,
+                             void *opaque);
 void cpu_unregister_io_memory(int table_address);
 CPUWriteMemoryFunc **cpu_get_io_memory_write(int io_index);
 CPUReadMemoryFunc **cpu_get_io_memory_read(int io_index);

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Paul Brook]

> The old array function call is supported so no existing drivers need
> to be modified.  They can continue to do 2 32 bit accesses because 2
> helper functions have been added to break up the accesses automatically.
> However, the helper functions should only be used until all drivers are
> converted to use the structure and then can be removed along
> with the old array functions api.  The replacement of the arrays with
> structures in the drivers is very straightforward for drivers that don't
> do 64 bit I/O and the few that do can be cleaned up to remove the
> work arounds for the lack of true 64 bit I/O by their maintainers.

This is going to be a bit of a pain, and a lot of duplication.  My expectation 
is that most devices don't know/care about 64-bit accesses and want them to 
be automatically split into a pair of 32-bit accesses. I suggest pushing this 
down into the lower level dispatch routines. By my reading your mem_writeq 
helpers are broken if we happen to have multiple regions with the same opaque 
value (this effects at least lsi53c895a.c).

In the interests of avoiding duplication, I'd also implement 
cpu_register_io_memory in terms of cpu_register_io_memory64.

> +    return ((CPUReadMemoryFunc64*)(*mmio->mem_read[idx][3]))
> (mmio->opaque[idx][0][3],  addr + mmio->region_offset[idx][0][3]);

Eww. It would be a good idea to fix this at the same time.

Paul

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Robert Reif]

Paul Brook wrote:
>> The old array function call is supported so no existing drivers need
>> to be modified.  They can continue to do 2 32 bit accesses because 2
>> helper functions have been added to break up the accesses automatically.
>> However, the helper functions should only be used until all drivers are
>> converted to use the structure and then can be removed along
>> with the old array functions api.  The replacement of the arrays with
>> structures in the drivers is very straightforward for drivers that don't
>> do 64 bit I/O and the few that do can be cleaned up to remove the
>> work arounds for the lack of true 64 bit I/O by their maintainers.
>>     
>
> This is going to be a bit of a pain, and a lot of duplication.  My expectation 
> is that most devices don't know/care about 64-bit accesses and want them to 
> be automatically split into a pair of 32-bit accesses. I suggest pushing this 
> down into the lower level dispatch routines. By my reading your mem_writeq 
> helpers are broken if we happen to have multiple regions with the same opaque 
> value (this effects at least lsi53c895a.c).
>
> In the interests of avoiding duplication, I'd also implement 
> cpu_register_io_memory in terms of cpu_register_io_memory64.
>
>   
>> +    return ((CPUReadMemoryFunc64*)(*mmio->mem_read[idx][3]))
>> (mmio->opaque[idx][0][3],  addr + mmio->region_offset[idx][0][3]);
>>     
>
> Eww. It would be a good idea to fix this at the same time.
>
> Paul
>
>   
The right way to do this is to convert the whole tree at once
so we don't need the helper functions and two versions of
cpu_register_io_memory.  Unfortunately people have written
hardware drivers that rely on the current weird behavior by
caching half of a 64 bit access so it can be accessed later.  This doesn't
work if the registers can also be accessed as 32 bits because the
cache trick implies a specific order of access.

95% of the hardware drivers could be trivially converted and
work fine.  It's those few that play games that need to be looked
at carefully and converted properly to do the right thing.  I think
the authors or maintainers of those drivers have the best knowledge
to back out the current workarounds and do 64 bit I/O properly.

That's why I'm taking a 2 step approach using the helper functions
and 2 cpu_register_io_memory functions.  Once the entire tree is
converted to use the structures, the helper functions and the second
cpu_register_io_memory function would be removed.  I can provide
patches to convert sparc to the new API and also convert most of
the simple drivers.  However it's the ones that have workarounds
for the current behavior that need careful scrutiny.

The ideal time for applying this patch would be at the start of a
new development cycle.  95% of the drivers could be converted
immediately and the problem ones could be identified and fixed
afterwards.  The issues with the helper functions doing a linear
scan and not supporting overlapping regions for 64 bit accesses
would only be a problem for a short period of time (maybe a week)
and only for some of the drivers that have 64 bit access issues.

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Paul Brook]

> The right way to do this is to convert the whole tree at once
> so we don't need the helper functions and two versions of
> cpu_register_io_memory.
> 95% of the hardware drivers could be trivially converted and
> work fine.  

I think you're missing my point. It's the 95% of drivers that I don't want to 
have to "convert". I'm working on the assumption that devices that actually 
implement 64-bit transfers are the exception rather than the rule.

So we have three options:

1) Omit the 64-bit handler for most devices. This will trigger the subwith 
code and associated overhead for no good reason[1].
2) Implement a 64-bit handler for every single device. 90% of these are going 
to be identical trivial wrappers round the 32-bit function. Maybe 5% actually 
need 64-bit accesses, and 5% are broken because someone messed up a 
copy/paste.
3) Implement splitting of 64-bit accesses in generic code.  Devices that 
actually care about 64-bit accesses can install a handler. Everything else 
indicates that it wants accesses split, either by a magic handler or a 
different registration function.

Your current patch implements a broken version of (3), with a vague hope that 
we'll switch to (2) at some time in the future.

I'm suggesting we implement (3) properly from the start.

Paul

[1] I still don't understand why the subwidth code exists, It's seems rather 
unlikely we'll have two different devices responding to different types of 
access the same address range. That's a separate argument though.

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Robert Reif]

[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]

Paul Brook wrote:
>> The right way to do this is to convert the whole tree at once
>> so we don't need the helper functions and two versions of
>> cpu_register_io_memory.
>> 95% of the hardware drivers could be trivially converted and
>> work fine.  
>>     
>
> I think you're missing my point. It's the 95% of drivers that I don't want to 
> have to "convert". I'm working on the assumption that devices that actually 
> implement 64-bit transfers are the exception rather than the rule.
>
> So we have three options:
>
> 1) Omit the 64-bit handler for most devices. This will trigger the subwith 
> code and associated overhead for no good reason[1].
> 2) Implement a 64-bit handler for every single device. 90% of these are going 
> to be identical trivial wrappers round the 32-bit function. Maybe 5% actually 
> need 64-bit accesses, and 5% are broken because someone messed up a 
> copy/paste.
> 3) Implement splitting of 64-bit accesses in generic code.  Devices that 
> actually care about 64-bit accesses can install a handler. Everything else 
> indicates that it wants accesses split, either by a magic handler or a 
> different registration function.
>
> Your current patch implements a broken version of (3), with a vague hope that 
> we'll switch to (2) at some time in the future.
>
> I'm suggesting we implement (3) properly from the start.
>
> Paul
>
> [1] I still don't understand why the subwidth code exists, It's seems rather 
> unlikely we'll have two different devices responding to different types of 
> access the same address range. That's a separate argument though.
>
>   
Here is a patch for most of the sparc32 hardware drivers.  It's
a very trivial and mechanical process for these drivers.  The one
driver that does 64 bit accesses just adds 64 bit access functions
because it's broken now and has no workaround to remove.  I don't
think converting most other drivers will be much harder.

[-- Attachment #2: hw.diff.txt --]
[-- Type: text/plain, Size: 17209 bytes --]

Index: hw/sparc32_dma.c
===================================================================
--- hw/sparc32_dma.c	(revision 7191)
+++ hw/sparc32_dma.c	(working copy)
@@ -199,16 +199,18 @@
     s->dmaregs[saddr] = val;
 }
 
-static CPUReadMemoryFunc *dma_mem_read[3] = {
+static CPUReadMemoryFuncs dma_mem_read = {
     NULL,
     NULL,
     dma_mem_readl,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *dma_mem_write[3] = {
+static CPUWriteMemoryFuncs dma_mem_write = {
     NULL,
     NULL,
     dma_mem_writel,
+    NULL,
 };
 
 static void dma_reset(void *opaque)
@@ -252,7 +254,7 @@
     s->irq = parent_irq;
     s->iommu = iommu;
 
-    dma_io_memory = cpu_register_io_memory(0, dma_mem_read, dma_mem_write, s);
+    dma_io_memory = cpu_register_io_memory64(0, &dma_mem_read, &dma_mem_write, s);
     cpu_register_physical_memory(daddr, DMA_SIZE, dma_io_memory);
 
     register_savevm("sparc32_dma", daddr, 2, dma_save, dma_load, s);
Index: hw/slavio_intctl.c
===================================================================
--- hw/slavio_intctl.c	(revision 7191)
+++ hw/slavio_intctl.c	(working copy)
@@ -132,16 +132,18 @@
     }
 }
 
-static CPUReadMemoryFunc *slavio_intctl_mem_read[3] = {
+static CPUReadMemoryFuncs slavio_intctl_mem_read = {
     NULL,
     NULL,
     slavio_intctl_mem_readl,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *slavio_intctl_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_intctl_mem_write = {
     NULL,
     NULL,
     slavio_intctl_mem_writel,
+    NULL,
 };
 
 // master system interrupt controller
@@ -206,16 +208,18 @@
     }
 }
 
-static CPUReadMemoryFunc *slavio_intctlm_mem_read[3] = {
+static CPUReadMemoryFuncs slavio_intctlm_mem_read = {
     NULL,
     NULL,
     slavio_intctlm_mem_readl,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *slavio_intctlm_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_intctlm_mem_write = {
     NULL,
     NULL,
     slavio_intctlm_mem_writel,
+    NULL,
 };
 
 void slavio_pic_info(Monitor *mon, void *opaque)
@@ -388,10 +392,10 @@
         slave->cpu = i;
         slave->master = s;
 
-        slavio_intctl_io_memory = cpu_register_io_memory(0,
-                                                         slavio_intctl_mem_read,
-                                                         slavio_intctl_mem_write,
-                                                         slave);
+        slavio_intctl_io_memory = cpu_register_io_memory64(0,
+                                                           &slavio_intctl_mem_read,
+                                                           &slavio_intctl_mem_write,
+                                                           slave);
         cpu_register_physical_memory(addr + i * TARGET_PAGE_SIZE, INTCTL_SIZE,
                                      slavio_intctl_io_memory);
 
@@ -399,10 +403,10 @@
         s->cpu_irqs[i] = parent_irq[i];
     }
 
-    slavio_intctlm_io_memory = cpu_register_io_memory(0,
-                                                      slavio_intctlm_mem_read,
-                                                      slavio_intctlm_mem_write,
-                                                      s);
+    slavio_intctlm_io_memory = cpu_register_io_memory64(0,
+                                                        &slavio_intctlm_mem_read,
+                                                        &slavio_intctlm_mem_write,
+                                                        s);
     cpu_register_physical_memory(addrg, INTCTLM_SIZE, slavio_intctlm_io_memory);
 
     register_savevm("slavio_intctl", addr, 1, slavio_intctl_save,
Index: hw/tcx.c
===================================================================
--- hw/tcx.c	(revision 7191)
+++ hw/tcx.c	(working copy)
@@ -463,16 +463,18 @@
     return;
 }
 
-static CPUReadMemoryFunc *tcx_dac_read[3] = {
+static CPUReadMemoryFuncs tcx_dac_read = {
     NULL,
     NULL,
     tcx_dac_readl,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *tcx_dac_write[3] = {
+static CPUWriteMemoryFuncs tcx_dac_write = {
     NULL,
     NULL,
     tcx_dac_writel,
+    NULL,
 };
 
 static uint32_t tcx_dummy_readl(void *opaque, target_phys_addr_t addr)
@@ -485,16 +487,18 @@
 {
 }
 
-static CPUReadMemoryFunc *tcx_dummy_read[3] = {
+static CPUReadMemoryFuncs tcx_dummy_read = {
     NULL,
     NULL,
     tcx_dummy_readl,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *tcx_dummy_write[3] = {
+static CPUWriteMemoryFuncs tcx_dummy_write = {
     NULL,
     NULL,
     tcx_dummy_writel,
+    NULL,
 };
 
 void tcx_init(target_phys_addr_t addr, int vram_size, int width, int height,
@@ -523,12 +527,12 @@
     vram_offset += size;
     vram_base += size;
 
-    io_memory = cpu_register_io_memory(0, tcx_dac_read, tcx_dac_write, s);
+    io_memory = cpu_register_io_memory64(0, &tcx_dac_read, &tcx_dac_write, s);
     cpu_register_physical_memory(addr + 0x00200000ULL, TCX_DAC_NREGS,
                                  io_memory);
 
-    dummy_memory = cpu_register_io_memory(0, tcx_dummy_read, tcx_dummy_write,
-                                          s);
+    dummy_memory = cpu_register_io_memory64(0, &tcx_dummy_read,
+                                            &tcx_dummy_write, s);
     cpu_register_physical_memory(addr + 0x00700000ULL, TCX_TEC_NREGS,
                                  dummy_memory);
     if (depth == 24) {
Index: hw/iommu.c
===================================================================
--- hw/iommu.c	(revision 7191)
+++ hw/iommu.c	(working copy)
@@ -236,16 +236,18 @@
     }
 }
 
-static CPUReadMemoryFunc *iommu_mem_read[3] = {
+static CPUReadMemoryFuncs iommu_mem_read = {
     NULL,
     NULL,
     iommu_mem_readl,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *iommu_mem_write[3] = {
+static CPUWriteMemoryFuncs iommu_mem_write = {
     NULL,
     NULL,
     iommu_mem_writel,
+    NULL,
 };
 
 static uint32_t iommu_page_get_flags(IOMMUState *s, target_phys_addr_t addr)
@@ -375,8 +377,8 @@
     s->version = version;
     s->irq = irq;
 
-    iommu_io_memory = cpu_register_io_memory(0, iommu_mem_read,
-                                             iommu_mem_write, s);
+    iommu_io_memory = cpu_register_io_memory64(0, &iommu_mem_read,
+                                               &iommu_mem_write, s);
     cpu_register_physical_memory(addr, IOMMU_NREGS * 4, iommu_io_memory);
 
     register_savevm("iommu", addr, 2, iommu_save, iommu_load, s);
Index: hw/esp.c
===================================================================
--- hw/esp.c	(revision 7191)
+++ hw/esp.c	(working copy)
@@ -563,16 +563,18 @@
     s->wregs[saddr] = val;
 }
 
-static CPUReadMemoryFunc *esp_mem_read[3] = {
+static CPUReadMemoryFuncs esp_mem_read = {
     esp_mem_readb,
     NULL,
     NULL,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *esp_mem_write[3] = {
+static CPUWriteMemoryFuncs esp_mem_write = {
     esp_mem_writeb,
     NULL,
     esp_mem_writeb,
+    NULL,
 };
 
 static void esp_save(QEMUFile *f, void *opaque)
@@ -660,7 +662,8 @@
     s->dma_memory_write = dma_memory_write;
     s->dma_opaque = dma_opaque;
 
-    esp_io_memory = cpu_register_io_memory(0, esp_mem_read, esp_mem_write, s);
+    esp_io_memory = cpu_register_io_memory64(0, &esp_mem_read, &esp_mem_write,
+                                             s);
     cpu_register_physical_memory(espaddr, ESP_REGS << it_shift, esp_io_memory);
 
     esp_reset(s);
Index: hw/m48t59.c
===================================================================
--- hw/m48t59.c	(revision 7191)
+++ hw/m48t59.c	(working copy)
@@ -565,16 +565,18 @@
     return retval;
 }
 
-static CPUWriteMemoryFunc *nvram_write[] = {
-    &nvram_writeb,
-    &nvram_writew,
-    &nvram_writel,
+static CPUWriteMemoryFuncs nvram_write = {
+    nvram_writeb,
+    nvram_writew,
+    nvram_writel,
+    NULL,
 };
 
-static CPUReadMemoryFunc *nvram_read[] = {
-    &nvram_readb,
-    &nvram_readw,
-    &nvram_readl,
+static CPUReadMemoryFuncs nvram_read = {
+    nvram_readb,
+    nvram_readw,
+    nvram_readl,
+    NULL,
 };
 
 static void m48t59_save(QEMUFile *f, void *opaque)
@@ -632,7 +634,8 @@
         register_ioport_write(io_base, 0x04, 1, NVRAM_writeb, s);
     }
     if (mem_base != 0) {
-        s->mem_index = cpu_register_io_memory(0, nvram_read, nvram_write, s);
+        s->mem_index = cpu_register_io_memory64(0, &nvram_read, &nvram_write,
+                                                s);
         cpu_register_physical_memory(mem_base, size, s->mem_index);
     }
     if (type == 59) {
Index: hw/slavio_misc.c
===================================================================
--- hw/slavio_misc.c	(revision 7191)
+++ hw/slavio_misc.c	(working copy)
@@ -128,16 +128,18 @@
     return ret;
 }
 
-static CPUReadMemoryFunc *slavio_cfg_mem_read[3] = {
+static CPUReadMemoryFuncs slavio_cfg_mem_read = {
     slavio_cfg_mem_readb,
     NULL,
     NULL,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *slavio_cfg_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_cfg_mem_write = {
     slavio_cfg_mem_writeb,
     NULL,
     NULL,
+    NULL,
 };
 
 static void slavio_diag_mem_writeb(void *opaque, target_phys_addr_t addr,
@@ -159,16 +161,18 @@
     return ret;
 }
 
-static CPUReadMemoryFunc *slavio_diag_mem_read[3] = {
+static CPUReadMemoryFuncs slavio_diag_mem_read = {
     slavio_diag_mem_readb,
     NULL,
     NULL,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *slavio_diag_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_diag_mem_write = {
     slavio_diag_mem_writeb,
     NULL,
     NULL,
+    NULL,
 };
 
 static void slavio_mdm_mem_writeb(void *opaque, target_phys_addr_t addr,
@@ -190,16 +194,18 @@
     return ret;
 }
 
-static CPUReadMemoryFunc *slavio_mdm_mem_read[3] = {
+static CPUReadMemoryFuncs slavio_mdm_mem_read = {
     slavio_mdm_mem_readb,
     NULL,
     NULL,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *slavio_mdm_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_mdm_mem_write = {
     slavio_mdm_mem_writeb,
     NULL,
     NULL,
+    NULL,
 };
 
 static void slavio_aux1_mem_writeb(void *opaque, target_phys_addr_t addr,
@@ -230,16 +236,18 @@
     return ret;
 }
 
-static CPUReadMemoryFunc *slavio_aux1_mem_read[3] = {
+static CPUReadMemoryFuncs slavio_aux1_mem_read = {
     slavio_aux1_mem_readb,
     NULL,
     NULL,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *slavio_aux1_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_aux1_mem_write = {
     slavio_aux1_mem_writeb,
     NULL,
     NULL,
+    NULL,
 };
 
 static void slavio_aux2_mem_writeb(void *opaque, target_phys_addr_t addr,
@@ -269,16 +277,18 @@
     return ret;
 }
 
-static CPUReadMemoryFunc *slavio_aux2_mem_read[3] = {
+static CPUReadMemoryFuncs slavio_aux2_mem_read = {
     slavio_aux2_mem_readb,
     NULL,
     NULL,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *slavio_aux2_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_aux2_mem_write = {
     slavio_aux2_mem_writeb,
     NULL,
     NULL,
+    NULL,
 };
 
 static void apc_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
@@ -297,16 +307,18 @@
     return ret;
 }
 
-static CPUReadMemoryFunc *apc_mem_read[3] = {
+static CPUReadMemoryFuncs apc_mem_read = {
     apc_mem_readb,
     NULL,
     NULL,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *apc_mem_write[3] = {
+static CPUWriteMemoryFuncs apc_mem_write = {
     apc_mem_writeb,
     NULL,
     NULL,
+    NULL,
 };
 
 static uint32_t slavio_sysctrl_mem_readl(void *opaque, target_phys_addr_t addr)
@@ -343,16 +355,18 @@
     }
 }
 
-static CPUReadMemoryFunc *slavio_sysctrl_mem_read[3] = {
+static CPUReadMemoryFuncs slavio_sysctrl_mem_read = {
     NULL,
     NULL,
     slavio_sysctrl_mem_readl,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *slavio_sysctrl_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_sysctrl_mem_write = {
     NULL,
     NULL,
     slavio_sysctrl_mem_writel,
+    NULL,
 };
 
 static uint32_t slavio_led_mem_readw(void *opaque, target_phys_addr_t addr)
@@ -386,16 +400,18 @@
     }
 }
 
-static CPUReadMemoryFunc *slavio_led_mem_read[3] = {
+static CPUReadMemoryFuncs slavio_led_mem_read = {
     NULL,
     slavio_led_mem_readw,
     NULL,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *slavio_led_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_led_mem_write = {
     NULL,
     slavio_led_mem_writew,
     NULL,
+    NULL,
 };
 
 static void slavio_misc_save(QEMUFile *f, void *opaque)
@@ -448,50 +464,50 @@
         /* 8 bit registers */
 
         // Slavio control
-        io = cpu_register_io_memory(0, slavio_cfg_mem_read,
-                                    slavio_cfg_mem_write, s);
+        io = cpu_register_io_memory64(0, &slavio_cfg_mem_read,
+                                      &slavio_cfg_mem_write, s);
         cpu_register_physical_memory(base + MISC_CFG, MISC_SIZE, io);
 
         // Diagnostics
-        io = cpu_register_io_memory(0, slavio_diag_mem_read,
-                                    slavio_diag_mem_write, s);
+        io = cpu_register_io_memory64(0, &slavio_diag_mem_read,
+                                      &slavio_diag_mem_write, s);
         cpu_register_physical_memory(base + MISC_DIAG, MISC_SIZE, io);
 
         // Modem control
-        io = cpu_register_io_memory(0, slavio_mdm_mem_read,
-                                    slavio_mdm_mem_write, s);
+        io = cpu_register_io_memory64(0, &slavio_mdm_mem_read,
+                                      &slavio_mdm_mem_write, s);
         cpu_register_physical_memory(base + MISC_MDM, MISC_SIZE, io);
 
         /* 16 bit registers */
-        io = cpu_register_io_memory(0, slavio_led_mem_read,
-                                    slavio_led_mem_write, s);
+        io = cpu_register_io_memory64(0, &slavio_led_mem_read,
+                                      &slavio_led_mem_write, s);
         /* ss600mp diag LEDs */
         cpu_register_physical_memory(base + MISC_LEDS, MISC_SIZE, io);
 
         /* 32 bit registers */
-        io = cpu_register_io_memory(0, slavio_sysctrl_mem_read,
-                                    slavio_sysctrl_mem_write, s);
+        io = cpu_register_io_memory64(0, &slavio_sysctrl_mem_read,
+                                      &slavio_sysctrl_mem_write, s);
         // System control
         cpu_register_physical_memory(base + MISC_SYS, SYSCTRL_SIZE, io);
     }
 
     // AUX 1 (Misc System Functions)
     if (aux1_base) {
-        io = cpu_register_io_memory(0, slavio_aux1_mem_read,
-                                    slavio_aux1_mem_write, s);
+        io = cpu_register_io_memory64(0, &slavio_aux1_mem_read,
+                                      &slavio_aux1_mem_write, s);
         cpu_register_physical_memory(aux1_base, MISC_SIZE, io);
     }
 
     // AUX 2 (Software Powerdown Control)
     if (aux2_base) {
-        io = cpu_register_io_memory(0, slavio_aux2_mem_read,
-                                    slavio_aux2_mem_write, s);
+        io = cpu_register_io_memory64(0, &slavio_aux2_mem_read,
+                                      &slavio_aux2_mem_write, s);
         cpu_register_physical_memory(aux2_base, MISC_SIZE, io);
     }
 
     // Power management (APC) XXX: not a Slavio device
     if (power_base) {
-        io = cpu_register_io_memory(0, apc_mem_read, apc_mem_write, s);
+        io = cpu_register_io_memory64(0, &apc_mem_read, &apc_mem_write, s);
         cpu_register_physical_memory(power_base, MISC_SIZE, io);
     }
 
Index: hw/eccmemctl.c
===================================================================
--- hw/eccmemctl.c	(revision 7191)
+++ hw/eccmemctl.c	(working copy)
@@ -220,16 +220,18 @@
     return ret;
 }
 
-static CPUReadMemoryFunc *ecc_mem_read[3] = {
+static CPUReadMemoryFuncs ecc_mem_read = {
     NULL,
     NULL,
     ecc_mem_readl,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *ecc_mem_write[3] = {
+static CPUWriteMemoryFuncs ecc_mem_write = {
     NULL,
     NULL,
     ecc_mem_writel,
+    NULL,
 };
 
 static void ecc_diag_mem_writeb(void *opaque, target_phys_addr_t addr,
@@ -250,16 +252,18 @@
     return ret;
 }
 
-static CPUReadMemoryFunc *ecc_diag_mem_read[3] = {
+static CPUReadMemoryFuncs ecc_diag_mem_read = {
     ecc_diag_mem_readb,
     NULL,
     NULL,
+    NULL,
 };
 
-static CPUWriteMemoryFunc *ecc_diag_mem_write[3] = {
+static CPUWriteMemoryFuncs ecc_diag_mem_write = {
     ecc_diag_mem_writeb,
     NULL,
     NULL,
+    NULL,
 };
 
 static int ecc_load(QEMUFile *f, void *opaque, int version_id)
@@ -325,11 +329,12 @@
     s->regs[0] = version;
     s->irq = irq;
 
-    ecc_io_memory = cpu_register_io_memory(0, ecc_mem_read, ecc_mem_write, s);
+    ecc_io_memory = cpu_register_io_memory64(0, &ecc_mem_read, &ecc_mem_write,
+                                             s);
     cpu_register_physical_memory(base, ECC_SIZE, ecc_io_memory);
     if (version == ECC_MCC) { // SS-600MP only
-        ecc_io_memory = cpu_register_io_memory(0, ecc_diag_mem_read,
-                                               ecc_diag_mem_write, s);
+        ecc_io_memory = cpu_register_io_memory64(0, &ecc_diag_mem_read,
+                                                 &ecc_diag_mem_write, s);
         cpu_register_physical_memory(base + 0x1000, ECC_DIAG_SIZE,
                                      ecc_io_memory);
     }

[-- Attachment #3: slavio_timer.diff.txt --]
[-- Type: text/plain, Size: 3785 bytes --]

Index: hw/slavio_timer.c
===================================================================
--- hw/slavio_timer.c	(revision 7190)
+++ hw/slavio_timer.c	(working copy)
@@ -137,9 +137,17 @@
         // read limit (system counter mode) or read most signifying
         // part of counter (user mode)
         if (slavio_timer_is_user(s)) {
+            uint64_t last_count = (uint64_t)(s->counthigh) << 32 | s->count;
             // read user timer MSW
             slavio_timer_get_out(s);
             ret = s->counthigh | s->reached;
+            if (last_count == TIMER_MAX_COUNT64) { 
+                uint64_t new_count = (uint64_t)ret << 32 | s->count;
+                if (new_count != last_count) {
+                    s->reached = TIMER_REACHED;
+                    ret |= TIMER_REACHED;
+                }
+            }  
         } else {
             // read limit
             // clear irq
@@ -177,6 +185,31 @@
     return ret;
 }
 
+static uint64_t slavio_timer_mem_readq(void *opaque, target_phys_addr_t addr)
+{
+    SLAVIO_TIMERState *s = opaque;
+    uint32_t saddr;
+    uint64_t ret = 0;
+
+    saddr = addr >> 2;
+    switch (saddr) {
+    case TIMER_LIMIT:
+        if (slavio_timer_is_user(s)) {
+            uint64_t last_count = (uint64_t)(s->counthigh) << 32 | s->count;
+            slavio_timer_get_out(s);
+            ret = (uint64_t)(s->counthigh | s->reached) << 32 | s->count;
+            if (last_count == TIMER_MAX_COUNT64 && ret != last_count) {
+                s->reached = TIMER_REACHED;
+                ret |= ((uint64_t)TIMER_REACHED << 32);
+            }  
+        }
+        break;
+    }
+    DPRINTF("read " TARGET_FMT_plx " = %016llx\n", addr, ret);
+
+    return ret;
+}
+
 static void slavio_timer_mem_writel(void *opaque, target_phys_addr_t addr,
                                     uint32_t val)
 {
@@ -303,16 +336,45 @@
     }
 }
 
-static CPUReadMemoryFunc *slavio_timer_mem_read[3] = {
+static void slavio_timer_mem_writeq(void *opaque, target_phys_addr_t addr,
+                                    uint64_t val)
+{
+    SLAVIO_TIMERState *s = opaque;
+    uint32_t saddr;
+
+    DPRINTF("write " TARGET_FMT_plx " %016llx\n", addr, val);
+    saddr = addr >> 2;
+    switch (saddr) {
+    case TIMER_LIMIT:
+        if (slavio_timer_is_user(s)) {
+            uint64_t count;
+
+            s->limit = TIMER_MAX_COUNT64;
+            s->count = val & TIMER_MAX_COUNT64;
+            s->counthigh = (val & TIMER_MAX_COUNT64) >> 32;
+            s->reached = 0;
+            count = ((uint64_t)s->counthigh << 32) | s->count;
+            DPRINTF("processor %d user timer set to %016llx\n", s->slave_index,
+                    count);
+            if (s->timer)
+                ptimer_set_count(s->timer, LIMIT_TO_PERIODS(s->limit - count));
+        }
+        break;
+    }
+}
+
+static CPUReadMemoryFuncs slavio_timer_mem_read = {
     NULL,
     NULL,
     slavio_timer_mem_readl,
+    slavio_timer_mem_readq
 };
 
-static CPUWriteMemoryFunc *slavio_timer_mem_write[3] = {
+static CPUWriteMemoryFuncs slavio_timer_mem_write = {
     NULL,
     NULL,
     slavio_timer_mem_writel,
+    slavio_timer_mem_writeq
 };
 
 static void slavio_timer_save(QEMUFile *f, void *opaque)
@@ -381,8 +443,8 @@
         ptimer_set_period(s->timer, TIMER_PERIOD);
     }
 
-    slavio_timer_io_memory = cpu_register_io_memory(0, slavio_timer_mem_read,
-                                                    slavio_timer_mem_write, s);
+    slavio_timer_io_memory = cpu_register_io_memory64(0, &slavio_timer_mem_read,
+                                                      &slavio_timer_mem_write, s);
     if (master)
         cpu_register_physical_memory(addr, CPU_TIMER_SIZE,
                                      slavio_timer_io_memory);

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Paul Brook]

> Here is a patch for most of the sparc32 hardware drivers.  It's
> a very trivial and mechanical process for these drivers.  The one
> driver that does 64 bit accesses just adds 64 bit access functions
> because it's broken now and has no workaround to remove.  I don't
> think converting most other drivers will be much harder.

sparc hardware is rather abnormal (for qemu at least) because it cares what 
happens when you use the wrong width. Most devices don't care, and having any 
NULL functions is liable to introduce significant overhead.

Paul

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Robert Reif]

Paul Brook wrote:
>> Here is a patch for most of the sparc32 hardware drivers.  It's
>> a very trivial and mechanical process for these drivers.  The one
>> driver that does 64 bit accesses just adds 64 bit access functions
>> because it's broken now and has no workaround to remove.  I don't
>> think converting most other drivers will be much harder.
>>     
>
> sparc hardware is rather abnormal (for qemu at least) because it cares what 
> happens when you use the wrong width. Most devices don't care, and having any 
> NULL functions is liable to introduce significant overhead.
>
> Paul
>
>   
Ok, so that explains the curious code in m48t59.c:

static void nvram_writeb (void *opaque, target_phys_addr_t addr, 
uint32_t value)
{
    m48t59_t *NVRAM = opaque;

    m48t59_write(NVRAM, addr, value & 0xff);
}

static void nvram_writew (void *opaque, target_phys_addr_t addr, 
uint32_t value)
{
    m48t59_t *NVRAM = opaque;

    m48t59_write(NVRAM, addr, (value >> 8) & 0xff);
    m48t59_write(NVRAM, addr + 1, value & 0xff);
}

static void nvram_writel (void *opaque, target_phys_addr_t addr, 
uint32_t value)
{
    m48t59_t *NVRAM = opaque;

    m48t59_write(NVRAM, addr, (value >> 24) & 0xff);
    m48t59_write(NVRAM, addr + 1, (value >> 16) & 0xff);
    m48t59_write(NVRAM, addr + 2, (value >> 8) & 0xff);
    m48t59_write(NVRAM, addr + 3, value & 0xff);
}

So nvram_writeq should be present on non sparc architectures
and actually should be doing 8 byte accesses?  How do we handle
architecture differences like this?  On sparc, it looks like the
sbus controller does this because the actual hardware really
only has an 8 bit bus.

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Paul Brook]

> > sparc hardware is rather abnormal (for qemu at least) because it cares
> > what happens when you use the wrong width. Most devices don't care, and
> > having any NULL functions is liable to introduce significant overhead.
>
> Ok, so that explains the curious code in m48t59.c:
>...
> So nvram_writeq should be present on non sparc architectures
> and actually should be doing 8 byte accesses?  How do we handle
> architecture differences like this?  On sparc, it looks like the
> sbus controller does this because the actual hardware really
> only has an 8 bit bus.

Are there actually any cases where this matters?

My guess is that in pactice we only have certain SPARC devices that need to 
trap when you do a wrong sized access, and for everything else you're told 
not to do that, and qemu can happily return garbage.

If this is the case then the IO_MEM_SUBWIDTH code seems like complete 
overkill. I reccommend ripping it out, and maybe having the registration 
function replace NULL with the unassigned hander.

Paul

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Blue Swirl]

On 5/1/09, Paul Brook <paul@codesourcery.com> wrote:
> > > sparc hardware is rather abnormal (for qemu at least) because it cares
>  > > what happens when you use the wrong width. Most devices don't care, and
>  > > having any NULL functions is liable to introduce significant overhead.
>  >
>
> > Ok, so that explains the curious code in m48t59.c:
>
> >...
>
> > So nvram_writeq should be present on non sparc architectures
>  > and actually should be doing 8 byte accesses?  How do we handle
>  > architecture differences like this?  On sparc, it looks like the
>  > sbus controller does this because the actual hardware really
>  > only has an 8 bit bus.
>
>
> Are there actually any cases where this matters?
>
>  My guess is that in pactice we only have certain SPARC devices that need to
>  trap when you do a wrong sized access, and for everything else you're told
>  not to do that, and qemu can happily return garbage.
>
>  If this is the case then the IO_MEM_SUBWIDTH code seems like complete
>  overkill. I reccommend ripping it out, and maybe having the registration
>  function replace NULL with the unassigned hander.

Maybe the registration could also be changed so that if the device
only cares for (say) 16 bits (and does not want trapping for the wrong
sized access), the 64, 32 and 8 bit cases are emulated at higher
level. This would shrink the code base a bit and maybe fits to the bus
model.

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Edgar E. Iglesias]

On Fri, May 01, 2009 at 06:51:08PM +0300, Blue Swirl wrote:
> On 5/1/09, Paul Brook <paul@codesourcery.com> wrote:
> > > > sparc hardware is rather abnormal (for qemu at least) because it cares
> >  > > what happens when you use the wrong width. Most devices don't care, and
> >  > > having any NULL functions is liable to introduce significant overhead.
> >  >
> >
> > > Ok, so that explains the curious code in m48t59.c:
> >
> > >...
> >
> > > So nvram_writeq should be present on non sparc architectures
> >  > and actually should be doing 8 byte accesses?  How do we handle
> >  > architecture differences like this?  On sparc, it looks like the
> >  > sbus controller does this because the actual hardware really
> >  > only has an 8 bit bus.
> >
> >
> > Are there actually any cases where this matters?
> >
> >  My guess is that in pactice we only have certain SPARC devices that need to
> >  trap when you do a wrong sized access, and for everything else you're told
> >  not to do that, and qemu can happily return garbage.
> >
> >  If this is the case then the IO_MEM_SUBWIDTH code seems like complete
> >  overkill. I reccommend ripping it out, and maybe having the registration
> >  function replace NULL with the unassigned hander.
> 
> Maybe the registration could also be changed so that if the device
> only cares for (say) 16 bits (and does not want trapping for the wrong
> sized access), the 64, 32 and 8 bit cases are emulated at higher
> level. This would shrink the code base a bit and maybe fits to the bus
> model.

I'd appreciate something along these lines :)

Cheers

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Robert Reif]

Blue Swirl wrote:
> On 5/1/09, Paul Brook <paul@codesourcery.com> wrote:
>   
>>>> sparc hardware is rather abnormal (for qemu at least) because it cares
>>>>         
>>  > > what happens when you use the wrong width. Most devices don't care, and
>>  > > having any NULL functions is liable to introduce significant overhead.
>>  >
>>
>>     
>>> Ok, so that explains the curious code in m48t59.c:
>>>       
>>> ...
>>>       
>>> So nvram_writeq should be present on non sparc architectures
>>>       
>>  > and actually should be doing 8 byte accesses?  How do we handle
>>  > architecture differences like this?  On sparc, it looks like the
>>  > sbus controller does this because the actual hardware really
>>  > only has an 8 bit bus.
>>
>>
>> Are there actually any cases where this matters?
>>
>>  My guess is that in pactice we only have certain SPARC devices that need to
>>  trap when you do a wrong sized access, and for everything else you're told
>>  not to do that, and qemu can happily return garbage.
>>
>>  If this is the case then the IO_MEM_SUBWIDTH code seems like complete
>>  overkill. I reccommend ripping it out, and maybe having the registration
>>  function replace NULL with the unassigned hander.
>>     
>
> Maybe the registration could also be changed so that if the device
> only cares for (say) 16 bits (and does not want trapping for the wrong
> sized access), the 64, 32 and 8 bit cases are emulated at higher
> level. This would shrink the code base a bit and maybe fits to the bus
> model.
>
>
>
>   
This sounds like the way to go.  The device would only need to support
its natural bus width and a higher level bus driver would be responsible
for doing the bus width conversion or faulting.  This moves the bus width
conversion code from each driver up to a specific bus driver.  The bus
driver could be a simple as converting NULL to helper functions.

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Robert Reif]

Paul Brook wrote:
>>> sparc hardware is rather abnormal (for qemu at least) because it cares
>>> what happens when you use the wrong width. Most devices don't care, and
>>> having any NULL functions is liable to introduce significant overhead.
>>>       
>> Ok, so that explains the curious code in m48t59.c:
>> ...
>> So nvram_writeq should be present on non sparc architectures
>> and actually should be doing 8 byte accesses?  How do we handle
>> architecture differences like this?  On sparc, it looks like the
>> sbus controller does this because the actual hardware really
>> only has an 8 bit bus.
>>     
>
> Are there actually any cases where this matters?
>
> My guess is that in pactice we only have certain SPARC devices that need to 
> trap when you do a wrong sized access, and for everything else you're told 
> not to do that, and qemu can happily return garbage.
>
> If this is the case then the IO_MEM_SUBWIDTH code seems like complete 
> overkill. I reccommend ripping it out, and maybe having the registration 
> function replace NULL with the unassigned hander.
>
> Paul
>
>
>
>   
Wouldn't making a version of the subpage structure the default
and getting rid of the multiple parallel arrays make more sense.
Then there would only be one type of I/O memory and no special
cases.

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Paul Brook]

> > My guess is that in pactice we only have certain SPARC devices that need
> > to trap when you do a wrong sized access, and for everything else you're
> > told not to do that, and qemu can happily return garbage.
> >
> > If this is the case then the IO_MEM_SUBWIDTH code seems like complete
> > overkill. I reccommend ripping it out, and maybe having the registration
> > function replace NULL with the unassigned hander.
>
> Wouldn't making a version of the subpage structure the default
> and getting rid of the multiple parallel arrays make more sense.
> Then there would only be one type of I/O memory and no special
> cases.

I'm not sure what you're suggesting. I've yet to see any actual use of the 
subwidth code, and the subpage stuff is fairly rare. 

Trying to push the subpage code down into the TLB handler might be possible, 
but you'd have to be careful to avoid performance regressions in the common 
code.

Paul

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Robert Reif]

Robert Reif wrote:
>
> static void nvram_writel (void *opaque, target_phys_addr_t addr, 
> uint32_t value)
> {
>    m48t59_t *NVRAM = opaque;
>
>    m48t59_write(NVRAM, addr, (value >> 24) & 0xff);
>    m48t59_write(NVRAM, addr + 1, (value >> 16) & 0xff);
>    m48t59_write(NVRAM, addr + 2, (value >> 8) & 0xff);
>    m48t59_write(NVRAM, addr + 3, value & 0xff);
> }
>
static void cirrus_vga_mem_writel(void *opaque, target_phys_addr_t addr, 
uint32_t val)
{
#ifdef TARGET_WORDS_BIGENDIAN
    cirrus_vga_mem_writeb(opaque, addr, (val >> 24) & 0xff);
    cirrus_vga_mem_writeb(opaque, addr + 1, (val >> 16) & 0xff);
    cirrus_vga_mem_writeb(opaque, addr + 2, (val >> 8) & 0xff);
    cirrus_vga_mem_writeb(opaque, addr + 3, val & 0xff);
#else
    cirrus_vga_mem_writeb(opaque, addr, val & 0xff);
    cirrus_vga_mem_writeb(opaque, addr + 1, (val >> 8) & 0xff);
    cirrus_vga_mem_writeb(opaque, addr + 2, (val >> 16) & 0xff);
    cirrus_vga_mem_writeb(opaque, addr + 3, (val >> 24) & 0xff);
#endif
}

Should a new intermediate bus layer also do byte swapping?

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Paul Brook]

> Should a new intermediate bus layer also do byte swapping?

Yes, though a bus layer isn't actually a necessary prerequisite.

As I mentioned in the recent "Smarter compilation for target devices" 
thread[1], having individual devices do byteswapping is bogus. A while ago I 
reworked the TLB handling, so there should now be spare bits that can be used 
for things like byteswapping.

Paul

[1] http://lists.gnu.org/archive/html/qemu-devel/2009-04/msg01820.html

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

On 5/2/09, Paul Brook <paul@codesourcery.com> wrote:
> > Should a new intermediate bus layer also do byte swapping?
>
>
> Yes, though a bus layer isn't actually a necessary prerequisite.
>
>  As I mentioned in the recent "Smarter compilation for target devices"
>  thread[1], having individual devices do byteswapping is bogus. A while ago I
>  reworked the TLB handling, so there should now be spare bits that can be used
>  for things like byteswapping.

This version does the byte swapping and width translation in exec.c.
Sparc32 OpenBIOS/Linux does not use the wrong sized accesses, so that
part may be untested.

Some problems:

- Small writes cause a read-modify-write cycle, but I think that may
happen with real devices too. Another solution is to write zero in
other byte lanes without reading.

- The LE/BE distinction does not seem to be very obvious for devices.
I think the board should give the is_le value to the device.

- There is a small performance hit for the native access case. This
could be avoided by extending io_mem_opaque to cover all sizes.

[-- Attachment #2: Emulate_non_native_widths.patch --]
[-- Type: application/mbox, Size: 22862 bytes --]

[-- Attachment #3: Use_new_registration_functions.patch --]
[-- Type: application/mbox, Size: 4761 bytes --]

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Paul Brook]

On Saturday 02 May 2009, Blue Swirl wrote:
> On 5/2/09, Paul Brook <paul@codesourcery.com> wrote:
> > > Should a new intermediate bus layer also do byte swapping?
> >
> > Yes, though a bus layer isn't actually a necessary prerequisite.
> >
> >  As I mentioned in the recent "Smarter compilation for target devices"
> >  thread[1], having individual devices do byteswapping is bogus. A while
> > ago I reworked the TLB handling, so there should now be spare bits that
> > can be used for things like byteswapping.
>
> This version does the byte swapping and width translation in exec.c.
> Sparc32 OpenBIOS/Linux does not use the wrong sized accesses, so that
> part may be untested.

This appears to be adding yet annother layer of indirection, which isn't 
really what I had inmind.

Paul

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Jamie Lokier]

Blue Swirl wrote:
> - Small writes cause a read-modify-write cycle, but I think that may
> happen with real devices too. Another solution is to write zero in
> other byte lanes without reading.

On PCI and I think most buses, there are separate byte-enable pins for
writing each byte lane and the hardware can decide to ignore the
unenabled bytes or not.

-- Jamie

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Edgar E. Iglesias]

On Tue, May 05, 2009 at 02:59:28AM +0100, Jamie Lokier wrote:
> Blue Swirl wrote:
> > - Small writes cause a read-modify-write cycle, but I think that may
> > happen with real devices too. Another solution is to write zero in
> > other byte lanes without reading.
> 
> On PCI and I think most buses, there are separate byte-enable pins for
> writing each byte lane and the hardware can decide to ignore the
> unenabled bytes or not.

>From my experience it is not uncommon for peripheral busses to
don't have the byte strobes. In fact, most (if not all) of the
embedded ones I've worked with are purely 32bit.

Cheers

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Robert Reif]

Paul Brook wrote:
>> The old array function call is supported so no existing drivers need
>> to be modified.  They can continue to do 2 32 bit accesses because 2
>> helper functions have been added to break up the accesses automatically.
>> However, the helper functions should only be used until all drivers are
>> converted to use the structure and then can be removed along
>> with the old array functions api.  The replacement of the arrays with
>> structures in the drivers is very straightforward for drivers that don't
>> do 64 bit I/O and the few that do can be cleaned up to remove the
>> work arounds for the lack of true 64 bit I/O by their maintainers.
>>     
>
> This is going to be a bit of a pain, and a lot of duplication.  My expectation 
> is that most devices don't know/care about 64-bit accesses and want them to 
> be automatically split into a pair of 32-bit accesses. I suggest pushing this 
> down into the lower level dispatch routines. By my reading your mem_writeq 
> helpers are broken if we happen to have multiple regions with the same opaque 
> value (this effects at least lsi53c895a.c).
>
> In the interests of avoiding duplication, I'd also implement 
> cpu_register_io_memory in terms of cpu_register_io_memory64.
>
>   
>> +    return ((CPUReadMemoryFunc64*)(*mmio->mem_read[idx][3]))
>> (mmio->opaque[idx][0][3],  addr + mmio->region_offset[idx][0][3]);
>>     
>
> Eww. It would be a good idea to fix this at the same time.
>
> Paul
>
>
>
>   
The issue this patch is trying to address is not the case where
software is trying to access 2 32 bit registers using a 64 bit
access (because that should fault on sane hardware) but the
case where we have a 64 bit counter and the 64 bit access is
split up into 2 32 bit accesses and the counter ends up being
read twice.  Some drivers work around this by caching half
of the 64 bit access so the cache is accessed for the second
half rather than the hardware.  However some hardware
(like sparc) can access the timer as either 32 or 64 bits so
this trick doesn't work.

Hardware that supports reading 2 32 bit registers with one 64
bit access can have the 64 callback do 2 32 bit accesses.

Re: [Qemu-devel] [PATCH] 64 bit I/O support v7

[Paul Brook]

> Hardware that supports reading 2 32 bit registers with one 64
> bit access can have the 64 callback do 2 32 bit accesses.

Doesn't this include most PCI devices?

Paul