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

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

[Isaku Yamahata]

This patch set fixes PCI bar allocation when bar overflow occured.
I checked if pmm_alloc facility can be used, but it doesn't suit for
pci bar allocation. So I resulted in new API, pci_region which
encapsulates region allocation and overflow checks.
The first patch introduces pci_region, and the second patch fixes
the overflow case with pci_region.

Isaku Yamahata (2):
  pci: introduce pci_region to manage pci io/memory/prefmemory regions.
  pciinit: use pci_region functions.

 Makefile         |    3 +-
 src/pci_region.c |   70 +++++++++++++++++++++++++++++++
 src/pciinit.c    |  122 ++++++++++++++++++++++++++---------------------------
 src/util.h       |   15 +++++++
 4 files changed, 147 insertions(+), 63 deletions(-)
 create mode 100644 src/pci_region.c

[Qemu-devel] [PATCH 1/2] pci: introduce pci_region to manage pci io/memory/prefmemory regions.

[Isaku Yamahata]

This patch adds helper functions to manage pci area.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 Makefile         |    3 +-
 src/pci_region.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/util.h       |   15 +++++++++++
 3 files changed, 87 insertions(+), 1 deletions(-)
 create mode 100644 src/pci_region.c

diff --git a/Makefile b/Makefile
index 9d412f1..1663a5d 100644
--- a/Makefile
+++ b/Makefile
@@ -19,7 +19,8 @@ SRCBOTH=misc.c pmm.c stacks.c output.c util.c block.c floppy.c ata.c mouse.c \
 SRC16=$(SRCBOTH) system.c disk.c font.c
 SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c coreboot.c boot.c \
       acpi.c smm.c mptable.c smbios.c pciinit.c optionroms.c mtrr.c \
-      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c dev-i440fx.c
+      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c dev-i440fx.c \
+      pci_region.c
 SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
 
 cc-option = $(shell if test -z "`$(1) $(2) -S -o /dev/null -xc \
diff --git a/src/pci_region.c b/src/pci_region.c
new file mode 100644
index 0000000..a4e71d9
--- /dev/null
+++ b/src/pci_region.c
@@ -0,0 +1,70 @@
+// helper functions to manage pci io/memory/prefetch memory region
+//
+// Copyright (C) 2009 Isaku Yamahata <yamahata at valinux co jp>
+//
+// This file may be distributed under the terms of the GNU LGPLv3 license.
+//
+//
+
+#include "util.h"
+
+#define PCI_REGION_DISABLED     (-1)
+
+void pci_region_init(struct pci_region *r, u32 start, u32 end)
+{
+    r->start = start;
+    r->end = end;
+
+    r->cur_end = start;
+}
+
+static u32 pci_region_alloc_align(struct pci_region *r, u32 size, u32 align)
+{
+    if (r->cur_end == PCI_REGION_DISABLED) {
+        return 0;
+    }
+
+    u32 s = ALIGN(r->cur_end, align);
+    if (s > r->end || s < r->cur_end) {
+        return 0;
+    }
+    u32 e = s + size;
+    if (e > r->end || e < s) {
+        return 0;
+    }
+    r->cur_end = e;
+    return s;
+}
+
+u32 pci_region_alloc(struct pci_region *r, u32 size)
+{
+    return pci_region_alloc_align(r, size, size);
+}
+
+u32 pci_region_align(struct pci_region *r, u32 align)
+{
+    return pci_region_alloc_align(r, 0, align);
+}
+
+void pci_region_revert(struct pci_region *r, u32 addr)
+{
+    r->cur_end = addr;
+}
+
+u32 pci_region_disable(struct pci_region *r)
+{
+    return r->cur_end = PCI_REGION_DISABLED;
+}
+
+u32 pci_region_addr(const struct pci_region *r)
+{
+    if (r->cur_end == PCI_REGION_DISABLED){
+        return r->end;
+    }
+    return r->cur_end;
+}
+
+u32 pci_region_size(const struct pci_region *r)
+{
+    return r->end - r->start;
+}
diff --git a/src/util.h b/src/util.h
index 5cc9f17..ecd1c16 100644
--- a/src/util.h
+++ b/src/util.h
@@ -344,6 +344,21 @@ void qemu_prep_reset(void);
 void smm_save_and_copy(void);
 void smm_relocate_and_restore(void);
 
+// pci_region.c
+struct pci_region {
+    u32 start;
+    u32 end;
+
+    u32 cur_end;
+};
+void pci_region_init(struct pci_region *r, u32 start, u32 end);
+u32 pci_region_alloc(struct pci_region *r, u32 size);
+u32 pci_region_align(struct pci_region *r, u32 align);
+void pci_region_revert(struct pci_region *r, u32 addr);
+u32 pci_region_disable(struct pci_region *r);
+u32 pci_region_addr(const struct pci_region *r);
+u32 pci_region_size(const struct pci_region *r);
+
 // pciinit.c
 extern const u8 pci_irqs[4];
 void pci_bios_allocate_regions(u16 bdf, void *arg);
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH 1/2] pci: introduce pci_region to manage pci io/memory/prefmemory regions.

