[Qemu-devel] [Seabios] [PATCH 0/3] 64bit PCI BARs allocations

[Qemu-devel] [Seabios] [PATCH 0/3] 64bit PCI BARs allocations

[Alexey Korolev]

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.

[Qemu-devel] [PATCH 1/3] Add new range above 4GB in _CRS table

[Alexey Korolev]

Since Windows is using CRS table for PCI resource allocation, this patch 
allows Windows
guests to work with PCI devices when PCI BAR allocation is above 4GB.
This also might be helpful on Linux when use_crs kernel boot option is set.

Michael I've added you to signed-off if you don't mind.

Signed-off-by: Alexey Korolev <alexey.korolev@endace.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
  src/acpi-dsdt.dsl |    7 +++++
  src/acpi-dsdt.hex |   66 
+++++++++++++++++++++++++++++++++++++++++++++--------
  src/config.h      |    2 +
  3 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 7082b65..c17e947 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -175,6 +175,13 @@ DefinitionBlock (
                      0x00000000,         // Address Translation Offset
                      0x1EC00000,         // Address Length
                      ,, , AddressRangeMemory, TypeStatic)
+                QWordMemory (ResourceProducer, PosDecode, MinFixed, 
MaxFixed, Cacheable, ReadWrite,
+                    0x00000000,          // Address Space Granularity
+                    0x8000000000,        // Address Range Minimum
+                    0xFFFFFFFFFF,        // Address Range Maximum
+                    0x00000000,          // Address Translation Offset
+                    0x8000000000,        // Address Length
+                    ,, , AddressRangeMemory, TypeStatic)
              })
          }
      }
diff --git a/src/acpi-dsdt.hex b/src/acpi-dsdt.hex
index 5dc7bb4..fe4c17e 100644
--- a/src/acpi-dsdt.hex
+++ b/src/acpi-dsdt.hex
@@ -3,12 +3,12 @@ static unsigned char AmlCode[] = {
  0x53,
  0x44,
  0x54,
-0xd3,
-0x10,
+0x1,
+0x11,
  0x0,
  0x0,
  0x1,
-0x2d,
+0xe,
  0x42,
  0x58,
  0x50,
@@ -110,16 +110,16 @@ static unsigned char AmlCode[] = {
  0x47,
  0x42,
  0x10,
-0x44,
-0x81,
+0x42,
+0x84,
  0x5f,
  0x53,
  0x42,
  0x5f,
  0x5b,
  0x82,
-0x4c,
-0x80,
+0x4a,
+0x83,
  0x50,
  0x43,
  0x49,
@@ -2064,10 +2064,10 @@ static unsigned char AmlCode[] = {
  0x52,
  0x53,
  0x11,
-0x42,
-0x7,
+0x40,
+0xa,
  0xa,
-0x6e,
+0x9c,
  0x88,
  0xd,
  0x0,
@@ -2176,6 +2176,52 @@ static unsigned char AmlCode[] = {
  0x0,
  0xc0,
  0x1e,
+0x8a,
+0x2b,
+0x0,
+0x0,
+0xc,
+0x3,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x80,
+0x0,
+0x0,
+0x0,
+0xff,
+0xff,
+0xff,
+0xff,
+0xff,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x0,
+0x80,
+0x0,
+0x0,
+0x0,
  0x79,
  0x0,
  0x10,
diff --git a/src/config.h b/src/config.h
index b0187a4..236f2e9 100644
--- a/src/config.h
+++ b/src/config.h
@@ -47,6 +47,8 @@

  #define BUILD_PCIMEM_START        0xe0000000
  #define BUILD_PCIMEM_END          0xfec00000    /* IOAPIC is mapped at */
+#define BUILD_PCIMEM_64_START     0x8000000000LL
+#define BUILD_PCIMEM_64_END       0x10000000000LL

  #define BUILD_IOAPIC_ADDR         0xfec00000
  #define BUILD_HPET_ADDRESS        0xfed00000
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[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.

Signed-off-by: Alexey Korolev <alexey.korolev@endace.com>
---
  src/pci.h     |    1 -
  src/pciinit.c |   59 
+++++++++++++++++++++++++++++++++-----------------------
  2 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/src/pci.h b/src/pci.h
index a2a5a4c..71f15fe 100644
--- a/src/pci.h
+++ b/src/pci.h
@@ -54,7 +54,6 @@ struct pci_device {
      struct {
          u32 addr;
          u32 size;
-        int is64;
      } bars[PCI_NUM_REGIONS];

      // Local information on device.
diff --git a/src/pciinit.c b/src/pciinit.c
index 7d83368..a574e38 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -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++;
          }
      }
@@ -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) {
@@ -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];
@@ -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);
+            }
          }
      }
  }
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Michael S. Tsirkin]

