64bit PCI BARs allocations

+ pci_config_writel(bdf, PCI_PREF_BASE_UPPER32, base>> 32);

This patch adds PCI_REGION_TYPE_PREFMEM_64 region type and modifies types of variables to make it possible to work with 64 bit addresses.

Why I've added just one region type PCI_REGION_TYPE_PREFMEM_64 and haven't added PCI_REGION_TYPE_MEM_64? According to PCI architecture specification, the
bridges can describe 64bit ranges for prefetchable type of memory only. So it's very
unlikely that devices exporting 64bit non-prefetchable BARs.

Anyway
this code will work
with 64bit non-prefetchable BARs unless the PCI device is not behind the secondary bus.

Signed-off-by Alexey Korolev <[email protected]>

---
 src/pci.h     |    1 -
 src/pciinit.c |   59
+++++++++++++++++++++++++++++++++-----------------------
 2 files changed, 35 insertions(+), 25 deletions(-)

src/pci.h +0 -1

index a2a5a4c..71f15fe 100644
@@ -54,7 +54,6 @@ struct pci_device {
     struct {
         u32 addr;
         u32 size;
-        int is64;
     } bars[PCI_NUM_REGIONS];
 
     // Local information on device.

src/pciinit.c +15 -13

index 7d83368..a574e38 100644
@@ -22,6 +22,7 @@ enum pci_region_type {
     PCI_REGION_TYPE_IO,
     PCI_REGION_TYPE_MEM,
     PCI_REGION_TYPE_PREFMEM,
+    PCI_REGION_TYPE_PREFMEM_64,
     PCI_REGION_TYPE_COUNT,
 };
 

@@ -29,18 +30,20 @@ static const char *region_type_name[] = {
     [ PCI_REGION_TYPE_IO ]      = "io",
     [ PCI_REGION_TYPE_MEM ]     = "mem",
     [ PCI_REGION_TYPE_PREFMEM ] = "prefmem",
+    [ PCI_REGION_TYPE_PREFMEM_64 ] = "prefmem64",
 };
 
 struct pci_bus {
     struct {
-        /* pci region stats */
-        u32 count[32 - PCI_MEM_INDEX_SHIFT];
-        u32 sum, max;
         /* seconday bus region sizes */
         u32 size;
-        /* pci region assignments */
-        u32 bases[32 - PCI_MEM_INDEX_SHIFT];
-        u32 base;
+        /* pci region stats */
+        u32 max;
+        u32 count[32 - PCI_MEM_INDEX_SHIFT];
+        s64 sum;
+         /* pci region assignments */
+        s64 base;
+        s64 bases[32 - PCI_MEM_INDEX_SHIFT];
     } r[PCI_REGION_TYPE_COUNT];
     struct pci_device *bus_dev;
 };

@@ -69,6 +72,8 @@ static enum pci_region_type pci_addr_to_type(u32 addr)
 {
     if (addr & PCI_BASE_ADDRESS_SPACE_IO)
         return PCI_REGION_TYPE_IO;
+    if (addr & PCI_BASE_ADDRESS_MEM_TYPE_64)
+        return PCI_REGION_TYPE_PREFMEM_64;
     if (addr & PCI_BASE_ADDRESS_MEM_PREFETCH)
         return PCI_REGION_TYPE_PREFMEM;
     return PCI_REGION_TYPE_MEM;

@@ -378,19 +383,16 @@ static void pci_bios_check_devices(struct
 pci_bus *busses)
         struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)];
         int i;
         for (i = 0; i < PCI_NUM_REGIONS; i++) {
-            u32 val, size;
+            u32 val, size, type;
             pci_bios_get_bar(pci, i, &val, &size);
             if (val == 0)
                 continue;
 
-            pci_bios_bus_reserve(bus, pci_addr_to_type(val), size);
+            type = pci_addr_to_type(val);
+            pci_bios_bus_reserve(bus, type, size);
             pci->bars[i].addr = val;
             pci->bars[i].size = size;
-            pci->bars[i].is64 = (!(val & PCI_BASE_ADDRESS_SPACE_IO) &&
-                                 (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK)
-                                 == PCI_BASE_ADDRESS_MEM_TYPE_64);
-
-            if (pci->bars[i].is64)
+            if (type == PCI_REGION_TYPE_PREFMEM_64)
                 i++;
         }

     }

src/pciinit.c +1 -0

@@ -426,6 +428,7 @@ static void pci_bios_check_devices(struct
 pci_bus *busses)
 static int pci_bios_init_root_regions(struct pci_bus *bus, u32
 start, u32 end)
 {
     bus->r[PCI_REGION_TYPE_IO].base = 0xc000;
+    bus->r[PCI_REGION_TYPE_PREFMEM_64].base = BUILD_PCIMEM_64_START;
 

     int reg1 = PCI_REGION_TYPE_PREFMEM, reg2 = PCI_REGION_TYPE_MEM;
     if (bus->r[reg1].sum < bus->r[reg2].sum) {

src/pciinit.c +12 -7

@@ -449,29 +452,34 @@ static int pci_bios_init_root_regions(struct
 pci_bus *bus, u32 start, u32 end)
 
 static void pci_bios_init_bus_bases(struct pci_bus *bus)
 {
-    u32 base, newbase, size;
+    u32 size;
+    s64 base, newbase;
     int type, i;
 
     for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
-        dprintf(1, "  type %s max %x sum %x base %x\n",
 region_type_name[type],
-                bus->r[type].max, bus->r[type].sum, bus->r[type].base);
+        dprintf(1, "  type %s max %x sum 0x%08x%08x base %08x%08x\n",
+                region_type_name[type], bus->r[type].max,
+                (u32)(bus->r[type].sum>>32), (u32)bus->r[type].sum,
+                (u32)(bus->r[type].base>>32), (u32)bus->r[type].base);
         base = bus->r[type].base;
         for (i = ARRAY_SIZE(bus->r[type].count)-1; i >= 0; i--) {
             size = pci_index_to_size(i, type);
             if (!bus->r[type].count[i])
                 continue;
             newbase = base + size * bus->r[type].count[i];
-            dprintf(1, "    size %8x: %d bar(s), %8x -> %8x\n",
-                    size, bus->r[type].count[i], base, newbase - 1);
+            dprintf(1, "    size %8x: %d bar(s), %08x%08x -> %08x%08x\n",
+                    size, bus->r[type].count[i], (u32)(base>>32),
 (u32)base,
+                    (u32)((newbase - 1)>>32), (u32)newbase - 1);
             bus->r[type].bases[i] = base;
             base = newbase;
         }
     }
 }
 
-static u32 pci_bios_bus_get_addr(struct pci_bus *bus, int type, u32 size)
+static s64 pci_bios_bus_get_addr(struct pci_bus *bus, int type, u32 size)
 {
-    u32 index, addr;
+    u32 index;
+    s64 addr;

     index = pci_size_to_index(size, type);
     addr = bus->r[type].bases[index];

src/pciinit.c +7 -4

@@ -540,13 +548,16 @@ static void pci_bios_map_devices(struct
 pci_bus *busses)
                 continue;
 
             int type = pci_addr_to_type(pci->bars[i].addr);
-            u32 addr = pci_bios_bus_get_addr(bus, type, pci->bars[i].size);
-            dprintf(1, "  bar %d, addr %x, size %x [%s]\n",
-                    i, addr, pci->bars[i].size, region_type_name[type]);
+            s64 addr = pci_bios_bus_get_addr(bus, type, pci->bars[i].size);
+            dprintf(1, "  bar %d, addr %08x%08x, size %x [%s]\n",
+                    i, (u32)(addr >> 32), (u32)addr, pci->bars[i].size,
+                    region_type_name[type]);
             pci_set_io_region_addr(pci, i, addr);
 
-            if (pci->bars[i].is64)
+            if (type == PCI_REGION_TYPE_PREFMEM_64) {
                 i++;
+                pci_set_io_region_addr(pci, i, addr >> 32);
+            }
         }
     }

}

