Re: [Qemu-devel] [PATCH] pci: fix bridge IO/BASE

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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

On 04/03/12 19:51, Blue Swirl wrote:

> I now know the root cause of the problem. OpenBIOS programs the BARs
> somewhat correctly just by accident. The initial io_base and mem_base
> for BARs are not correct, but because the host bridge BARs (and also 6
> of which 4 are not even BARs!) are programmed first, the bases
> happened to settle to values that happen to work. The commit revealed
> the problem since the settling didn't happen. The mask changes just
> let the host bridge setup continue to do the magic.
>
> By just changing OpenBIOS (see attached patch), I can get the devices
> to work (assuming that VGA is a separate problem). There's no need to
> change QEMU.

Hi Blue/Michael,

Thanks for debugging this. I can confirm that building OpenBIOS SVN 
trunk with Blue's patch and testing against QEMU git master with the 
attached patch (to temporarily back out 
2b50aa1f14d8e0a40f905ad0c022443c15646914 and 
de58ac72b6a062d1a61478284c0c0f8a0428613e which breaks VGA on 
PPC/SPARC64) appears to fix the problem for me.

Thanks a lot to both of you for taking the time to track this down :)


ATB,

Mark.

[-- Attachment #2: qemu-mem-revert.patch --]
[-- Type: text/x-diff, Size: 7390 bytes --]

diff --git a/ioport.c b/ioport.c
index 8a474d3..36fa3a4 100644
--- a/ioport.c
+++ b/ioport.c
@@ -328,7 +328,6 @@ void portio_list_init(PortioList *piolist,
     piolist->ports = callbacks;
     piolist->nr = 0;
     piolist->regions = g_new0(MemoryRegion *, n);
-    piolist->aliases = g_new0(MemoryRegion *, n);
     piolist->address_space = NULL;
     piolist->opaque = opaque;
     piolist->name = name;
@@ -337,7 +336,6 @@ void portio_list_init(PortioList *piolist,
 void portio_list_destroy(PortioList *piolist)
 {
     g_free(piolist->regions);
-    g_free(piolist->aliases);
 }
 
 static void portio_list_add_1(PortioList *piolist,
@@ -347,7 +345,7 @@ static void portio_list_add_1(PortioList *piolist,
 {
     MemoryRegionPortio *pio;
     MemoryRegionOps *ops;
-    MemoryRegion *region, *alias;
+    MemoryRegion *region;
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
@@ -364,20 +362,12 @@ static void portio_list_add_1(PortioList *piolist,
     ops->old_portio = pio;
 
     region = g_new(MemoryRegion, 1);
-    alias = g_new(MemoryRegion, 1);
-    /*
-     * Use an alias so that the callback is called with an absolute address,
-     * rather than an offset relative to to start + off_low.
-     */
     memory_region_init_io(region, ops, piolist->opaque, piolist->name,
-                          UINT64_MAX);
-    memory_region_init_alias(alias, piolist->name,
-                             region, start + off_low, off_high - off_low);
+                          off_high - off_low);
+    memory_region_set_offset(region, start + off_low);
     memory_region_add_subregion(piolist->address_space,
-                                start + off_low, alias);
-    piolist->regions[piolist->nr] = region;
-    piolist->aliases[piolist->nr] = alias;
-    ++piolist->nr;
+                                start + off_low, region);
+    piolist->regions[piolist->nr++] = region;
 }
 
 void portio_list_add(PortioList *piolist,
@@ -419,19 +409,15 @@ void portio_list_add(PortioList *piolist,
 
 void portio_list_del(PortioList *piolist)
 {
-    MemoryRegion *mr, *alias;
+    MemoryRegion *mr;
     unsigned i;
 
     for (i = 0; i < piolist->nr; ++i) {
         mr = piolist->regions[i];
-        alias = piolist->aliases[i];
-        memory_region_del_subregion(piolist->address_space, alias);
-        memory_region_destroy(alias);
+        memory_region_del_subregion(piolist->address_space, mr);
         memory_region_destroy(mr);
         g_free((MemoryRegionOps *)mr->ops);
         g_free(mr);
-        g_free(alias);
         piolist->regions[i] = NULL;
-        piolist->aliases[i] = NULL;
     }
 }
diff --git a/ioport.h b/ioport.h
index ab29c89..ae3e9da 100644
--- a/ioport.h
+++ b/ioport.h
@@ -60,7 +60,6 @@ typedef struct PortioList {
     struct MemoryRegion *address_space;
     unsigned nr;
     struct MemoryRegion **regions;
-    struct MemoryRegion **aliases;
     void *opaque;
     const char *name;
 } PortioList;
diff --git a/memory.c b/memory.c
index 6565e2e..926201a 100644
--- a/memory.c
+++ b/memory.c
@@ -389,17 +389,17 @@ static void memory_region_iorange_read(IORange *iorange,
 
         *data = ((uint64_t)1 << (width * 8)) - 1;
         if (mrp) {
-            *data = mrp->read(mr->opaque, offset);
+            *data = mrp->read(mr->opaque, offset + mr->offset);
         } else if (width == 2) {
             mrp = find_portio(mr, offset, 1, false);
             assert(mrp);
-            *data = mrp->read(mr->opaque, offset) |
-                    (mrp->read(mr->opaque, offset + 1) << 8);
+            *data = mrp->read(mr->opaque, offset + mr->offset) |
+                    (mrp->read(mr->opaque, offset + mr->offset + 1) << 8);
         }
         return;
     }
     *data = 0;
-    access_with_adjusted_size(offset, data, width,
+    access_with_adjusted_size(offset + mr->offset, data, width,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_read_accessor, mr);
@@ -416,16 +416,16 @@ static void memory_region_iorange_write(IORange *iorange,
         const MemoryRegionPortio *mrp = find_portio(mr, offset, width, true);
 
         if (mrp) {
-            mrp->write(mr->opaque, offset, data);
+            mrp->write(mr->opaque, offset + mr->offset, data);
         } else if (width == 2) {
             mrp = find_portio(mr, offset, 1, false);
             assert(mrp);
-            mrp->write(mr->opaque, offset, data & 0xff);
-            mrp->write(mr->opaque, offset + 1, data >> 8);
+            mrp->write(mr->opaque, offset + mr->offset, data & 0xff);
+            mrp->write(mr->opaque, offset + mr->offset + 1, data >> 8);
         }
         return;
     }
-    access_with_adjusted_size(offset, &data, width,
+    access_with_adjusted_size(offset + mr->offset, &data, width,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_write_accessor, mr);
@@ -796,6 +796,7 @@ void memory_region_init(MemoryRegion *mr,
         mr->size = int128_2_64();
     }
     mr->addr = 0;
+    mr->offset = 0;
     mr->subpage = false;
     mr->enabled = true;
     mr->terminates = false;
@@ -857,7 +858,7 @@ static uint64_t memory_region_dispatch_read1(MemoryRegion *mr,
     }
 
     /* FIXME: support unaligned access */
-    access_with_adjusted_size(addr, &data, size,
+    access_with_adjusted_size(addr + mr->offset, &data, size,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_read_accessor, mr);
@@ -911,7 +912,7 @@ static void memory_region_dispatch_write(MemoryRegion *mr,
     }
 
     /* FIXME: support unaligned access */
-    access_with_adjusted_size(addr, &data, size,
+    access_with_adjusted_size(addr + mr->offset, &data, size,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_write_accessor, mr);
@@ -1054,6 +1055,11 @@ bool memory_region_is_rom(MemoryRegion *mr)
     return mr->ram && mr->readonly;
 }
 
+void memory_region_set_offset(MemoryRegion *mr, target_phys_addr_t offset)
+{
+    mr->offset = offset;
+}
+
 void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
 {
     uint8_t mask = 1 << client;
diff --git a/memory.h b/memory.h
index b7bccd1..a9cd586 100644
--- a/memory.h
+++ b/memory.h
@@ -115,6 +115,7 @@ struct MemoryRegion {
     MemoryRegion *parent;
     Int128 size;
     target_phys_addr_t addr;
+    target_phys_addr_t offset;
     void (*destructor)(MemoryRegion *mr);
     ram_addr_t ram_addr;
     IORange iorange;
@@ -370,6 +371,14 @@ bool memory_region_is_rom(MemoryRegion *mr);
 void *memory_region_get_ram_ptr(MemoryRegion *mr);
 
 /**
+ * memory_region_set_offset: Sets an offset to be added to MemoryRegionOps
+ *                           callbacks.
+ *
+ * This function is deprecated and should not be used in new code.
+ */
+void memory_region_set_offset(MemoryRegion *mr, target_phys_addr_t offset);
+
+/**
  * memory_region_set_log: Turn dirty logging on or off for a region.
  *
  * Turns dirty logging on or off for a specified client (display, migration).