On Wed, Dec 28, 2011 at 06:26:05PM +1300, Alexey Korolev wrote:
> 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.

> 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?

> 
> Signed-off-by: Alexey Korolev <alexey.korolev@endace.com>
> ---
>  src/pci.h     |    1 -
>  src/pciinit.c |   59
> +++++++++++++++++++++++++++++++++-----------------------
>  2 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/src/pci.h b/src/pci.h
> index a2a5a4c..71f15fe 100644
> --- a/src/pci.h
> +++ b/src/pci.h
> @@ -54,7 +54,6 @@ struct pci_device {
>      struct {
>          u32 addr;
>          u32 size;
> -        int is64;
>      } bars[PCI_NUM_REGIONS];
> 
>      // Local information on device.
> diff --git a/src/pciinit.c b/src/pciinit.c
> index 7d83368..a574e38 100644
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -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++;
>          }
>      }
> @@ -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) {
> @@ -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];
> @@ -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);
> +            }
>          }
>      }
>  }
> -- 
> 1.7.5.4

Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Alexey Korolev]

On 29/12/11 00:30, Michael S. Tsirkin wrote:
> On Wed, Dec 28, 2011 at 06:26:05PM +1300, Alexey Korolev wrote:
>> 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.
>
>> 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?
If a device is on the root bus and have BAR flags 0x4 (TYPE_MEMORY and 
64 bit),
memory will be allocated in 64bit range all flags remain the same. I did 
this just out
of curiosity and this appears to work well.
>
>

Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Kevin O'Connor]

On Wed, Dec 28, 2011 at 06:26:05PM +1300, Alexey Korolev wrote:
> 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.
[...]
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -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.

>  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];

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

> @@ -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.

> @@ -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++;

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).

-Kevin

Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Alexey Korolev]

On 29/12/11 15:56, Kevin O'Connor wrote:
> On Wed, Dec 28, 2011 at 06:26:05PM +1300, Alexey Korolev wrote:
>> 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.
> [...]
>> --- a/src/pciinit.c
>> +++ b/src/pciinit.c
>> @@ -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.

One possible solution is to have different descriptors for secondary buses
and the root bus.  In that case we can have 3 sections per secondary bus
and the root bus will contain memory regions without any 'physical' meaning.

>>   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];
> Why the choice of s64 over u64?  Given the amount of bit manipulation
> on these values, I think using u64 would be safer.
No problem.
Addresses could not exceed 40bit's so we basically may touch the last bit
only when negative value is stored.
>
>> @@ -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.
It is bit confusing but this doesn't touch actual types. So even if
we have a 64bit not-prefetchable BAR code will be working.
>> @@ -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++;
> 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).
Agree, something has hold in my mind that BAR size it is limited to 4GB,
but just looked into PCI spec - there are no limitations stated.
Well this requires bigger changes, as 64bit BAR size accounting touches
more computations comparing to 64bit BAR addresses.

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?

Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Kevin O'Connor]