Signature · 1 line

1.7.5.4

All devices behind a bridge need to have all their regions consecutive and not overlapping with all the normal memory ranges.
Since prefetchable memory is described by one record, we must avoid the situations when 32bit and 64bit prefetchable regions are present within one secondary bus.

Signed-off-by Alexey Korolev<[email protected]>

---
 src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 48 insertions(+), 21 deletions(-)

src/pciinit.c +25 -1

index a574e38..92942d5 100644
@@ -17,6 +17,7 @@
 
 #define PCI_BRIDGE_IO_MIN      0x1000
 #define PCI_BRIDGE_MEM_MIN   0x100000
+#define PCI_BRIDGE_MEM_MAX   0x80000000
 
 enum pci_region_type {
     PCI_REGION_TYPE_IO,

@@ -45,6 +46,7 @@ struct pci_bus {
         s64 base;
         s64 bases[32 - PCI_MEM_INDEX_SHIFT];
     } r[PCI_REGION_TYPE_COUNT];
+    int is64;
     struct pci_device *bus_dev;
 };
 

@@ -369,6 +371,26 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
         bus->r[type].max = size;
 }
 
+static void pci_bios_secondary_bus_reserve(struct pci_bus *parent,
+                                           struct pci_bus *s, int type)
+{
+    u32 limit = (type == PCI_REGION_TYPE_IO) ?
+        PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
+
+    if (s->r[type].sum>  PCI_BRIDGE_MEM_MAX) {
+        panic("Size: %08x%08x is too big\n",
+                (u32)s->r[type].sum, (u32)(s->r[type].sum>>32));
+    }
+    s->r[type].size = (u32)s->r[type].sum;
+    if (s->r[type].size<  limit)
+        s->r[type].size = limit;
+    s->r[type].size = pci_size_roundup(s->r[type].size);
+
+    pci_bios_bus_reserve(parent, type, s->r[type].size);
+    dprintf(1, "    size: %x, type %s\n",
+                s->r[type].size, region_type_name[type]);
+}
+
 static void pci_bios_check_devices(struct pci_bus *busses)
 {
     dprintf(1, "PCI: check devices\n");

@@ -392,8 +414,10 @@ static void pci_bios_check_devices(struct pci_bus *busses)
             pci_bios_bus_reserve(bus, type, size);
             pci->bars[i].addr = val;
             pci->bars[i].size = size;
-            if (type == PCI_REGION_TYPE_PREFMEM_64)
+            if (type == PCI_REGION_TYPE_PREFMEM_64) {
+                bus->is64 = 1;
                 i++;
+            }
         }
     }
 

src/pciinit.c +5 -7

+                   s->r[PCI_REGION_TYPE_PREFMEM].sum) {
+           panic("Sparse PCI prefmem regions on the bus %d\n", secondary_bus);
+        }
+
+        dprintf(1, "PCI: secondary bus %d\n", secondary_bus);
         int type;
         for (type = 0; type<  PCI_REGION_TYPE_COUNT; type++) {
-            u32 limit = (type == PCI_REGION_TYPE_IO) ?
-                PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
-            s->r[type].size = s->r[type].sum;
-            if (s->r[type].size<  limit)
-                s->r[type].size = limit;
-            s->r[type].size = pci_size_roundup(s->r[type].size);
-            pci_bios_bus_reserve(parent, type, s->r[type].size);

src/pciinit.c +13 -6

+            pci_bios_secondary_bus_reserve(parent, s, type);
+       }
+   }
 }
 
 #define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1)

@@ -507,14 +530,17 @@ static void pci_bios_map_devices(struct pci_bus *busses)
         struct pci_bus *parent =&busses[pci_bdf_to_bus(bdf)];
         int type;
         for (type = 0; type<  PCI_REGION_TYPE_COUNT; type++) {
+            if ((type == PCI_REGION_TYPE_PREFMEM_64&&  !s->is64) ||
+                (type == PCI_REGION_TYPE_PREFMEM&&  s->is64))
+                continue;
             s->r[type].base = pci_bios_bus_get_addr(
                 parent, type, s->r[type].size);
         }
         dprintf(1, "PCI: init bases bus %d (secondary)\n", secondary_bus);
         pci_bios_init_bus_bases(s);
 
-        u32 base = s->r[PCI_REGION_TYPE_IO].base;
-        u32 limit = base + s->r[PCI_REGION_TYPE_IO].size - 1;
+        s64 base = s->r[PCI_REGION_TYPE_IO].base;
+        s64 limit = base + s->r[PCI_REGION_TYPE_IO].size - 1;
         pci_config_writeb(bdf, PCI_IO_BASE, base>>  PCI_IO_SHIFT);
         pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
         pci_config_writeb(bdf, PCI_IO_LIMIT, limit>>  PCI_IO_SHIFT);

@@ -525,12 +551,13 @@ static void pci_bios_map_devices(struct pci_bus *busses)
         pci_config_writew(bdf, PCI_MEMORY_BASE, base>>  PCI_MEMORY_SHIFT);
         pci_config_writew(bdf, PCI_MEMORY_LIMIT, limit>>  PCI_MEMORY_SHIFT);
 
