From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35421) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RfrtG-0002MN-VF for qemu-devel@nongnu.org; Wed, 28 Dec 2011 06:41:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RfrtF-0002YC-Fy for qemu-devel@nongnu.org; Wed, 28 Dec 2011 06:41:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:32696) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RfrtF-0002Y7-5K for qemu-devel@nongnu.org; Wed, 28 Dec 2011 06:41:49 -0500 Date: Wed, 28 Dec 2011 13:43:02 +0200 From: "Michael S. Tsirkin" Message-ID: <20111228114302.GB26310@redhat.com> References: <4EFA9DF0.7050902@endace.com> <4EFAAABB.3020804@endace.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4EFAAABB.3020804@endace.com> Subject: Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Korolev Cc: sfd@endace.com, yamahata@valinux.co.jp, seabios@seabios.org, qemu-devel@nongnu.org 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 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 >