On Thu, Dec 29, 2011 at 06:00:04PM +1300, Alexey Korolev wrote:
> On 29/12/11 15:56, Kevin O'Connor wrote:
> >On Wed, Dec 28, 2011 at 06:26:05PM +1300, Alexey Korolev wrote:
> >>--- a/src/pciinit.c
> >>+++ b/src/pciinit.c
> >>@@ -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.

The root bus can have five regions - 64bit non-prefmem is also
possible.

Given that the allocation for devices on the root bus is distinct from
devices behind bridges, I think one could separate out the root bus
assignment pass into its own code.  Indeed, much of the root bus
allocation is already separate.

> 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?

As an idea - keep the current three buckets (io, mem, prefmem) in
struct pci_bus, but expand them to track 64bit sizes.  Add a new flag
to each bucket to track if every device on the given bus is 64bit
capable for that resource.  When assigning the resources for a bridge,
only use 64bit prefmem if all prefmem bars behind that bridge are
64bit capable.

-Kevin

Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Alexey Korolev]

>> @@ -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?

Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Michael S. Tsirkin]

On Thu, Dec 29, 2011 at 06:32:37PM +1300, Alexey Korolev wrote:
> 
> >>@@ -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?

Yes. qemu puts all devices directly on the root bus.
system devices are also commonly located on the root bus.

Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Gerd Hoffmann]

On 12/29/11 03:56, Kevin O'Connor wrote:
> On Wed, Dec 28, 2011 at 06:26:05PM +1300, Alexey Korolev wrote:
>> 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.
> [...]
>> --- a/src/pciinit.c
>> +++ b/src/pciinit.c
>> @@ -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.

We need to take care to not confuse things here.  We have pci bars which
can be 64bit capable.  We (will) have a memory region (aka address
space) above 4G.  We may or may not map 64bit bars there.

> 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.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Kevin O'Connor]

On Tue, Jan 03, 2012 at 04:14:58PM +0100, Gerd Hoffmann wrote:
> On 12/29/11 03:56, Kevin O'Connor wrote:
> > 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.

I was looking at this last week, along with the coreboot PCI code.  I
think this could be done in four "passes" - first pass to find the
total size, minimum alignment, and maximum address of all resources
for all regular devices.  The second pass propagates this info to
bridges.  The third pass allocates the resources.  The fourth pass
assigns the resources to the devices.

The current code already effectively has these four "passes" - the
thing I think is missing is tracking the "maximum address" of
resources, propagating that to bridges, and assigning allocations
above 4G.

-Kevin

[Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[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.

Signed-off-by: Alexey Korolev<alexey.korolev@endace.com>
---
  src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
  1 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index a574e38..92942d5 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -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++;
+            }
          }
      }

@@ -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&&
+                   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);
-        }
-        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;
+            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.
-- 
1.7.5.4

Re: [Qemu-devel] [SeaBIOS] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Alexey Korolev]

I hate thunderbird. Will resend the patches tomorrow.

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

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Michael S. Tsirkin]

On Wed, Dec 28, 2011 at 06:35:55PM +1300, Alexey Korolev wrote:
> 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.

> 
> Signed-off-by: Alexey Korolev<alexey.korolev@endace.com>

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

> ---
>  src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/src/pciinit.c b/src/pciinit.c
> index a574e38..92942d5 100644
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -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++;
> +            }
>          }
>      }
> 
> @@ -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.

> +                   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);
> -        }
> -        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?

> +            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.
> -- 
> 1.7.5.4
> 

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Alexey Korolev]

On 29/12/11 00:43, Michael S. Tsirkin wrote:
> On Wed, Dec 28, 2011 at 06:35:55PM +1300, Alexey Korolev wrote:
>> 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.
> 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.
>
> 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.
>
>> Signed-off-by: Alexey Korolev<alexey.korolev@endace.com>
> 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.
>> ---
>>   src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
>>   1 files changed, 48 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/pciinit.c b/src/pciinit.c
>> index a574e38..92942d5 100644
>> --- a/src/pciinit.c
>> +++ b/src/pciinit.c
>> @@ -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++;
>> +            }
>>           }
>>       }
>>
>> @@ -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.
>
>> +                   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);
>> -        }
>> -        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?
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.

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Michael S. Tsirkin]