-        base = s->r[PCI_REGION_TYPE_PREFMEM].base;
-        limit = base + s->r[PCI_REGION_TYPE_PREFMEM].size - 1;
+        type = s->is64 ? PCI_REGION_TYPE_PREFMEM_64 : PCI_REGION_TYPE_PREFMEM;
+        base = s->r[type].base;
+        limit = base + s->r[type].size - 1;
         pci_config_writew(bdf, PCI_PREF_MEMORY_BASE, base>>  PCI_PREF_MEMORY_SHIFT);
         pci_config_writew(bdf, PCI_PREF_MEMORY_LIMIT, limit>>  PCI_PREF_MEMORY_SHIFT);
-        pci_config_writel(bdf, PCI_PREF_BASE_UPPER32, 0);
-        pci_config_writel(bdf, PCI_PREF_LIMIT_UPPER32, 0);
+        pci_config_writel(bdf, PCI_PREF_BASE_UPPER32, base>>  32);
+        pci_config_writel(bdf, PCI_PREF_LIMIT_UPPER32, limit>>  32);
     }
 
     // Map regions on each device.

Signature · 1 line

1.7.5.4

Hi,

There were a number of requests about support of 64bit PCI BAR allocations.

Also we have observed the issue on guests with older linux version (2.6.18): if we
have a 64bit BAR allocated within first 4GB, the OS may hang during start process.
(I guess it is an OS bug, but we need to take care of this).

This patch addresses these two issues and allows 64bit BARs to be allocated in ranges
above 4GB.
Patch consists of three parts:
1. Add new range above 4GB in _CRS table to let Windows 2008 work properly. Thanks
a lot to Michael S. Triskin for this brilliant idea.
2. Added new PCI_REGION_TYPE_PREFMEM_64 region type in pciinit and changed types of variables.
3. Take care about PCI devices with 64bit BARs on secondary buses.

Patches have been tested on several configurations which includes linux 2.6.18 - 3.0 &
windows 2008. Everything works quite well.

This patch adds PCI_REGION_TYPE_PREFMEM_64 region type and modifies types of variables to make it possible to work with 64 bit addresses.

Why I've added just one region type PCI_REGION_TYPE_PREFMEM_64 and haven't added PCI_REGION_TYPE_MEM_64? According to PCI architecture specification, the
bridges can describe 64bit ranges for prefetchable type of memory only. So it's very
unlikely that devices exporting 64bit non-prefetchable BARs. Anyway this code will work
with 64bit non-prefetchable BARs unless the PCI device is not behind the secondary bus.

