[Qemu-devel] [PATCH 0/2] seabios: pciinit: fix bar allocation

[Qemu-devel] [PATCH 0/2] seabios: pciinit: fix bar allocation

[Isaku Yamahata]

This patch set fixes pci bar allocation.
Cam Macdonell reported that there was something wrong with 64bit/32bit
512M BAR. This addresses his report.

Isaku Yamahata (2):
  seabios: pciinit: fix 64bit bar initilization.
  seabios: pciinit: fix overflow when bar allocation.

 src/pciinit.c |   56 ++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 36 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH 1/2] seabios: pciinit: fix 64bit bar initilization.

[Isaku Yamahata]

When 64bit bar allocation failed, leave it untouched as 32bit bar case.
There is no point to set higher bit to all 1, it is just leftover from
debug code.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 src/pciinit.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index b110531..f75e552 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -116,12 +116,8 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
 
     int is_64bit = !(val & PCI_BASE_ADDRESS_SPACE_IO) &&
         (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64;
-    if (is_64bit) {
-        if (size > 0) {
-            pci_config_writel(bdf, ofs + 4, 0);
-        } else {
-            pci_config_writel(bdf, ofs + 4, ~0);
-        }
+    if (is_64bit && size > 0) {
+        pci_config_writel(bdf, ofs + 4, 0);
     }
     return is_64bit;
 }
-- 
1.7.1.1

[Qemu-devel] Re: [SeaBIOS] [PATCH 1/2] seabios: pciinit: fix 64bit bar initilization.

[Kevin O'Connor]

On Mon, Jul 26, 2010 at 02:02:45PM +0900, Isaku Yamahata wrote:
> When 64bit bar allocation failed, leave it untouched as 32bit bar case.
> There is no point to set higher bit to all 1, it is just leftover from
> debug code.

Thanks.

-Kevin

[Qemu-devel] [PATCH 2/2] seabios: pciinit: fix overflow when bar allocation.

[Isaku Yamahata]

When allocating bar, overflow can occur.
So add overflow check and don't allocate bar if overflowed.
Overflow check is ugly, but necessary.
Another suggested way is to change related variables u64 from u32
thus overflow can't occur because the related value are all u32 addressable.
Anyway even with u64, it is necessary to the resulted value > max_u32.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 src/pciinit.c |   48 ++++++++++++++++++++++++++++++++++--------------
 1 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index f75e552..9be895c 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -49,6 +49,12 @@ static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
     dprintf(1, "region %d: 0x%08x\n", region_num, addr);
 }
 
+static inline int pci_bar_overflow(u32 addr, u32 size)
+{
+    return (ALIGN(addr, size) < addr) ||
+        (ALIGN(addr, size) + size < ALIGN(addr, size));
+}
+
 /*
  * return value
  *      0:     32bit BAR
@@ -78,7 +84,8 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
     if (val != 0) {
         if (val & PCI_BASE_ADDRESS_SPACE_IO) {
             paddr = &pci_bios_io_addr;
-            if (ALIGN(*paddr, size) + size >= 64 * 1024) {
+            if (pci_bar_overflow(*paddr, size) ||
+                ALIGN(*paddr, size) + size >= 64 * 1024) {
                 dprintf(1,
                         "io region of (bdf 0x%x bar %d) can't be mapped.\n",
                         bdf, region_num);
@@ -90,7 +97,8 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
                  /* If pci_bios_prefmem_addr == 0, keep old behaviour */
                  pci_bios_prefmem_addr != 0) {
             paddr = &pci_bios_prefmem_addr;
-            if (ALIGN(*paddr, size) + size >= BUILD_PCIPREFMEM_END) {
+            if (pci_bar_overflow(*paddr, size) ||
+                ALIGN(*paddr, size) + size >= BUILD_PCIPREFMEM_END) {
                 dprintf(1,
                         "prefmem region of (bdf 0x%x bar %d) can't be mapped. "
                         "decrease BUILD_PCIMEM_SIZE and recompile. size %x\n",
@@ -99,7 +107,8 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
             }
         } else {
             paddr = &pci_bios_mem_addr;
-            if (ALIGN(*paddr, size) + size >= BUILD_PCIMEM_END) {
+            if (pci_bar_overflow(*paddr, size) ||
+                ALIGN(*paddr, size) + size >= BUILD_PCIMEM_END) {
                 dprintf(1,
                         "mem region of (bdf 0x%x bar %d) can't be mapped. "
                         "increase BUILD_PCIMEM_SIZE and recompile. size %x\n",
@@ -152,6 +161,12 @@ static const struct pci_device_id pci_isa_bridge_tbl[] = {
     PCI_DEVICE_END
 };
 
+static inline int pci_addr_overflow(u32 old, u32 base, u32 addr, u32 end)
+{
+    /* if without overflow, old <= base <= addr <= end */
+    return (old > base) || (base > addr) || (addr > end);
+}
+
 #define PCI_IO_ALIGN            4096
 #define PCI_IO_SHIFT            8
 #define PCI_MEMORY_ALIGN        (1UL << 20)
@@ -184,36 +199,41 @@ static void pci_bios_init_device_bridge(u16 bdf, void *arg)
         pci_bios_init_device_in_bus(secbus);
     }
 
-    pci_bios_io_addr = ALIGN(pci_bios_io_addr, PCI_IO_ALIGN);
-    pci_bios_mem_addr = ALIGN(pci_bios_mem_addr, PCI_MEMORY_ALIGN);
-    pci_bios_prefmem_addr =
-        ALIGN(pci_bios_prefmem_addr, PCI_PREF_MEMORY_ALIGN);
-
-    u32 io_end = pci_bios_io_addr;
-    if (io_end == io_base) {
+    u32 io_end = ALIGN(pci_bios_io_addr, PCI_IO_ALIGN);
+    if (pci_addr_overflow(io_old, io_base, pci_bios_io_addr, io_end) ||
+        io_end == io_base) {
         pci_bios_io_addr = io_old;
         io_base = 0xffff;
         io_end = 1;
+    } else {
+        pci_bios_io_addr = io_end;
     }
     pci_config_writeb(bdf, PCI_IO_BASE, io_base >> PCI_IO_SHIFT);
     pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
     pci_config_writeb(bdf, PCI_IO_LIMIT, (io_end - 1) >> PCI_IO_SHIFT);
     pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, 0);
 
-    u32 mem_end = pci_bios_mem_addr;
-    if (mem_end == mem_base) {
+    u32 mem_end = ALIGN(pci_bios_mem_addr, PCI_MEMORY_ALIGN);
+    if (pci_addr_overflow(mem_old, mem_base, pci_bios_mem_addr, mem_end) ||
+        mem_end == mem_base) {
         pci_bios_mem_addr = mem_old;
         mem_base = 0xffffffff;
         mem_end = 1;
+    } else {
+        pci_bios_io_addr = mem_end;
     }
     pci_config_writew(bdf, PCI_MEMORY_BASE, mem_base >> PCI_MEMORY_SHIFT);
     pci_config_writew(bdf, PCI_MEMORY_LIMIT, (mem_end -1) >> PCI_MEMORY_SHIFT);
 
-    u32 prefmem_end = pci_bios_prefmem_addr;
-    if (prefmem_end == prefmem_base) {
+    u32 prefmem_end = ALIGN(pci_bios_prefmem_addr, PCI_PREF_MEMORY_ALIGN);
+    if (pci_addr_overflow(prefmem_old, prefmem_base,
+                          pci_bios_prefmem_addr, prefmem_end) ||
+        prefmem_end == prefmem_base) {
         pci_bios_prefmem_addr = prefmem_old;
         prefmem_base = 0xffffffff;
         prefmem_end = 1;
+    } else {
+        pci_bios_prefmem_addr = prefmem_end;
     }
     pci_config_writew(bdf, PCI_PREF_MEMORY_BASE,
                       prefmem_base >> PCI_PREF_MEMORY_SHIFT);
-- 
1.7.1.1

[Qemu-devel] Re: [SeaBIOS] [PATCH 2/2] seabios: pciinit: fix overflow when bar allocation.

[Kevin O'Connor]

On Mon, Jul 26, 2010 at 02:02:46PM +0900, Isaku Yamahata wrote:
> When allocating bar, overflow can occur.
> So add overflow check and don't allocate bar if overflowed.
> Overflow check is ugly, but necessary.
> Another suggested way is to change related variables u64 from u32
> thus overflow can't occur because the related value are all u32 addressable.
> Anyway even with u64, it is necessary to the resulted value > max_u32.

I wonder if we could create malloc zones for this and then use
pmm_alloc.  The pmm_alloc function already handles overflow and
alignments.

One difficulty, though, is that pmm_alloc doesn't guarentee linear
allocations, and it looks like pci_bios_init_device_bridge assumes
this.

-Kevin

[Qemu-devel] Re: [SeaBIOS] [PATCH 2/2] seabios: pciinit: fix overflow when bar allocation.

[Isaku Yamahata]

On Mon, Jul 26, 2010 at 09:45:42PM -0400, Kevin O'Connor wrote:
> On Mon, Jul 26, 2010 at 02:02:46PM +0900, Isaku Yamahata wrote:
> > When allocating bar, overflow can occur.
> > So add overflow check and don't allocate bar if overflowed.
> > Overflow check is ugly, but necessary.
> > Another suggested way is to change related variables u64 from u32
> > thus overflow can't occur because the related value are all u32 addressable.
> > Anyway even with u64, it is necessary to the resulted value > max_u32.
> 
> I wonder if we could create malloc zones for this and then use
> pmm_alloc.  The pmm_alloc function already handles overflow and
> alignments.
> 
> One difficulty, though, is that pmm_alloc doesn't guarentee linear
> allocations, and it looks like pci_bios_init_device_bridge assumes
> this.

Yes, contiguous allocation is necessary for bridge filtering.
Anyway I'll give pmm_malloc() a try and see what can be done.
-- 
yamahata