On Thu, Dec 29, 2011 at 06:40:26PM +1300, Alexey Korolev wrote:
> On 29/12/11 00:43, Michael S. Tsirkin wrote:
> >On Wed, Dec 28, 2011 at 06:35:55PM +1300, Alexey Korolev wrote:
> >>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.
> >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.

> >
> >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.
> >
> >>Signed-off-by: Alexey Korolev<alexey.korolev@endace.com>
> >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.
> >>---
> >>  src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
> >>  1 files changed, 48 insertions(+), 21 deletions(-)
> >>
> >>diff --git a/src/pciinit.c b/src/pciinit.c
> >>index a574e38..92942d5 100644
> >>--- a/src/pciinit.c
> >>+++ b/src/pciinit.c
> >>@@ -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++;
> >>+            }
> >>          }
> >>      }
> >>
> >>@@ -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.
> >
> >>+                   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);
> >>-        }
> >>-        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?
> 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.

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Alexey Korolev]

On 30/12/11 05:18, Michael S. Tsirkin wrote:
> On Thu, Dec 29, 2011 at 06:40:26PM +1300, Alexey Korolev wrote:
>> On 29/12/11 00:43, Michael S. Tsirkin wrote:
>>> On Wed, Dec 28, 2011 at 06:35:55PM +1300, Alexey Korolev wrote:
>>>> 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.
>>> 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.
Well it is really difficult to say what is a "legal behavior".
Yes it is legal behavior for PCI operations. But it could be a problem 
in software .
I won't be surprised if a driver fail to start if we substitute the 
regions. It will be very hard
to check all possible cases.

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Alexey Korolev]

On 29/12/11 00:43, Michael S. Tsirkin wrote:
> On Wed, Dec 28, 2011 at 06:35:55PM +1300, Alexey Korolev wrote:
>> 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.
> 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.
>
> 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.
>
>> Signed-off-by: Alexey Korolev<alexey.korolev@endace.com>
> 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.
>> ---
>>   src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
>>   1 files changed, 48 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/pciinit.c b/src/pciinit.c
>> index a574e38..92942d5 100644
>> --- a/src/pciinit.c
>> +++ b/src/pciinit.c
>> @@ -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++;
>> +            }
>>           }
>>       }
>>
>> @@ -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.
>
>> +                   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);
>> -        }
>> -        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?
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

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Michael S. Tsirkin]

On Thu, Dec 29, 2011 at 06:41:36PM +1300, Alexey Korolev wrote:
> On 29/12/11 00:43, Michael S. Tsirkin wrote:
> >On Wed, Dec 28, 2011 at 06:35:55PM +1300, Alexey Korolev wrote:
> >>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.

Can you find out why? qemu has a flag to easily debug
guests with gdb, shouldn't be hard.

> As far as I remember it was qemu-0.15 so may be 1.0 have no such an
> issue. I will check this.
> >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.
> >
> >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.
> >
> >>Signed-off-by: Alexey Korolev<alexey.korolev@endace.com>
> >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.
> >>---
> >>  src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
> >>  1 files changed, 48 insertions(+), 21 deletions(-)
> >>
> >>diff --git a/src/pciinit.c b/src/pciinit.c
> >>index a574e38..92942d5 100644
> >>--- a/src/pciinit.c
> >>+++ b/src/pciinit.c
> >>@@ -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++;
> >>+            }
> >>          }
> >>      }
> >>
> >>@@ -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.
> >
> >>+                   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);
> >>-        }
> >>-        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?
> 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

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Michael S. Tsirkin]

On Thu, Dec 29, 2011 at 06:41:36PM +1300, Alexey Korolev wrote:
> >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.

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

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Alexey Korolev]