src/pciinit.c +0 -0

 struct pci_bus {

     struct {

src/pciinit.c +0 -3

-        /* pci region stats */
-        u32 count[32 - PCI_MEM_INDEX_SHIFT];
-        u32 sum, max;
         /* seconday bus region sizes */
         u32 size;

src/pciinit.c +2 -2

@@ -378,19 +383,16 @@ static void pci_bios_check_devices(struct
 pci_bus *busses)
         struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)];
         int i;
         for (i = 0; i < PCI_NUM_REGIONS; i++) {
-            u32 val, size;
+            u32 val, size, type;
             pci_bios_get_bar(pci, i, &val, &size);
             if (val == 0)
                 continue;
 
-            pci_bios_bus_reserve(bus, pci_addr_to_type(val), size);
+            type = pci_addr_to_type(val);

Quoted 9 lines · Alexey Korolev

This patch adds PCI_REGION_TYPE_PREFMEM_64 region type and modifies types of variables to make it possible to work with 64 bit addresses.

Why I've added just one region type PCI_REGION_TYPE_PREFMEM_64 and haven't added PCI_REGION_TYPE_MEM_64? According to PCI architecture specification, the
bridges can describe 64bit ranges for prefetchable type of memory only. So it's very
unlikely that devices exporting 64bit non-prefetchable BARs.

Might happen for system devices I guess.

Quoted 4 lines

Anyway
this code will work
with 64bit non-prefetchable BARs unless the PCI device is not behind the secondary bus.

So what happens if such a device is on root bus?

Quoted 12 lines · Alexey Korolev

This patch adds PCI_REGION_TYPE_PREFMEM_64 region type and modifies types of variables to make it possible to work with 64 bit addresses.

Why I've added just one region type PCI_REGION_TYPE_PREFMEM_64 and haven't added PCI_REGION_TYPE_MEM_64? According to PCI architecture specification, the
bridges can describe 64bit ranges for prefetchable type of memory only. So it's very
unlikely that devices exporting 64bit non-prefetchable BARs. Anyway this code will work
with 64bit non-prefetchable BARs unless the PCI device is not behind the secondary bus.

[...]

src/pciinit.c enum pci_region_type { · line 27

Quoted 9 lines

src/pciinit.c +1 -0

@@ -22,6 +22,7 @@ enum pci_region_type {
      PCI_REGION_TYPE_IO,
      PCI_REGION_TYPE_MEM,
      PCI_REGION_TYPE_PREFMEM,
+    PCI_REGION_TYPE_PREFMEM_64,
      PCI_REGION_TYPE_COUNT,
  };

This doesn't seem right. A 64bit bar is not a new category - it's just a way of representing larger values within the existing categories. Tracking of 64bit prefmem sections separately from regular prefmem sections doesn't make sense, because both need to be allocated from the same pool when behind a bridge.

Quoted 7 lines

src/pciinit.c +0 -0

  struct pci_bus {

      struct {

src/pciinit.c +0 -3

-        /* pci region stats */
-        u32 count[32 - PCI_MEM_INDEX_SHIFT];
-        u32 sum, max;
          /* seconday bus region sizes */
          u32 size;

src/pciinit.c static const char *region_type_name[] = { · line 46

Quoted 10 lines

src/pciinit.c +7 -3

-        /* pci region assignments */
-        u32 bases[32 - PCI_MEM_INDEX_SHIFT];
-        u32 base;
+        /* pci region stats */
+        u32 max;
+        u32 count[32 - PCI_MEM_INDEX_SHIFT];
+        s64 sum;
+         /* pci region assignments */
+        s64 base;
+        s64 bases[32 - PCI_MEM_INDEX_SHIFT];

Why the choice of s64 over u64? Given the amount of bit manipulation on these values, I think using u64 would be safer.

src/pciinit.c static enum pci_region_type pci_addr_to_type(u32 addr) · line 76

Quoted 7 lines

src/pciinit.c +2 -0

@@ -69,6 +72,8 @@ static enum pci_region_type pci_addr_to_type(u32 addr)
  {
      if (addr&  PCI_BASE_ADDRESS_SPACE_IO)
          return PCI_REGION_TYPE_IO;
+    if (addr&  PCI_BASE_ADDRESS_MEM_TYPE_64)
+        return PCI_REGION_TYPE_PREFMEM_64;

This seems dangerous - a 64bit bar can be non-prefetchable - getting this wrong could cause random (hard to debug) crashes.

Quoted 11 lines

src/pciinit.c +1 -1

@@ -378,19 +383,16 @@ static void pci_bios_check_devices(struct
 pci_bus *busses)
          struct pci_bus *bus =&busses[pci_bdf_to_bus(pci->bdf)];
          int i;
          for (i = 0; i<  PCI_NUM_REGIONS; i++) {
-            u32 val, size;
+            u32 val, size, type;
              pci_bios_get_bar(pci, i,&val,&size);
              if (val == 0)
                  continue;
 

src/pciinit.c static void pci_bios_check_devices(struct pci_bus *busses) · line 396

Quoted 12 lines

src/pciinit.c +3 -6

-            pci_bios_bus_reserve(bus, pci_addr_to_type(val), size);
+            type = pci_addr_to_type(val);
+            pci_bios_bus_reserve(bus, type, size);
              pci->bars[i].addr = val;
              pci->bars[i].size = size;
-            pci->bars[i].is64 = (!(val&  PCI_BASE_ADDRESS_SPACE_IO)&&
-                                 (val&  PCI_BASE_ADDRESS_MEM_TYPE_MASK)
-                                 == PCI_BASE_ADDRESS_MEM_TYPE_64);
-
-            if (pci->bars[i].is64)
+            if (type == PCI_REGION_TYPE_PREFMEM_64)
                  i++;

If there is a 64bit bar, then the size could be over 32bits - the code really needs to handle this (or it could cause overlapping regions which result in random crashes).

src/pciinit.c static enum pci_region_type pci_addr_to_type(u32 addr) · line 76

Quoted 7 lines

src/pciinit.c +2 -0

@@ -69,6 +72,8 @@ static enum pci_region_type pci_addr_to_type(u32 addr)
  {
      if (addr&  PCI_BASE_ADDRESS_SPACE_IO)
          return PCI_REGION_TYPE_IO;
+    if (addr&  PCI_BASE_ADDRESS_MEM_TYPE_64)
+        return PCI_REGION_TYPE_PREFMEM_64;

This seems dangerous - a 64bit bar can be non-prefetchable - getting this wrong could cause random (hard to debug) crashes.

Quoted 4 lines · Alexey Korolev

All devices behind a bridge need to have all their regions consecutive and not overlapping with all the normal memory ranges.
Since prefetchable memory is described by one record, we must avoid the situations when 32bit and 64bit prefetchable regions are present within one secondary bus.

How do we avoid this? Assume we have two devices:
a 32 bit and a 64 bit one, behind a bridge.
There are two main things we can do:
1. Make the 64 bit device only use the low 32 bit

2. Put the 32 bit one in the non-prefetcheable range

1 probably makes more sense for small BARs
2 probably makes more sense for large ones

Try also looking at e.g. linux bus scanning code for more ideas.

Another thing I don't see addressed here is that support for 64 bit ranges is I think optional in the bridge.

Quoted 1 line

Signed-off-by Alexey Korolev<[email protected]>

Whitespace is corrupted: checkyour mail setup?
There should be spaces around operators:
a< b, I see a< b. Sometimes a< b (two spaces after).

Quoted 64 lines

---
  src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
  1 files changed, 48 insertions(+), 21 deletions(-)

src/pciinit.c +25 -1

index a574e38..92942d5 100644
@@ -17,6 +17,7 @@
 
  #define PCI_BRIDGE_IO_MIN      0x1000
  #define PCI_BRIDGE_MEM_MIN   0x100000
+#define PCI_BRIDGE_MEM_MAX   0x80000000
 
  enum pci_region_type {
      PCI_REGION_TYPE_IO,

@@ -45,6 +46,7 @@ struct pci_bus {
          s64 base;
          s64 bases[32 - PCI_MEM_INDEX_SHIFT];
      } r[PCI_REGION_TYPE_COUNT];
+    int is64;
      struct pci_device *bus_dev;
  };
 

@@ -369,6 +371,26 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
          bus->r[type].max = size;
  }
 
+static void pci_bios_secondary_bus_reserve(struct pci_bus *parent,
+                                           struct pci_bus *s, int type)
+{
+    u32 limit = (type == PCI_REGION_TYPE_IO) ?
+        PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
+
+    if (s->r[type].sum>   PCI_BRIDGE_MEM_MAX) {
+        panic("Size: %08x%08x is too big\n",
+                (u32)s->r[type].sum, (u32)(s->r[type].sum>>32));
+    }
+    s->r[type].size = (u32)s->r[type].sum;
+    if (s->r[type].size<   limit)
+        s->r[type].size = limit;
+    s->r[type].size = pci_size_roundup(s->r[type].size);
+
+    pci_bios_bus_reserve(parent, type, s->r[type].size);
+    dprintf(1, "    size: %x, type %s\n",
+                s->r[type].size, region_type_name[type]);
+}
+
  static void pci_bios_check_devices(struct pci_bus *busses)
  {
      dprintf(1, "PCI: check devices\n");

@@ -392,8 +414,10 @@ static void pci_bios_check_devices(struct pci_bus *busses)
              pci_bios_bus_reserve(bus, type, size);
              pci->bars[i].addr = val;
              pci->bars[i].size = size;
-            if (type == PCI_REGION_TYPE_PREFMEM_64)
+            if (type == PCI_REGION_TYPE_PREFMEM_64) {
+                bus->is64 = 1;
                  i++;
+            }
          }
      }
 

src/pciinit.c static void pci_bios_check_devices(struct pci_bus *busses) · line 432

Quoted 5 lines

src/pciinit.c +2 -0

@@ -404,22 +428,21 @@ static void pci_bios_check_devices(struct pci_bus *busses)
          if (!s->bus_dev)
              continue;
          struct pci_bus *parent =&busses[pci_bdf_to_bus(s->bus_dev->bdf)];
+
+        if (s->r[PCI_REGION_TYPE_PREFMEM_64].sum&&

Space before&& here and elsewhere.

Quoted 14 lines

src/pciinit.c +5 -7

+                   s->r[PCI_REGION_TYPE_PREFMEM].sum) {
+           panic("Sparse PCI prefmem regions on the bus %d\n", secondary_bus);
+        }
+
+        dprintf(1, "PCI: secondary bus %d\n", secondary_bus);
          int type;
          for (type = 0; type<   PCI_REGION_TYPE_COUNT; type++) {
-            u32 limit = (type == PCI_REGION_TYPE_IO) ?
-                PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
-            s->r[type].size = s->r[type].sum;
-            if (s->r[type].size<   limit)
-                s->r[type].size = limit;
-            s->r[type].size = pci_size_roundup(s->r[type].size);
-            pci_bios_bus_reserve(parent, type, s->r[type].size);

src/pciinit.c static void pci_bios_check_devices(struct pci_bus *busses) · line 442

Quoted 10 lines

src/pciinit.c +3 -7

-        }
-        dprintf(1, "PCI: secondary bus %d sizes: io %x, mem %x, prefmem %x\n",
-                secondary_bus,
-                s->r[PCI_REGION_TYPE_IO].size,
-                s->r[PCI_REGION_TYPE_MEM].size,
-                s->r[PCI_REGION_TYPE_PREFMEM].size);
-    }
+            if ((type == PCI_REGION_TYPE_PREFMEM_64&&   !s->is64) ||
+                (type == PCI_REGION_TYPE_PREFMEM&&   s->is64))
+                continue;

Can't figure this out. What does this do?

Quoted 4 lines · Alexey Korolev

All devices behind a bridge need to have all their regions consecutive and not overlapping with all the normal memory ranges.
Since prefetchable memory is described by one record, we must avoid the situations when 32bit and 64bit prefetchable regions are present within one secondary bus.

How do we avoid this? Assume we have two devices:
a 32 bit and a 64 bit one, behind a bridge.
There are two main things we can do:
1. Make the 64 bit device only use the low 32 bit

2. Put the 32 bit one in the non-prefetcheable range

1 probably makes more sense for small BARs
2 probably makes more sense for large ones

Try also looking at e.g. linux bus scanning code for more ideas.

Another thing I don't see addressed here is that support for 64 bit ranges is I think optional in the bridge.

Quoted 1 line

Signed-off-by Alexey Korolev<[email protected]>

Whitespace is corrupted: checkyour mail setup?
There should be spaces around operators:
a< b, I see a< b. Sometimes a< b (two spaces after).

Quoted 64 lines

---
  src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
  1 files changed, 48 insertions(+), 21 deletions(-)

src/pciinit.c +25 -1

index a574e38..92942d5 100644
@@ -17,6 +17,7 @@
 
  #define PCI_BRIDGE_IO_MIN      0x1000
  #define PCI_BRIDGE_MEM_MIN   0x100000
+#define PCI_BRIDGE_MEM_MAX   0x80000000
 
  enum pci_region_type {
      PCI_REGION_TYPE_IO,

@@ -45,6 +46,7 @@ struct pci_bus {
          s64 base;
          s64 bases[32 - PCI_MEM_INDEX_SHIFT];
      } r[PCI_REGION_TYPE_COUNT];
+    int is64;
      struct pci_device *bus_dev;
  };
 

@@ -369,6 +371,26 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
          bus->r[type].max = size;
  }
 
+static void pci_bios_secondary_bus_reserve(struct pci_bus *parent,
+                                           struct pci_bus *s, int type)
+{
+    u32 limit = (type == PCI_REGION_TYPE_IO) ?
+        PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
+
+    if (s->r[type].sum>   PCI_BRIDGE_MEM_MAX) {
+        panic("Size: %08x%08x is too big\n",
+                (u32)s->r[type].sum, (u32)(s->r[type].sum>>32));
+    }
+    s->r[type].size = (u32)s->r[type].sum;
+    if (s->r[type].size<   limit)
+        s->r[type].size = limit;
+    s->r[type].size = pci_size_roundup(s->r[type].size);
+
+    pci_bios_bus_reserve(parent, type, s->r[type].size);
+    dprintf(1, "    size: %x, type %s\n",
+                s->r[type].size, region_type_name[type]);
+}
+
  static void pci_bios_check_devices(struct pci_bus *busses)
  {
      dprintf(1, "PCI: check devices\n");

@@ -392,8 +414,10 @@ static void pci_bios_check_devices(struct pci_bus *busses)
              pci_bios_bus_reserve(bus, type, size);
              pci->bars[i].addr = val;
              pci->bars[i].size = size;
-            if (type == PCI_REGION_TYPE_PREFMEM_64)
+            if (type == PCI_REGION_TYPE_PREFMEM_64) {
+                bus->is64 = 1;
                  i++;
+            }
          }
      }
 

src/pciinit.c static void pci_bios_check_devices(struct pci_bus *busses) · line 432

Quoted 5 lines

src/pciinit.c +2 -0

@@ -404,22 +428,21 @@ static void pci_bios_check_devices(struct pci_bus *busses)
          if (!s->bus_dev)
              continue;
          struct pci_bus *parent =&busses[pci_bdf_to_bus(s->bus_dev->bdf)];
+
+        if (s->r[PCI_REGION_TYPE_PREFMEM_64].sum&&

Space before&& here and elsewhere.

Quoted 14 lines

src/pciinit.c +5 -7

+                   s->r[PCI_REGION_TYPE_PREFMEM].sum) {
+           panic("Sparse PCI prefmem regions on the bus %d\n", secondary_bus);
+        }
+
+        dprintf(1, "PCI: secondary bus %d\n", secondary_bus);
          int type;
          for (type = 0; type<   PCI_REGION_TYPE_COUNT; type++) {
-            u32 limit = (type == PCI_REGION_TYPE_IO) ?
-                PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
-            s->r[type].size = s->r[type].sum;
-            if (s->r[type].size<   limit)
-                s->r[type].size = limit;
-            s->r[type].size = pci_size_roundup(s->r[type].size);
-            pci_bios_bus_reserve(parent, type, s->r[type].size);

src/pciinit.c static void pci_bios_check_devices(struct pci_bus *busses) · line 442

Quoted 10 lines

src/pciinit.c +3 -7

-        }
-        dprintf(1, "PCI: secondary bus %d sizes: io %x, mem %x, prefmem %x\n",
-                secondary_bus,
-                s->r[PCI_REGION_TYPE_IO].size,
-                s->r[PCI_REGION_TYPE_MEM].size,
-                s->r[PCI_REGION_TYPE_PREFMEM].size);
-    }
+            if ((type == PCI_REGION_TYPE_PREFMEM_64&&   !s->is64) ||
+                (type == PCI_REGION_TYPE_PREFMEM&&   s->is64))
+                continue;

Can't figure this out. What does this do?

Quoted 1 line

Patches have been tested on several configurations which includes

Quoted 2 lines

linux 2.6.18 - 3.0 &
windows 2008. Everything works quite well.

Which qemu version did you use?

Quoted 9 lines · Michael S. Tsirkin

Quoted 4 lines · Alexey Korolev

All devices behind a bridge need to have all their regions consecutive and not overlapping with all the normal memory ranges.
Since prefetchable memory is described by one record, we must avoid the situations when 32bit and 64bit prefetchable regions are present within one secondary bus.

How do we avoid this? Assume we have two devices:
a 32 bit and a 64 bit one, behind a bridge.
There are two main things we can do:
1. Make the 64 bit device only use the low 32 bit

It was my first implementation. Unfortunately older versions of
Linux (Like 2.6.18) hang during startup with this.
As far as I remember it was qemu-0.15 so may be 1.0 have no such an issue. I will check this.

Quoted 1 line

2. Put the 32 bit one in the non-prefetcheable range

I'd rather not do this. Bios should not change memory region types. It will confuse guest OS drivers.

Quoted 6 lines

1 probably makes more sense for small BARs
2 probably makes more sense for large ones

Try also looking at e.g. linux bus scanning code for more ideas.

Quoted 7 lines

Another thing I don't see addressed here is that support for 64 bit ranges is I think optional in the bridge.

Quoted 1 line

Signed-off-by Alexey Korolev<[email protected]>

Whitespace is corrupted: checkyour mail setup?
There should be spaces around operators:
a< b, I see a< b. Sometimes a< b (two spaces after).

Yes, it's thunderbird :(. Sorry about that.
It seems the patches need to have some functional changes.

Quoted 95 lines

Quoted 64 lines

---
 src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 48 insertions(+), 21 deletions(-)

src/pciinit.c +25 -1

index a574e38..92942d5 100644
@@ -17,6 +17,7 @@
 
 #define PCI_BRIDGE_IO_MIN      0x1000
 #define PCI_BRIDGE_MEM_MIN   0x100000
+#define PCI_BRIDGE_MEM_MAX   0x80000000
 
 enum pci_region_type {
     PCI_REGION_TYPE_IO,

@@ -45,6 +46,7 @@ struct pci_bus {
         s64 base;
         s64 bases[32 - PCI_MEM_INDEX_SHIFT];
     } r[PCI_REGION_TYPE_COUNT];
+    int is64;
     struct pci_device *bus_dev;
 };
 

@@ -369,6 +371,26 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
         bus->r[type].max = size;
 }
 
+static void pci_bios_secondary_bus_reserve(struct pci_bus *parent,
+                                           struct pci_bus *s, int type)
+{
+    u32 limit = (type == PCI_REGION_TYPE_IO) ?
+        PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
+
+    if (s->r[type].sum>   PCI_BRIDGE_MEM_MAX) {
+        panic("Size: %08x%08x is too big\n",
+                (u32)s->r[type].sum, (u32)(s->r[type].sum>>32));
+    }
+    s->r[type].size = (u32)s->r[type].sum;
+    if (s->r[type].size<   limit)
+        s->r[type].size = limit;
+    s->r[type].size = pci_size_roundup(s->r[type].size);
+
+    pci_bios_bus_reserve(parent, type, s->r[type].size);
+    dprintf(1, "    size: %x, type %s\n",
+                s->r[type].size, region_type_name[type]);
+}
+
 static void pci_bios_check_devices(struct pci_bus *busses)
 {
     dprintf(1, "PCI: check devices\n");

@@ -392,8 +414,10 @@ static void pci_bios_check_devices(struct pci_bus *busses)
             pci_bios_bus_reserve(bus, type, size);
             pci->bars[i].addr = val;
             pci->bars[i].size = size;
-            if (type == PCI_REGION_TYPE_PREFMEM_64)
+            if (type == PCI_REGION_TYPE_PREFMEM_64) {
+                bus->is64 = 1;
                 i++;
+            }
         }
     }
 

