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