Re: [Qemu-devel] commit 08521e2 breaks SLOF usb boot

[Paolo Bonzini <pbonzini@redhat.com>]

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

Il 14/06/2013 12:32, Nikunj A Dadhania ha scritto:
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>> commit 08521e28c7e6e8cc1f53424a0f845f58d2ed9546
>> Author: Paolo Bonzini <pbonzini@redhat.com>
>> Date:   Fri May 24 12:54:01 2013 +0200
>>
>>     memory: add big endian support to access_with_adjusted_size
>>     
>>     This will be used to split 8-byte access down to two four-byte accesses.
>>     
>>     Reviewed-by: Richard Henderson <rth@twiddle.net>
>>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>>
>> If I hack the above funniness in my USB EHCI driver, somewhere down the
>> qemu crashes at code introduced by this patch:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x0000000000000000 in ?? ()
>> (gdb) bt
>> #0 0x0000000000000000 in ?? ()
>> #1 0x00005555557a0ea4 in access_with_adjusted_size (addr=addr@entry=12, value=value@entry=0x7fffd5a86680, size=size@entry=1, access_size_min=<optimized out>, access_size_max=<optimized out>,
>> access=0x5555557a1f80 <memory_region_oldmmio_write_accessor>, opaque=0x5555567f8ab8) at /home/nikunj/work/power/code/qemu/memory.c:396
>> #2 0x00005555557a5ebb in memory_region_dispatch_write (size=1, data=0, addr=12, mr=0x5555567f8ab8) at /home/nikunj/work/power/code/qemu/memory.c:998
>>
>> Reverting this, I can safely boot using a usb-storage device put on ehci controller.
> 
> Just reverting this patch does not help though, i will need to figure
> which all commits are bad.

Hi Nikunj,

can you try the attached patch?

Alexey, with some luck it may even fix virtio-blk too.

Paolo