src/pciinit.c static void pci_bios_check_devices(struct pci_bus *busses) · line 432

Quoted 5 lines

src/pciinit.c +2 -0

@@ -404,22 +428,21 @@ static void pci_bios_check_devices(struct pci_bus *busses)
         if (!s->bus_dev)
             continue;
         struct pci_bus *parent =&busses[pci_bdf_to_bus(s->bus_dev->bdf)];
+
+        if (s->r[PCI_REGION_TYPE_PREFMEM_64].sum&&

Space before&& here and elsewhere.

Quoted 14 lines

src/pciinit.c +5 -7

+                   s->r[PCI_REGION_TYPE_PREFMEM].sum) {
+           panic("Sparse PCI prefmem regions on the bus %d\n", secondary_bus);
+        }
+
+        dprintf(1, "PCI: secondary bus %d\n", secondary_bus);
         int type;
         for (type = 0; type<   PCI_REGION_TYPE_COUNT; type++) {
-            u32 limit = (type == PCI_REGION_TYPE_IO) ?
-                PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
-            s->r[type].size = s->r[type].sum;
-            if (s->r[type].size<   limit)
-                s->r[type].size = limit;
-            s->r[type].size = pci_size_roundup(s->r[type].size);
-            pci_bios_bus_reserve(parent, type, s->r[type].size);

src/pciinit.c static void pci_bios_check_devices(struct pci_bus *busses) · line 442

Quoted 10 lines

src/pciinit.c +3 -7

-        }
-        dprintf(1, "PCI: secondary bus %d sizes: io %x, mem %x, prefmem %x\n",
-                secondary_bus,
-                s->r[PCI_REGION_TYPE_IO].size,
-                s->r[PCI_REGION_TYPE_MEM].size,
-                s->r[PCI_REGION_TYPE_PREFMEM].size);
-    }
+            if ((type == PCI_REGION_TYPE_PREFMEM_64&&   !s->is64) ||
+                (type == PCI_REGION_TYPE_PREFMEM&&   s->is64))
+                continue;