On 30/12/11 05:21, Michael S. Tsirkin wrote:
> On Thu, Dec 29, 2011 at 06:41:36PM +1300, Alexey Korolev wrote:
>>> 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.
>
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?

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Kevin O'Connor]

On Fri, Dec 30, 2011 at 06:10:13PM +1300, Alexey Korolev wrote:
> On 30/12/11 05:21, Michael S. Tsirkin wrote:
> >On Thu, Dec 29, 2011 at 06:41:36PM +1300, Alexey Korolev wrote:
> >>>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.
> >
> 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.

There are very few panic calls in seabios.  Halting in the system
firmware is very unfriendly and should be avoided.

As described in my other email, if there is a mix of prefmem bars on a
secondary bus, then just allocate all those bars in the first 4gig.
No reason to halt the boot.

> 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?

You can look at the pci allocation code in coreboot.

-Kevin

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Alexey Korolev]

On 30/12/11 05:21, Michael S. Tsirkin wrote:
> On Thu, Dec 29, 2011 at 06:41:36PM +1300, Alexey Korolev wrote:
>>> 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.
>
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.

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?

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Kevin O'Connor]

On Thu, Dec 29, 2011 at 06:41:36PM +1300, Alexey Korolev wrote:
> On 29/12/11 00:43, Michael S. Tsirkin wrote:
> >On Wed, Dec 28, 2011 at 06:35:55PM +1300, Alexey Korolev wrote:
> >>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.

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?

-Kevin


--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -545,8 +545,10 @@ static void pci_bios_map_devices(struct pci_bus *busses)
                     i, addr, pci->bars[i].size, region_type_name[type]);
             pci_set_io_region_addr(pci, i, addr);
 
-            if (pci->bars[i].is64)
+            if (pci->bars[i].is64) {
                 i++;
+                pci_set_io_region_addr(pci, i, 0);
+            }
         }
     }
 }

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Alexey Korolev]

>> >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?

Yes I see. Will test after short holidays, now it's quite late in New Zealand.
BTW: Tried today version 1.0 to reproduce this issue and could not
reproduce it.

Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

[Kevin O'Connor]

On Wed, Dec 28, 2011 at 01:43:02PM +0200, Michael S. Tsirkin wrote:
> On Wed, Dec 28, 2011 at 06:35:55PM +1300, Alexey Korolev wrote:
> > 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

I would vote for option 1.  Supporting option 2 seems complex and it
would only help the case where there is a large 64bit prefmem bar
(that can't reliably fit in low memory) behind a bridge that supports
64bit prefmem bars that also has a small 32bit prefmem bar (which can
reliably fit in low memory) behind it.  This condition seems too
unlikely to be worth supporting the additional complexity.

-Kevin

Re: [Qemu-devel] [Seabios] [PATCH 0/3] 64bit PCI BARs allocations

[Michael S. Tsirkin]

On Wed, Dec 28, 2011 at 05:41:20PM +1300, Alexey Korolev wrote:
> 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.

Which qemu version did you use?

Re: [Qemu-devel] [Seabios] [PATCH 0/3] 64bit PCI BARs allocations

[Alexey Korolev]

>>
> Patches have been tested on several configurations which includes
>> 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

Re: [Qemu-devel] [Seabios] [PATCH 0/3] 64bit PCI BARs allocations

[Michael S. Tsirkin]

On Thu, Dec 29, 2011 at 09:20:00AM +0000, Alexey Korolev wrote:
> >>
> > Patches have been tested on several configurations which includes
> >> 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?

Re: [Qemu-devel] [Seabios] [PATCH 0/3] 64bit PCI BARs allocations

[Alexey Korolev]

On 30/12/11 05:21, Michael S. Tsirkin wrote:
> On Thu, Dec 29, 2011 at 09:20:00AM +0000, Alexey Korolev wrote:
>>> Patches have been tested on several configurations which includes
>>>> 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?
qemu-kvm