[-- Attachment #2: bigendian.patch --]
[-- Type: text/x-patch, Size: 9073 bytes --]

diff --git a/exec.c b/exec.c
index c99a883..c8658c6 100644
--- a/exec.c
+++ b/exec.c
@@ -1379,7 +1379,7 @@ static void *qemu_safe_ram_ptr(ram_addr_t addr)
 
 /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
  * but takes a size argument */
-static void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
+static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
 {
     if (*size == 0) {
         return NULL;
@@ -1898,14 +1898,10 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
-    unsigned access_size_min = mr->ops->impl.min_access_size;
-    unsigned access_size_max = mr->ops->impl.max_access_size;
+    unsigned access_size_max = mr->ops->valid.max_access_size;
 
     /* Regions are assumed to support 1-4 byte accesses unless
        otherwise specified.  */
-    if (access_size_min == 0) {
-        access_size_min = 1;
-    }
     if (access_size_max == 0) {
         access_size_max = 4;
     }
@@ -1922,9 +1918,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
     if (l > access_size_max) {
         l = access_size_max;
     }
-    /* ??? The users of this function are wrong, not supporting minimums larger
-       than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
-    assert(l >= access_size_min);
 
     return l;
 }
diff --git a/memory.c b/memory.c
index c8f9a2b..a8db78c 100644
--- a/memory.c
+++ b/memory.c
@@ -339,65 +339,104 @@ static void flatview_simplify(FlatView *view)
     }
 }
 
-static void memory_region_oldmmio_read_accessor(void *opaque,
+static bool memory_region_big_endian(MemoryRegion *mr)
+{
+#ifdef TARGET_WORDS_BIGENDIAN
+    return mr->ops->endianness != DEVICE_LITTLE_ENDIAN;
+#else
+    return mr->ops->endianness == DEVICE_BIG_ENDIAN;
+#endif
+}
+
+static bool memory_region_wrong_endianness(MemoryRegion *mr)
+{
+#ifdef TARGET_WORDS_BIGENDIAN
+    return mr->ops->endianness == DEVICE_LITTLE_ENDIAN;
+#else
+    return mr->ops->endianness == DEVICE_BIG_ENDIAN;
+#endif
+}
+
+static void adjust_endianness(MemoryRegion *mr, uint64_t *data, unsigned size)
+{
+    if (memory_region_wrong_endianness(mr)) {
+        switch (size) {
+        case 1:
+            break;
+        case 2:
+            *data = bswap16(*data);
+            break;
+        case 4:
+            *data = bswap32(*data);
+            break;
+        case 8:
+            *data = bswap64(*data);
+            break;
+        default:
+            abort();
+        }
+    }
+}
+
+static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
                                                 hwaddr addr,
                                                 uint64_t *value,
                                                 unsigned size,
                                                 unsigned shift,
                                                 uint64_t mask)
 {
-    MemoryRegion *mr = opaque;
     uint64_t tmp;
 
     tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
+    adjust_endianness(mr, &tmp, size);
     *value |= (tmp & mask) << shift;
 }
 
-static void memory_region_read_accessor(void *opaque,
+static void memory_region_read_accessor(MemoryRegion *mr,
                                         hwaddr addr,
                                         uint64_t *value,
                                         unsigned size,
                                         unsigned shift,
                                         uint64_t mask)
 {
-    MemoryRegion *mr = opaque;
     uint64_t tmp;
 
     if (mr->flush_coalesced_mmio) {
         qemu_flush_coalesced_mmio_buffer();
     }
     tmp = mr->ops->read(mr->opaque, addr, size);
+    adjust_endianness(mr, &tmp, size);
     *value |= (tmp & mask) << shift;
 }
 
-static void memory_region_oldmmio_write_accessor(void *opaque,
+static void memory_region_oldmmio_write_accessor(MemoryRegion *mr,
                                                  hwaddr addr,
                                                  uint64_t *value,
                                                  unsigned size,
                                                  unsigned shift,
                                                  uint64_t mask)
 {
-    MemoryRegion *mr = opaque;
     uint64_t tmp;
 
     tmp = (*value >> shift) & mask;
+    adjust_endianness(mr, &tmp, size);
     mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
 }
 
-static void memory_region_write_accessor(void *opaque,
+static void memory_region_write_accessor(MemoryRegion *mr,
                                          hwaddr addr,
                                          uint64_t *value,
                                          unsigned size,
                                          unsigned shift,
                                          uint64_t mask)
 {
-    MemoryRegion *mr = opaque;
     uint64_t tmp;
 
     if (mr->flush_coalesced_mmio) {
         qemu_flush_coalesced_mmio_buffer();
     }
     tmp = (*value >> shift) & mask;
+    adjust_endianness(mr, &tmp, size);
     mr->ops->write(mr->opaque, addr, tmp, size);
 }
 
@@ -406,13 +445,13 @@ static void access_with_adjusted_size(hwaddr addr,
                                       unsigned size,
                                       unsigned access_size_min,
                                       unsigned access_size_max,
-                                      void (*access)(void *opaque,
+                                      void (*access)(MemoryRegion *mr,
                                                      hwaddr addr,
                                                      uint64_t *value,
                                                      unsigned size,
                                                      unsigned shift,
                                                      uint64_t mask),
-                                      void *opaque)
+                                      MemoryRegion *mr)
 {
     uint64_t access_mask;
     unsigned access_size;
@@ -428,13 +467,15 @@ static void access_with_adjusted_size(hwaddr addr,
     /* FIXME: support unaligned access? */
     access_size = MAX(MIN(size, access_size_max), access_size_min);
     access_mask = -1ULL >> (64 - access_size * 8);
-    for (i = 0; i < size; i += access_size) {
-#ifdef TARGET_WORDS_BIGENDIAN
-        access(opaque, addr + i, value, access_size,
-               (size - access_size - i) * 8, access_mask);
-#else
-        access(opaque, addr + i, value, access_size, i * 8, access_mask);
-#endif
+    if (memory_region_big_endian(mr)) {
+        for (i = 0; i < size; i += access_size) {
+            access(mr, addr + i, value, access_size,
+                   (size - access_size - i) * 8, access_mask);
+        }
+    } else {
+        for (i = 0; i < size; i += access_size) {
+            access(mr, addr + i, value, access_size, i * 8, access_mask);
+        }
     }
 }
 
@@ -786,15 +827,6 @@ static void memory_region_destructor_rom_device(MemoryRegion *mr)
     qemu_ram_free(mr->ram_addr & TARGET_PAGE_MASK);
 }
 
-static bool memory_region_wrong_endianness(MemoryRegion *mr)
-{
-#ifdef TARGET_WORDS_BIGENDIAN
-    return mr->ops->endianness == DEVICE_LITTLE_ENDIAN;
-#else
-    return mr->ops->endianness == DEVICE_BIG_ENDIAN;
-#endif
-}
-
 void memory_region_init(MemoryRegion *mr,
                         Object *owner,
                         const char *name,
@@ -841,7 +872,7 @@ static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
     if (current_cpu != NULL) {
         cpu_unassigned_access(current_cpu, addr, false, false, 0, size);
     }
-    return 0;
+    return -1ULL;
 }
 
 static void unassigned_mem_write(void *opaque, hwaddr addr,
@@ -922,27 +953,6 @@ static uint64_t memory_region_dispatch_read1(MemoryRegion *mr,
     return data;
 }
 
-static void adjust_endianness(MemoryRegion *mr, uint64_t *data, unsigned size)
-{
-    if (memory_region_wrong_endianness(mr)) {
-        switch (size) {
-        case 1:
-            break;
-        case 2:
-            *data = bswap16(*data);
-            break;
-        case 4:
-            *data = bswap32(*data);
-            break;
-        case 8:
-            *data = bswap64(*data);
-            break;
-        default:
-            abort();
-        }
-    }
-}
-
 static bool memory_region_dispatch_read(MemoryRegion *mr,
                                         hwaddr addr,
                                         uint64_t *pval,
@@ -954,7 +964,6 @@ static bool memory_region_dispatch_read(MemoryRegion *mr,
     }
 
     *pval = memory_region_dispatch_read1(mr, addr, size);
-    adjust_endianness(mr, pval, size);
     return false;
 }
 
@@ -968,8 +977,6 @@ static bool memory_region_dispatch_write(MemoryRegion *mr,
         return true;
     }
 
-    adjust_endianness(mr, &data, size);
-
     if (mr->ops->write) {
         access_with_adjusted_size(addr, &data, size,
                                   mr->ops->impl.min_access_size,