Quoted 1 line

Can't figure this out. What does this do?

Bios will panic if it founds prefmem BARs in both 32bit and 64bit areas. Otherwise we will pick one which exists or 32bit one if both are not present.
Anyway this part is very likely to be rewritten.

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Alexey Korolev

All devices behind a bridge need to have all their regions consecutive and not overlapping with all the normal memory ranges.
Since prefetchable memory is described by one record, we must avoid the situations when 32bit and 64bit prefetchable regions are present within one secondary bus.

Quoted 4 lines

How do we avoid this? Assume we have two devices:
a 32 bit and a 64 bit one, behind a bridge.
There are two main things we can do:
1. Make the 64 bit device only use the low 32 bit

It was my first implementation. Unfortunately older versions of
Linux (Like 2.6.18) hang during startup with this.

As far as I remember it was qemu-0.15 so may be 1.0 have no such an issue. I will check this.

Quoted 1 line

2. Put the 32 bit one in the non-prefetcheable range

I'd rather not do this. Bios should not change memory region types. It will confuse guest OS drivers.

Quoted 6 lines

1 probably makes more sense for small BARs
2 probably makes more sense for large ones

Try also looking at e.g. linux bus scanning code for more ideas.

Quoted 7 lines

Another thing I don't see addressed here is that support for 64 bit ranges is I think optional in the bridge.

Quoted 1 line

Signed-off-by Alexey Korolev<[email protected]>