[Michael S. Tsirkin]

On Mon, Oct 18, 2010 at 06:34:22PM +0900, Isaku Yamahata wrote:
> This patch adds helper functions to manage pci area.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  Makefile         |    3 +-
>  src/pci_region.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util.h       |   15 +++++++++++
>  3 files changed, 87 insertions(+), 1 deletions(-)
>  create mode 100644 src/pci_region.c
> 
> diff --git a/Makefile b/Makefile
> index 9d412f1..1663a5d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -19,7 +19,8 @@ SRCBOTH=misc.c pmm.c stacks.c output.c util.c block.c floppy.c ata.c mouse.c \
>  SRC16=$(SRCBOTH) system.c disk.c font.c
>  SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c coreboot.c boot.c \
>        acpi.c smm.c mptable.c smbios.c pciinit.c optionroms.c mtrr.c \
> -      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c dev-i440fx.c
> +      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c dev-i440fx.c \
> +      pci_region.c
>  SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
>  
>  cc-option = $(shell if test -z "`$(1) $(2) -S -o /dev/null -xc \
> diff --git a/src/pci_region.c b/src/pci_region.c
> new file mode 100644
> index 0000000..a4e71d9
> --- /dev/null
> +++ b/src/pci_region.c
> @@ -0,0 +1,70 @@
> +// helper functions to manage pci io/memory/prefetch memory region
> +//
> +// Copyright (C) 2009 Isaku Yamahata <yamahata at valinux co jp>
> +//
> +// This file may be distributed under the terms of the GNU LGPLv3 license.
> +//
> +//
> +
> +#include "util.h"
> +
> +#define PCI_REGION_DISABLED     (-1)
> +
> +void pci_region_init(struct pci_region *r, u32 start, u32 end)
> +{
> +    r->start = start;
> +    r->end = end;
> +
> +    r->cur_end = start;
> +}
> +
> +static u32 pci_region_alloc_align(struct pci_region *r, u32 size, u32 align)
> +{
> +    if (r->cur_end == PCI_REGION_DISABLED) {
> +        return 0;
> +    }

So is special value PCI_REGION_DISABLED or cur_end?

> +
> +    u32 s = ALIGN(r->cur_end, align);
> +    if (s > r->end || s < r->cur_end) {
> +        return 0;
> +    }
> +    u32 e = s + size;
> +    if (e > r->end || e < s) {
> +        return 0;
> +    }
> +    r->cur_end = e;
> +    return s;
> +}
> +
> +u32 pci_region_alloc(struct pci_region *r, u32 size)
> +{
> +    return pci_region_alloc_align(r, size, size);
> +}
> +
> +u32 pci_region_align(struct pci_region *r, u32 align)
> +{
> +    return pci_region_alloc_align(r, 0, align);
> +}
> +
> +void pci_region_revert(struct pci_region *r, u32 addr)
> +{
> +    r->cur_end = addr;
> +}
> +
> +u32 pci_region_disable(struct pci_region *r)
> +{
> +    return r->cur_end = PCI_REGION_DISABLED;
> +}
> +
> +u32 pci_region_addr(const struct pci_region *r)
> +{
> +    if (r->cur_end == PCI_REGION_DISABLED){
> +        return r->end;
> +    }
> +    return r->cur_end;
> +}
> +
> +u32 pci_region_size(const struct pci_region *r)
> +{
> +    return r->end - r->start;
> +}
> diff --git a/src/util.h b/src/util.h
> index 5cc9f17..ecd1c16 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -344,6 +344,21 @@ void qemu_prep_reset(void);
>  void smm_save_and_copy(void);
>  void smm_relocate_and_restore(void);
>  
> +// pci_region.c
> +struct pci_region {
> +    u32 start;
> +    u32 end;
> +
> +    u32 cur_end;
> +};
> +void pci_region_init(struct pci_region *r, u32 start, u32 end);
> +u32 pci_region_alloc(struct pci_region *r, u32 size);
> +u32 pci_region_align(struct pci_region *r, u32 align);
> +void pci_region_revert(struct pci_region *r, u32 addr);
> +u32 pci_region_disable(struct pci_region *r);
> +u32 pci_region_addr(const struct pci_region *r);
> +u32 pci_region_size(const struct pci_region *r);
> +

Please document structure fields here and functions in the .c file.

>  // pciinit.c
>  extern const u8 pci_irqs[4];
>  void pci_bios_allocate_regions(u16 bdf, void *arg);
> -- 
> 1.7.1.1

[Qemu-devel] Re: [SeaBIOS] [PATCH 1/2] pci: introduce pci_region to manage pci io/memory/prefmemory regions.

[Isaku Yamahata]

On Mon, Oct 18, 2010 at 11:55:08AM +0200, Michael S. Tsirkin wrote:
> > +static u32 pci_region_alloc_align(struct pci_region *r, u32 size, u32 align)
> > +{
> > +    if (r->cur_end == PCI_REGION_DISABLED) {
> > +        return 0;
> > +    }
> 
> So is special value PCI_REGION_DISABLED or cur_end?

cur_end is in special state, and PCI_REGION_DISABLED was chosen
such that cur_end can't be PCI_REGION_DISABLED in normal case.
-- 
yamahata

[Qemu-devel] [PATCH 2/2] pciinit: use pci_region functions.

[Isaku Yamahata]

This patch cleans up pci region allocation with pci_region.
Now it is aware of overflow.

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

diff --git a/src/pciinit.c b/src/pciinit.c
index 0346423..2a01aaa 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -17,9 +17,10 @@
 
 static void pci_bios_init_device_in_bus(int bus);
 
-static u32 pci_bios_io_addr;
-static u32 pci_bios_mem_addr;
-static u32 pci_bios_prefmem_addr;
+static struct pci_region pci_bios_io_region;
+static struct pci_region pci_bios_mem_region;
+static struct pci_region pci_bios_prefmem_region;
+
 /* host irqs corresponding to PCI irqs A-D */
 const u8 pci_irqs[4] = {
     10, 10, 11, 11
@@ -54,7 +55,7 @@ static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
  */
 static int pci_bios_allocate_region(u16 bdf, int region_num)
 {
-    u32 *paddr;
+    struct pci_region *r;
     u32 ofs = pci_bar(bdf, region_num);
 
     u32 old = pci_config_readl(bdf, ofs);
@@ -74,41 +75,34 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
 
     u32 size = (~(val & mask)) + 1;
     if (val != 0) {
+        const char *type;
+        const char *msg;
         if (val & PCI_BASE_ADDRESS_SPACE_IO) {
-            paddr = &pci_bios_io_addr;
-            if (ALIGN(*paddr, size) + size >= 64 * 1024) {
-                dprintf(1,
-                        "io region of (bdf 0x%x bar %d) can't be mapped.\n",
-                        bdf, region_num);
-                size = 0;
-            }
+            r = &pci_bios_io_region;
+            type = "io";
+            msg = "";
         } else if ((val & PCI_BASE_ADDRESS_MEM_PREFETCH) &&
-                 /* keep behaviour on bus = 0 */
-                 pci_bdf_to_bus(bdf) != 0 &&
-                 /* 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) {
-                dprintf(1,
-                        "prefmem region of (bdf 0x%x bar %d) can't be mapped. "
-                        "decrease BUILD_PCIMEM_SIZE and recompile. size %x\n",
-                        bdf, region_num, BUILD_PCIPREFMEM_SIZE);
-                size = 0;
-            }
+                   /* keep behaviour on bus = 0 */
+                   pci_bdf_to_bus(bdf) != 0 &&
+                   /* If pci_bios_prefmem_addr == 0, keep old behaviour */
+                   pci_region_addr(&pci_bios_prefmem_region) != 0) {
+            r = &pci_bios_prefmem_region;
+            type = "prefmem";
+            msg = "decrease BUILD_PCIMEM_SIZE and recompile. size %x";
         } else {
-            paddr = &pci_bios_mem_addr;
-            if (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",
-                        bdf, region_num, BUILD_PCIMEM_SIZE);
-                size = 0;
-            }
+            r = &pci_bios_mem_region;
+            type = "mem";
+            msg = "increase BUILD_PCIMEM_SIZE and recompile.";
         }
-        if (size > 0) {
-            *paddr = ALIGN(*paddr, size);
-            pci_set_io_region_addr(bdf, region_num, *paddr);
-            *paddr += size;
+        u32 addr = pci_region_alloc(r, size);
+        if (addr > 0) {
+            pci_set_io_region_addr(bdf, region_num, addr);
+        } else {
+            size = 0;
+            dprintf(1,
+                    "%s region of (bdf 0x%x bar %d) can't be mapped. "
+                    "%s size %x\n",
+                    type, bdf, region_num, msg, pci_region_size(r));
         }
     }
 
@@ -163,33 +157,34 @@ static void pci_bios_init_device_bridge(u16 bdf, void *arg)
     pci_bios_allocate_region(bdf, 1);
     pci_bios_allocate_region(bdf, PCI_ROM_SLOT);
 
-    u32 io_old = pci_bios_io_addr;
-    u32 mem_old = pci_bios_mem_addr;
-    u32 prefmem_old = pci_bios_prefmem_addr;
+    u32 io_old = pci_region_addr(&pci_bios_io_region);
+    u32 mem_old = pci_region_addr(&pci_bios_mem_region);
+    u32 prefmem_old = pci_region_addr(&pci_bios_prefmem_region);
 
     /* IO BASE is assumed to be 16 bit */
-    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);
+    if (pci_region_align(&pci_bios_io_region, PCI_IO_ALIGN) == 0) {
+        pci_region_disable(&pci_bios_io_region);
+    }
+    if (pci_region_align(&pci_bios_mem_region, PCI_MEMORY_ALIGN) == 0) {
+        pci_region_disable(&pci_bios_mem_region);
+    }
+    if (pci_region_align(&pci_bios_prefmem_region,
+                         PCI_PREF_MEMORY_ALIGN) == 0) {
+        pci_region_disable(&pci_bios_prefmem_region);
+    }
 
-    u32 io_base = pci_bios_io_addr;
-    u32 mem_base = pci_bios_mem_addr;
-    u32 prefmem_base = pci_bios_prefmem_addr;
+    u32 io_base = pci_region_addr(&pci_bios_io_region);
+    u32 mem_base = pci_region_addr(&pci_bios_mem_region);
+    u32 prefmem_base = pci_region_addr(&pci_bios_prefmem_region);
 
     u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS);
     if (secbus > 0) {
         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) {
-        pci_bios_io_addr = io_old;
+    u32 io_end = pci_region_align(&pci_bios_io_region, PCI_IO_ALIGN);
+    if (io_end == 0) {
+        pci_region_revert(&pci_bios_io_region, io_old);
         io_base = 0xffff;
         io_end = 1;
     }
@@ -198,18 +193,19 @@ static void pci_bios_init_device_bridge(u16 bdf, void *arg)
     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) {
-        pci_bios_mem_addr = mem_old;
+    u32 mem_end = pci_region_align(&pci_bios_mem_region, PCI_MEMORY_ALIGN);
+    if (mem_end == 0) {
+        pci_region_revert(&pci_bios_mem_region, mem_old);
         mem_base = 0xffffffff;
         mem_end = 1;
     }
     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) {
-        pci_bios_prefmem_addr = prefmem_old;
+    u32 prefmem_end = pci_region_align(&pci_bios_prefmem_region,
+                                       PCI_PREF_MEMORY_ALIGN);
+    if (prefmem_end == 0) {
+        pci_region_revert(&pci_bios_prefmem_region, prefmem_old);
         prefmem_base = 0xffffffff;
         prefmem_end = 1;
     }
@@ -406,9 +402,11 @@ pci_setup(void)
 
     dprintf(3, "pci setup\n");
 
-    pci_bios_io_addr = 0xc000;
-    pci_bios_mem_addr = BUILD_PCIMEM_START;
-    pci_bios_prefmem_addr = BUILD_PCIPREFMEM_START;
+    pci_region_init(&pci_bios_io_region, 0xc000, 64 * 1024);
+    pci_region_init(&pci_bios_mem_region,
+                    BUILD_PCIMEM_START, BUILD_PCIMEM_END);
+    pci_region_init(&pci_bios_prefmem_region,
+                    BUILD_PCIPREFMEM_START, BUILD_PCIPREFMEM_END);
 
     pci_bios_init_bus();
 
-- 
1.7.1.1

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

[Michael S. Tsirkin]

On Mon, Oct 18, 2010 at 06:34:21PM +0900, Isaku Yamahata wrote:
> This patch set fixes PCI bar allocation when bar overflow occured.
> I checked if pmm_alloc facility can be used, but it doesn't suit for
> pci bar allocation. So I resulted in new API, pci_region which
> encapsulates region allocation and overflow checks.
> The first patch introduces pci_region, and the second patch fixes
> the overflow case with pci_region.
> 
> Isaku Yamahata (2):
>   pci: introduce pci_region to manage pci io/memory/prefmemory regions.
>   pciinit: use pci_region functions.
> 
>  Makefile         |    3 +-
>  src/pci_region.c |   70 +++++++++++++++++++++++++++++++
>  src/pciinit.c    |  122 ++++++++++++++++++++++++++---------------------------
>  src/util.h       |   15 +++++++
>  4 files changed, 147 insertions(+), 63 deletions(-)
>  create mode 100644 src/pci_region.c

Could you clarify what do you mean by bar overflow please?

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

[Isaku Yamahata]

On Mon, Oct 18, 2010 at 11:47:42AM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 18, 2010 at 06:34:21PM +0900, Isaku Yamahata wrote:
> > This patch set fixes PCI bar allocation when bar overflow occured.
> > I checked if pmm_alloc facility can be used, but it doesn't suit for
> > pci bar allocation. So I resulted in new API, pci_region which
> > encapsulates region allocation and overflow checks.
> > The first patch introduces pci_region, and the second patch fixes
> > the overflow case with pci_region.
> > 
> > Isaku Yamahata (2):
> >   pci: introduce pci_region to manage pci io/memory/prefmemory regions.
> >   pciinit: use pci_region functions.
> > 
> >  Makefile         |    3 +-
> >  src/pci_region.c |   70 +++++++++++++++++++++++++++++++
> >  src/pciinit.c    |  122 ++++++++++++++++++++++++++---------------------------
> >  src/util.h       |   15 +++++++
> >  4 files changed, 147 insertions(+), 63 deletions(-)
> >  create mode 100644 src/pci_region.c
> 
> Could you clarify what do you mean by bar overflow please?

This is originally raised by Cam.
http://www.seabios.org/pipermail/seabios/2010-August/000888.html
For example, pci_bios_bios() has *paddr += size.
It may overflow and really does with huge bar(1G, 2G...)
reported by Cam and Adnan.

This patch series is revised version of the following as
Kevin has requested me to try to use pmm_malloc() in order
to avoid ugly overflow check.
http://www.seabios.org/pipermail/seabios/2010-July/000794.html
-- 
yamahata