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

["Michael S. Tsirkin" <mst@redhat.com>]

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.