Whitespace is corrupted: checkyour mail setup?
There should be spaces around operators:
a< b, I see a< b. Sometimes a< b (two spaces after).

Yes, it's thunderbird :(. Sorry about that.
It seems the patches need to have some functional changes anyway.

Quoted 95 lines

Quoted 64 lines

---
 src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 48 insertions(+), 21 deletions(-)

src/pciinit.c +25 -1

index a574e38..92942d5 100644
@@ -17,6 +17,7 @@
 
 #define PCI_BRIDGE_IO_MIN      0x1000
 #define PCI_BRIDGE_MEM_MIN   0x100000
+#define PCI_BRIDGE_MEM_MAX   0x80000000
 
 enum pci_region_type {
     PCI_REGION_TYPE_IO,

@@ -45,6 +46,7 @@ struct pci_bus {
         s64 base;
         s64 bases[32 - PCI_MEM_INDEX_SHIFT];
     } r[PCI_REGION_TYPE_COUNT];
+    int is64;
     struct pci_device *bus_dev;
 };
 

@@ -369,6 +371,26 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
         bus->r[type].max = size;
 }
 
+static void pci_bios_secondary_bus_reserve(struct pci_bus *parent,
+                                           struct pci_bus *s, int type)
+{
+    u32 limit = (type == PCI_REGION_TYPE_IO) ?
+        PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
+
+    if (s->r[type].sum>   PCI_BRIDGE_MEM_MAX) {
+        panic("Size: %08x%08x is too big\n",
+                (u32)s->r[type].sum, (u32)(s->r[type].sum>>32));
+    }
+    s->r[type].size = (u32)s->r[type].sum;
+    if (s->r[type].size<   limit)
+        s->r[type].size = limit;
+    s->r[type].size = pci_size_roundup(s->r[type].size);
+
+    pci_bios_bus_reserve(parent, type, s->r[type].size);
+    dprintf(1, "    size: %x, type %s\n",
+                s->r[type].size, region_type_name[type]);
+}
+
 static void pci_bios_check_devices(struct pci_bus *busses)
 {
     dprintf(1, "PCI: check devices\n");

@@ -392,8 +414,10 @@ static void pci_bios_check_devices(struct pci_bus *busses)
             pci_bios_bus_reserve(bus, type, size);
             pci->bars[i].addr = val;
             pci->bars[i].size = size;
-            if (type == PCI_REGION_TYPE_PREFMEM_64)
+            if (type == PCI_REGION_TYPE_PREFMEM_64) {
+                bus->is64 = 1;
                 i++;
+            }
         }
     }
 

src/pciinit.c static void pci_bios_check_devices(struct pci_bus *busses) · line 432

Quoted 5 lines

src/pciinit.c +2 -0

@@ -404,22 +428,21 @@ static void pci_bios_check_devices(struct pci_bus *busses)
         if (!s->bus_dev)
             continue;
         struct pci_bus *parent =&busses[pci_bdf_to_bus(s->bus_dev->bdf)];
+
+        if (s->r[PCI_REGION_TYPE_PREFMEM_64].sum&&

Space before&& here and elsewhere.

Quoted 14 lines

src/pciinit.c +5 -7

+                   s->r[PCI_REGION_TYPE_PREFMEM].sum) {
+           panic("Sparse PCI prefmem regions on the bus %d\n", secondary_bus);
+        }
+
+        dprintf(1, "PCI: secondary bus %d\n", secondary_bus);
         int type;
         for (type = 0; type<   PCI_REGION_TYPE_COUNT; type++) {
-            u32 limit = (type == PCI_REGION_TYPE_IO) ?
-                PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
-            s->r[type].size = s->r[type].sum;
-            if (s->r[type].size<   limit)
-                s->r[type].size = limit;
-            s->r[type].size = pci_size_roundup(s->r[type].size);
-            pci_bios_bus_reserve(parent, type, s->r[type].size);

src/pciinit.c static void pci_bios_check_devices(struct pci_bus *busses) · line 442

Quoted 10 lines

src/pciinit.c +3 -7

-        }
-        dprintf(1, "PCI: secondary bus %d sizes: io %x, mem %x, prefmem %x\n",
-                secondary_bus,
-                s->r[PCI_REGION_TYPE_IO].size,
-                s->r[PCI_REGION_TYPE_MEM].size,
-                s->r[PCI_REGION_TYPE_PREFMEM].size);
-    }
+            if ((type == PCI_REGION_TYPE_PREFMEM_64&&   !s->is64) ||
+                (type == PCI_REGION_TYPE_PREFMEM&&   s->is64))
+                continue;

Quoted 1 line

Can't figure this out. What does this do?

Bios will panic if it founds prefmem BARs in both 32bit and 64bit areas. Otherwise we will pick one which exists or 32bit one if both are not present.

Thanks
Alexey

Quoted 10 lines

src/pciinit.c static enum pci_region_type pci_addr_to_type(u32 addr) · line 76

Quoted 7 lines

src/pciinit.c +2 -0

