[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Alexey Korolev
Subject: Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges
Date: Thu, 29 Dec 2011 18:00:04 +1300
User-agent: Mozilla/5.0 (X11; Linux i686; rv:8.0) Gecko/20111124 Thunderbird/8.0

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

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]