@@ -69,6 +72,8 @@ static enum pci_region_type pci_addr_to_type(u32 addr)
 {
     if (addr&  PCI_BASE_ADDRESS_SPACE_IO)
         return PCI_REGION_TYPE_IO;
+    if (addr&  PCI_BASE_ADDRESS_MEM_TYPE_64)
+        return PCI_REGION_TYPE_PREFMEM_64;

This seems dangerous - a 64bit bar can be non-prefetchable - getting this wrong could cause random (hard to debug) crashes.

Just out of curiosity - how this could happen? Having 64bit non-prefetchable BAR implies that the device is not behind any bridge (as bridges describe 64bit ranges for prefetchable memory only). Is it possible on nowadays systems?

Quoted 1 line

Can't figure this out. What does this do?

Bios will panic if it founds prefmem BARs in both 32bit and 64bit areas.

Otherwise we will pick one which exists or 32bit one if both are not present.

Quoted 6 lines

Quoted 1 line

Patches have been tested on several configurations which includes

Quoted 2 lines

linux 2.6.18 - 3.0 &
windows 2008. Everything works quite well.

Which qemu version did you use?

I tried both 0.15 and 1.0

Quoted 5 lines · Alexey Korolev

Quoted 4 lines

Patches have been tested on several configurations which includes

Quoted 2 lines

linux 2.6.18 - 3.0&
windows 2008. Everything works quite well.

Which qemu version did you use?

I tried both 0.15 and 1.0

qemu or qemu-kcm?

Quoted 14 lines · Alexey Korolev

Quoted 9 lines · Michael S. Tsirkin

Quoted 4 lines · Alexey Korolev

All devices behind a bridge need to have all their regions consecutive and not overlapping with all the normal memory ranges.
Since prefetchable memory is described by one record, we must avoid the situations when 32bit and 64bit prefetchable regions are present within one secondary bus.

How do we avoid this? Assume we have two devices:
a 32 bit and a 64 bit one, behind a bridge.
There are two main things we can do:
1. Make the 64 bit device only use the low 32 bit

It was my first implementation. Unfortunately older versions of
Linux (Like 2.6.18) hang during startup with this.
As far as I remember it was qemu-0.15 so may be 1.0 have no such an issue. I will check this.

Quoted 3 lines

Quoted 1 line

2. Put the 32 bit one in the non-prefetcheable range

I'd rather not do this. Bios should not change memory region types. It will confuse guest OS drivers.

It shouldn't, it's a legal behavious. Prefetcheable BAR allows certain optimizations but does not require them.

Quoted 4 lines · Alexey Korolev

All devices behind a bridge need to have all their regions consecutive and not overlapping with all the normal memory ranges.
Since prefetchable memory is described by one record, we must avoid the situations when 32bit and 64bit prefetchable regions are present within one secondary bus.

How do we avoid this? Assume we have two devices:
a 32 bit and a 64 bit one, behind a bridge. There are two main things we can do:
1. Make the 64 bit device only use the low 32 bit
2. Put the 32 bit one in the non-prefetcheable range

Quoted 2 lines · Alexey Korolev

Quoted 1 line

Can't figure this out. What does this do?

Bios will panic if it founds prefmem BARs in both 32bit and 64bit areas.

That's not good, it's a legal configuration.

Quoted 2 lines · Alexey Korolev

Quoted 1 line

Can't figure this out. What does this do?

Bios will panic if it founds prefmem BARs in both 32bit and 64bit areas.

That's not good, it's a legal configuration.

Quoted 15 lines · Kevin O'Connor

src/pciinit.c enum pci_region_type { · line 27

Quoted 9 lines · Alexey Korolev

src/pciinit.c +1 -0

@@ -22,6 +22,7 @@ enum pci_region_type {
     PCI_REGION_TYPE_IO,
     PCI_REGION_TYPE_MEM,
     PCI_REGION_TYPE_PREFMEM,
+    PCI_REGION_TYPE_PREFMEM_64,
     PCI_REGION_TYPE_COUNT,
 };

This doesn't seem right. A 64bit bar is not a new category - it's just a way of representing larger values within the existing categories. Tracking of 64bit prefmem sections separately from regular prefmem sections doesn't make sense, because both need to be allocated from the same pool when behind a bridge.

It's a way to account all memory sections on the root bus, as on the root bus we can have all 4 regions.
I don't like this part as well, it causes confusions.

It makes sense to figure out how shall we account all memory sections on the root bus.
Will it be separated structures for root bus and secondary buses? Do you have another idea?

Quoted 3 lines · Michael S. Tsirkin

Quoted 2 lines · Alexey Korolev

Quoted 1 line

Can't figure this out. What does this do?

Bios will panic if it founds prefmem BARs in both 32bit and 64bit areas.

Quoted 2 lines

That's not good, it's a legal configuration.

To be more complete : Bios will panic if it founds prefmem BARs in both 32bit and 64bit areas
on the same secondary Bus.
I agree about that this limitation is too strict at the moment. But this is just a limitation, there are
plenty of them in Seabios.

To answer if it is a legal or not we need to have a look how real BIOS would behave in a such
scenario. Unfortunately I have no chance to have this configuration on hand. If you have (may
have) this I would appreciate if you let us know how the Bios will behave?

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Alexey Korolev

All devices behind a bridge need to have all their regions consecutive and not overlapping with all the normal memory ranges.
Since prefetchable memory is described by one record, we must avoid the situations when 32bit and 64bit prefetchable regions are present within one secondary bus.

Quoted 4 lines

How do we avoid this? Assume we have two devices:
a 32 bit and a 64 bit one, behind a bridge.
There are two main things we can do:
1. Make the 64 bit device only use the low 32 bit

It was my first implementation. Unfortunately older versions of
Linux (Like 2.6.18) hang during startup with this.
As far as I remember it was qemu-0.15 so may be 1.0 have no such an issue. I will check this.

Quoted 7 lines

Quoted 3 lines

There are two main things we can do:
1. Make the 64 bit device only use the low 32 bit

It was my first implementation. Unfortunately older versions of
Linux (Like 2.6.18) hang during startup with this.
As far as I remember it was qemu-0.15 so may be 1.0 have no such an issue. I will check this.

That seems really odd - there's nothing unusual with a 64bit bar being set to an address under 4gig.

Looking at the current code, it's not initializing 64bit bars properly (it doesn't initialize the top bits of the 64bit address). Does just the patch below (totally untested) improve things for you?

Quoted 12 lines · Alexey Korolev

This patch adds PCI_REGION_TYPE_PREFMEM_64 region type and modifies types of variables to make it possible to work with 64 bit addresses.

Why I've added just one region type PCI_REGION_TYPE_PREFMEM_64 and haven't added PCI_REGION_TYPE_MEM_64? According to PCI architecture specification, the
bridges can describe 64bit ranges for prefetchable type of memory only. So it's very
unlikely that devices exporting 64bit non-prefetchable BARs. Anyway this code will work
with 64bit non-prefetchable BARs unless the PCI device is not behind the secondary bus.

[...]

src/pciinit.c enum pci_region_type { · line 27

Quoted 9 lines

src/pciinit.c +1 -0

@@ -22,6 +22,7 @@ enum pci_region_type {
     PCI_REGION_TYPE_IO,
     PCI_REGION_TYPE_MEM,
     PCI_REGION_TYPE_PREFMEM,
+    PCI_REGION_TYPE_PREFMEM_64,
     PCI_REGION_TYPE_COUNT,
 };

This doesn't seem right. A 64bit bar is not a new category - it's just a way of representing larger values within the existing categories.

Tracking of 64bit prefmem sections separately from regular prefmem sections doesn't make sense, because both need to be allocated from the same pool when behind a bridge.

Quoted 3 lines · Kevin O'Connor

Tracking of 64bit prefmem sections separately from regular prefmem sections doesn't make sense, because both need to be allocated from the same pool when behind a bridge.

Yea, bridges make the picture a bit more complicated. A bridge itself might not be 64bit capable, which makes it impossible to map devices behind it above 4G. Likewise with multiple devices where one of them has a 32bit prefmem bar: The (single) prefmem memory window must be below 4G then too.

I think we'll have to keep track for each bar whenever we'll go map it above or below 4G. For starters this logic can be simply to only map 64bit bars of root bus devices above 4G, which completely sidesteps the bridge issues for now.

Once qemu has full pci-e support we probably want refine this logic, which probably requires an extra pass between pci bar discovery and placing the bars into the memory regions we have.