[PATCH v4 0/2] pci: load memory window setup from host

[Michael S. Tsirkin]

This is a refactoring of patch
PATCH v2: pci: load memory window setup from host.

Please note qemu merged the required interface already.
Please review, and consider for release.

Changes from v3:
    - fix bug causing windows to crash
    - better debug output (when enabled)
Changes from v2:
    - address comments by Kevin:
      avoid dynamic memory allocation
      refactor code to avoid special-casing loaded windows
    - split out to two patch series
Changes from v1:
        - fix bug in 64 bit range check
        - address Kevin's comments:
                move file load into pciinit.c
                dont reorder initialization
                sizeof style fix

Michael S. Tsirkin (2):
  pciinit: refactor mem init code
  pci: load memory window setup from host

src/pciinit.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 24 deletions(-)

[PATCH v4 1/2] pciinit: refactor mem init code

[Michael S. Tsirkin]

Refactor memory initialization code:
- split window initialization from other setup
- pass all window data around by pointer

This is in preparation for getting window data
from qemu.

Signed-off-by: Michael S. Tsirkin 
---
 src/pciinit.c | 102 +++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 79 insertions(+), 23 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index 6d7f8fa..ef5e33b 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -59,6 +59,13 @@ struct pci_bus {
     struct pci_device *bus_dev;
 };

+struct pci_mem {
+    u64 start32;
+    u64 end32;
+    u64 start64;
+    u64 end64;
+};
+
 static u32 pci_bar(struct pci_device *pci, int region_num)
 {
     if (region_num != PCI_ROM_SLOT) {
@@ -357,17 +364,22 @@ static void pci_enable_default_vga(void)
  * Platform device initialization
  ****************************************************************/

+void i440fx_setup(struct pci_device *dev, void *arg)
+{
+    pci_slot_get_irq = piix_pci_slot_get_irq;
+}
+
 void i440fx_mem_addr_setup(struct pci_device *dev, void *arg)
 {
+    struct pci_mem *mem = arg;
+
     if (RamSize <= 0x80000000)
-        pcimem_start = 0x80000000;
+        mem->start32 = 0x80000000;
     else if (RamSize <= 0xc0000000)
-        pcimem_start = 0xc0000000;
-
-    pci_slot_get_irq = piix_pci_slot_get_irq;
+        mem->start32 = 0xc0000000;
 }

-void mch_mem_addr_setup(struct pci_device *dev, void *arg)
+void mch_setup(struct pci_device *dev, void *arg)
 {
     u64 addr = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
     u32 size = Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
@@ -381,13 +393,20 @@ void mch_mem_addr_setup(struct pci_device *dev, void *arg)
     pci_config_writel(bdf, Q35_HOST_BRIDGE_PCIEXBAR, lower);
     add_e820(addr, size, E820_RESERVED);

-    /* setup pci i/o window (above mmconfig) */
-    pcimem_start = addr + size;
-
     pci_slot_get_irq = mch_pci_slot_get_irq;
 }

-static const struct pci_device_id pci_platform_tbl[] = {
+void mch_mem_addr_setup(struct pci_device *dev, void *arg)
+{
+    struct pci_mem *mem = arg;
+    u64 addr = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
+    u32 size = Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
+
+    /* setup pci i/o window (above mmconfig) */
+    mem->start32 = addr + size;
+}
+
+static const struct pci_device_id pci_mem_addr_setup_tbl[] = {
     PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441,
                i440fx_mem_addr_setup),
     PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
@@ -395,11 +414,34 @@ static const struct pci_device_id pci_platform_tbl[] = {
     PCI_DEVICE_END
 };

+static const struct pci_device_id pci_platform_setup_tbl[] = {
+    PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441,
+               i440fx_setup),
+    PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
+               mch_setup),
+    PCI_DEVICE_END
+};
+
+static void pci_bios_init_mem_addr(struct pci_mem *mem)
+{
+    struct pci_device *pci;
+
+    /* Default in case no device found */
+    mem->start32 = RamSize;
+    mem->end32 = BUILD_PCIMEM_END;
+    mem->start64 = 0x100000000LL + RamSizeOver4G;
+    mem->end64 = 0;
+
+    foreachpci(pci) {
+        pci_init_device(pci_mem_addr_setup_tbl, pci, mem);
+    }
+}
+
 static void pci_bios_init_platform(void)
 {
     struct pci_device *pci;
     foreachpci(pci) {
-        pci_init_device(pci_platform_tbl, pci, NULL);
+        pci_init_device(pci_platform_setup_tbl, pci, NULL);
     }
 }

@@ -676,7 +718,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
  ****************************************************************/

// Setup region bases (given the regions' size and alignment)
-static int pci_bios_init_root_regions(struct pci_bus *bus)
+static int pci_bios_init_root_regions(struct pci_bus *bus, struct pci_mem *mem)
 {
     bus->r[PCI_REGION_TYPE_IO].base = 0xc000;

@@ -690,13 +732,13 @@ static int pci_bios_init_root_regions(struct pci_bus *bus)
     }
     u64 sum = pci_region_sum(r_end);
     u64 align = pci_region_align(r_end);
-    r_end->base = ALIGN_DOWN((pcimem_end - sum), align);
+    r_end->base = ALIGN_DOWN((mem->end32 - sum), align);
     sum = pci_region_sum(r_start);
     align = pci_region_align(r_start);
     r_start->base = ALIGN_DOWN((r_end->base - sum), align);

-    if ((r_start->base < pcimem_start) ||
-         (r_start->base > pcimem_end))
+    if ((r_start->base < mem->start32) ||
+         (r_start->base > mem->end32))
         // Memory range requested is larger than available.
         return -1;
     return 0;
@@ -755,9 +797,9 @@ static void pci_region_map_entries(struct pci_bus *busses, struct pci_region *r)
     }
 }

-static void pci_bios_map_devices(struct pci_bus *busses)
+static void pci_bios_map_devices(struct pci_bus *busses, struct pci_mem *mem)
 {
-    if (pci_bios_init_root_regions(busses)) {
+    if (pci_bios_init_root_regions(busses, mem)) {
         struct pci_region r64_mem, r64_pref;
         r64_mem.list.first = NULL;
         r64_pref.list.first = NULL;
@@ -766,7 +808,7 @@ static void pci_bios_map_devices(struct pci_bus *busses)
         pci_region_migrate_64bit_entries(&busses[0].r[PCI_REGION_TYPE_PREFMEM],
                                          &r64_pref);

-        if (pci_bios_init_root_regions(busses))
+        if (pci_bios_init_root_regions(busses, mem))
             panic("PCI: out of 32bit address space\n");

u64 sum_mem = pci_region_sum(&r64_mem);
@@ -774,16 +816,24 @@ static void pci_bios_map_devices(struct pci_bus *busses)
         u64 align_mem = pci_region_align(&r64_mem);
         u64 align_pref = pci_region_align(&r64_pref);

-        r64_mem.base = ALIGN(0x100000000LL + RamSizeOver4G, align_mem);
+        r64_mem.base = ALIGN(mem->start64, align_mem);
+        /*
+         * We don't know where the window ends, so pack all 64 bit memory
+         * at start of window.
+         */
         r64_pref.base = ALIGN(r64_mem.base + sum_mem, align_pref);
-        pcimem64_start = r64_mem.base;
-        pcimem64_end = r64_pref.base + sum_pref;
+
+        mem->start64 = r64_mem.base;
+        mem->end64 = r64_pref.base + sum_pref;
+
+        if (mem->end64 < mem->start64)
+            panic("PCI: out of 64bit address space\n");

pci_region_map_entries(busses, &r64_mem);
         pci_region_map_entries(busses, &r64_pref);
     } else {
         // no bars mapped high -> drop 64bit window (see dsdt)
-        pcimem64_start = 0;
+        mem->start64 = mem->end64 = 0;
     }
     // Map regions on each device.
     int bus;
@@ -802,6 +852,8 @@ static void pci_bios_map_devices(struct pci_bus *busses)
 void
 pci_setup(void)
 {
+    struct pci_mem mem;
+
     if (!CONFIG_QEMU)
         return;

@@ -816,7 +868,7 @@ pci_setup(void)
     dprintf(1, "=== PCI device probing ===\n");
     pci_probe_devices();

-    pcimem_start = RamSize;
+    pci_bios_init_mem_addr(&mem);
     pci_bios_init_platform();

dprintf(1, "=== PCI new allocation pass #1 ===\n");
@@ -830,7 +882,11 @@ pci_setup(void)
         return;

dprintf(1, "=== PCI new allocation pass #2 ===\n");
-    pci_bios_map_devices(busses);
+    pci_bios_map_devices(busses, &mem);
+    pcimem_start = mem.start32;
+    pcimem_end = mem.end32;
+    pcimem64_start = mem.start64;
+    pcimem64_end = mem.end64;

pci_bios_init_devices();

[PATCH v4 1/2] pciinit: refactor mem init code

[Igor Mammedov]

On Mon, 29 Jul 2013 12:33:46 +0300
"Michael S. Tsirkin"  wrote:
> Refactor memory initialization code:
> split window initialization from other setup
> pass all window data around by pointer
>
> This is in preparation for getting window data
> from qemu.
> Signed-off-by: Michael S. Tsirkin
>
> src/pciinit.c | 102 +++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 79 insertions(+), 23 deletions(-)
>
> diff --git a/src/pciinit.c b/src/pciinit.c
> index 6d7f8fa..ef5e33b 100644
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -59,6 +59,13 @@ struct pci_bus {
>      struct pci_device *bus_dev;
>  };
>
> +struct pci_mem {
> u64 start32;
> u64 end32;
> u64 start64;
> u64 end64;
>
> +};
>
> static u32 pci_bar(struct pci_device *pci, int region_num)
>  {
>      if (region_num != PCI_ROM_SLOT) {
> @@ -357,17 +364,22 @@ static void pci_enable_default_vga(void)
> Platform device initialization
>
> ****************************************************************/
>
> +void i440fx_setup(struct pci_device *dev, void *arg)
> +{
> pci_slot_get_irq = piix_pci_slot_get_irq;
>
> +}
>
> void i440fx_mem_addr_setup(struct pci_device *dev, void *arg)
>  {
> struct pci_mem *mem = arg;
> if (RamSize <= 0x80000000)
>    pcimem_start = 0x80000000;
>    mem->start32 = 0x80000000;
> else if (RamSize <= 0xc0000000)
>    pcimem_start = 0xc0000000;
> pci_slot_get_irq = piix_pci_slot_get_irq;
>    mem->start32 = 0xc0000000;
>
> }
>
> -void mch_mem_addr_setup(struct pci_device *dev, void *arg)
> +void mch_setup(struct pci_device *dev, void *arg)
>  {
>      u64 addr = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
>      u32 size = Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
> @@ -381,13 +393,20 @@ void mch_mem_addr_setup(struct pci_device *dev, void *arg)
>      pci_config_writel(bdf, Q35_HOST_BRIDGE_PCIEXBAR, lower);
>      add_e820(addr, size, E820_RESERVED);
> /* setup pci i/o window (above mmconfig) */
> pcimem_start = addr + size;
> pci_slot_get_irq = mch_pci_slot_get_irq;
>
> }
>
> -static const struct pci_device_id pci_platform_tbl[] = {
> +void mch_mem_addr_setup(struct pci_device *dev, void *arg)
> +{
> struct pci_mem *mem = arg;
> u64 addr = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
> u32 size = Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
> /* setup pci i/o window (above mmconfig) */
> mem->start32 = addr + size;
>
> +}
>
> +static const struct pci_device_id pci_mem_addr_setup_tbl[] = {
>      PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441,
>                 i440fx_mem_addr_setup),
>      PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
> @@ -395,11 +414,34 @@ static const struct pci_device_id pci_platform_tbl[] = {
>      PCI_DEVICE_END
>  };
>
> +static const struct pci_device_id pci_platform_setup_tbl[] = {
> PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441,
>           i440fx_setup),
> PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
>           mch_setup),
> PCI_DEVICE_END
>
> +};
>
> +static void pci_bios_init_mem_addr(struct pci_mem *mem)
> +{
> struct pci_device *pci;
> /* Default in case no device found */
> mem->start32 = RamSize;
> mem->end32 = BUILD_PCIMEM_END;
> mem->start64 = 0x100000000LL + RamSizeOver4G;
> mem->end64 = 0;

perhaps mem->end64 = mem->start64 to avoid negative diff if it's ever used?
> foreachpci(pci) {
>    pci_init_device(pci_mem_addr_setup_tbl, pci, mem);
> }
>
> +}
>
> static void pci_bios_init_platform(void)
>  {
>      struct pci_device *pci;
>      foreachpci(pci) {
>    pci_init_device(pci_platform_tbl, pci, NULL);
>    pci_init_device(pci_platform_setup_tbl, pci, NULL);
> }
>
> }
>
> @@ -676,7 +718,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>   ****************************************************************/
>
> // Setup region bases (given the regions' size and alignment)
> -static int pci_bios_init_root_regions(struct pci_bus *bus)
> +static int pci_bios_init_root_regions(struct pci_bus *bus, struct pci_mem *mem)
>  {
>      bus->r[PCI_REGION_TYPE_IO].base = 0xc000;
>
> @@ -690,13 +732,13 @@ static int pci_bios_init_root_regions(struct pci_bus *bus)
>      }
>      u64 sum = pci_region_sum(r_end);
>      u64 align = pci_region_align(r_end);
> r_end->base = ALIGN_DOWN((pcimem_end - sum), align);
> r_end->base = ALIGN_DOWN((mem->end32 - sum), align);
> sum = pci_region_sum(r_start);
> align = pci_region_align(r_start);
> r_start->base = ALIGN_DOWN((r_end->base - sum), align);
> if ((r_start->base < pcimem_start) ||
>     (r_start->base > pcimem_end))
> if ((r_start->base < mem->start32) ||
>     (r_start->base > mem->end32))
>    // Memory range requested is larger than available.
>    return -1;
> return 0;
>
> @@ -755,9 +797,9 @@ static void pci_region_map_entries(struct pci_bus *busses, struct pci_region *r)
>      }
>  }
>
> -static void pci_bios_map_devices(struct pci_bus *busses)
> +static void pci_bios_map_devices(struct pci_bus *busses, struct pci_mem *mem)
>  {
> if (pci_bios_init_root_regions(busses)) {
> if (pci_bios_init_root_regions(busses, mem)) {
>     struct pci_region r64_mem, r64_pref;
>     r64_mem.list.first = NULL;
>     r64_pref.list.first = NULL;
>
> @@ -766,7 +808,7 @@ static void pci_bios_map_devices(struct pci_bus *busses)
>          pci_region_migrate_64bit_entries(&busses[0].r[PCI_REGION_TYPE_PREFMEM],
>                                           &r64_pref);
>    if (pci_bios_init_root_regions(busses))
>    if (pci_bios_init_root_regions(busses, mem))
>        panic("PCI: out of 32bit address space\n");
>
>    u64 sum_mem = pci_region_sum(&r64_mem);
>
> @@ -774,16 +816,24 @@ static void pci_bios_map_devices(struct pci_bus *busses)
>          u64 align_mem = pci_region_align(&r64_mem);
>          u64 align_pref = pci_region_align(&r64_pref);
>    r64_mem.base = ALIGN(0x100000000LL + RamSizeOver4G, align_mem);
>    r64_mem.base = ALIGN(mem->start64, align_mem);
>    /*
>     * We don't know where the window ends, so pack all 64 bit memory
>     * at start of window.
>     */
>    r64_pref.base = ALIGN(r64_mem.base + sum_mem, align_pref);
>    pcimem64_start = r64_mem.base;
>    pcimem64_end = r64_pref.base + sum_pref;
>    mem->start64 = r64_mem.base;
>    mem->end64 = r64_pref.base + sum_pref;
>    if (mem->end64 < mem->start64)
>        panic("PCI: out of 64bit address space\n");
>
>    pci_region_map_entries(busses, &r64_mem);
>    pci_region_map_entries(busses, &r64_pref);
>
> } else {
>        // no bars mapped high -> drop 64bit window (see dsdt)
>    pcimem64_start = 0;
>    mem->start64 = mem->end64 = 0;
> }
>    // Map regions on each device.
>    int bus;
>
> @@ -802,6 +852,8 @@ static void pci_bios_map_devices(struct pci_bus *busses)
>  void
>  pci_setup(void)
>  {
> struct pci_mem mem;
> if (!CONFIG_QEMU)
>        return;
>
> @@ -816,7 +868,7 @@ pci_setup(void)
>      dprintf(1, "=== PCI device probing ===\n");
>      pci_probe_devices();
> pcimem_start = RamSize;
>
> pci_bios_init_mem_addr(&mem);
> pci_bios_init_platform();
>
> dprintf(1, "=== PCI new allocation pass #1 ===\n");
>
> @@ -830,7 +882,11 @@ pci_setup(void)
>          return;
>  dprintf(1, "=== PCI new allocation pass #2 ===\n");
> pci_bios_map_devices(busses);
>
> pci_bios_map_devices(busses, &mem);
>
> pcimem_start = mem.start32;
>
> pcimem_end = mem.end32;
>
> pcimem64_start = mem.start64;
>
> pcimem64_end = mem.end64;
>
> pci_bios_init_devices();

[PATCH v4 1/2] pciinit: refactor mem init code

[Michael S. Tsirkin]

On Mon, Jul 29, 2013 at 01:23:21PM +0200, Igor Mammedov wrote:
> On Mon, 29 Jul 2013 12:33:46 +0300
> "Michael S. Tsirkin"  wrote:
> > Refactor memory initialization code:
> > split window initialization from other setup
> > pass all window data around by pointer
> >
> > This is in preparation for getting window data
> > from qemu.
> > Signed-off-by: Michael S. Tsirkin
> >
> > src/pciinit.c | 102 +++++++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 79 insertions(+), 23 deletions(-)
> >
> > diff --git a/src/pciinit.c b/src/pciinit.c
> > index 6d7f8fa..ef5e33b 100644
> > --- a/src/pciinit.c
> > +++ b/src/pciinit.c
> > @@ -59,6 +59,13 @@ struct pci_bus {
> >      struct pci_device *bus_dev;
> >  };
> >
> > +struct pci_mem {
> > u64 start32;
> > u64 end32;
> > u64 start64;
> > u64 end64;
> >
> > +};
> >
> > static u32 pci_bar(struct pci_device *pci, int region_num)
> >  {
> >      if (region_num != PCI_ROM_SLOT) {
> > @@ -357,17 +364,22 @@ static void pci_enable_default_vga(void)
> > Platform device initialization
> >
> > ****************************************************************/
> >
> > +void i440fx_setup(struct pci_device *dev, void *arg)
> > +{
> > pci_slot_get_irq = piix_pci_slot_get_irq;
> >
> > +}
> >
> > void i440fx_mem_addr_setup(struct pci_device *dev, void *arg)
> >  {
> > struct pci_mem *mem = arg;
> > if (RamSize <= 0x80000000)
> >    pcimem_start = 0x80000000;
> >    mem->start32 = 0x80000000;
> > else if (RamSize <= 0xc0000000)
> >    pcimem_start = 0xc0000000;
> > pci_slot_get_irq = piix_pci_slot_get_irq;
> >    mem->start32 = 0xc0000000;
> >
> > }
> >
> > -void mch_mem_addr_setup(struct pci_device *dev, void *arg)
> > +void mch_setup(struct pci_device *dev, void *arg)
> >  {
> >      u64 addr = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
> >      u32 size = Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
> > @@ -381,13 +393,20 @@ void mch_mem_addr_setup(struct pci_device *dev, void *arg)
> >      pci_config_writel(bdf, Q35_HOST_BRIDGE_PCIEXBAR, lower);
> >      add_e820(addr, size, E820_RESERVED);
> > /* setup pci i/o window (above mmconfig) */
> > pcimem_start = addr + size;
> > pci_slot_get_irq = mch_pci_slot_get_irq;
> >
> > }
> >
> > -static const struct pci_device_id pci_platform_tbl[] = {
> > +void mch_mem_addr_setup(struct pci_device *dev, void *arg)
> > +{
> > struct pci_mem *mem = arg;
> > u64 addr = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
> > u32 size = Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
> > /* setup pci i/o window (above mmconfig) */
> > mem->start32 = addr + size;
> >
> > +}
> >
> > +static const struct pci_device_id pci_mem_addr_setup_tbl[] = {
> >      PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441,
> >                 i440fx_mem_addr_setup),
> >      PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
> > @@ -395,11 +414,34 @@ static const struct pci_device_id pci_platform_tbl[] = {
> >      PCI_DEVICE_END
> >  };
> >
> > +static const struct pci_device_id pci_platform_setup_tbl[] = {
> > PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441,
> >           i440fx_setup),
> > PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
> >           mch_setup),
> > PCI_DEVICE_END
> >
> > +};
> >
> > +static void pci_bios_init_mem_addr(struct pci_mem *mem)
> > +{
> > struct pci_device *pci;
> > /* Default in case no device found */
> > mem->start32 = RamSize;
> > mem->end32 = BUILD_PCIMEM_END;
> > mem->start64 = 0x100000000LL + RamSizeOver4G;
> > mem->end64 = 0;
>
> perhaps mem->end64 = mem->start64 to avoid negative diff if it's ever used?

An empty range is as likely to crash guest, no?
I haven't actually tried.

Below we panic on mem->end64 < mem->start64, this
won't trigger if end == start.
> > foreachpci(pci) {
> >    pci_init_device(pci_mem_addr_setup_tbl, pci, mem);
> > }
> >
> > +}
> >
> > static void pci_bios_init_platform(void)
> >  {
> >      struct pci_device *pci;
> >      foreachpci(pci) {
> >    pci_init_device(pci_platform_tbl, pci, NULL);
> >    pci_init_device(pci_platform_setup_tbl, pci, NULL);
> > }
> >
> > }
> >
> > @@ -676,7 +718,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> >   ****************************************************************/
> >
> > // Setup region bases (given the regions' size and alignment)
> > -static int pci_bios_init_root_regions(struct pci_bus *bus)
> > +static int pci_bios_init_root_regions(struct pci_bus *bus, struct pci_mem *mem)
> >  {
> >      bus->r[PCI_REGION_TYPE_IO].base = 0xc000;
> >
> > @@ -690,13 +732,13 @@ static int pci_bios_init_root_regions(struct pci_bus *bus)
> >      }
> >      u64 sum = pci_region_sum(r_end);
> >      u64 align = pci_region_align(r_end);
> > r_end->base = ALIGN_DOWN((pcimem_end - sum), align);
> > r_end->base = ALIGN_DOWN((mem->end32 - sum), align);
> > sum = pci_region_sum(r_start);
> > align = pci_region_align(r_start);
> > r_start->base = ALIGN_DOWN((r_end->base - sum), align);
> > if ((r_start->base < pcimem_start) ||
> >     (r_start->base > pcimem_end))
> > if ((r_start->base < mem->start32) ||
> >     (r_start->base > mem->end32))
> >    // Memory range requested is larger than available.
> >    return -1;
> > return 0;
> >
> > @@ -755,9 +797,9 @@ static void pci_region_map_entries(struct pci_bus *busses, struct pci_region *r)
> >      }
> >  }
> >
> > -static void pci_bios_map_devices(struct pci_bus *busses)
> > +static void pci_bios_map_devices(struct pci_bus *busses, struct pci_mem *mem)
> >  {
> > if (pci_bios_init_root_regions(busses)) {
> > if (pci_bios_init_root_regions(busses, mem)) {
> >     struct pci_region r64_mem, r64_pref;
> >     r64_mem.list.first = NULL;
> >     r64_pref.list.first = NULL;
> >
> > @@ -766,7 +808,7 @@ static void pci_bios_map_devices(struct pci_bus *busses)
> >          pci_region_migrate_64bit_entries(&busses[0].r[PCI_REGION_TYPE_PREFMEM],
> >                                           &r64_pref);
> >    if (pci_bios_init_root_regions(busses))
> >    if (pci_bios_init_root_regions(busses, mem))
> >        panic("PCI: out of 32bit address space\n");
> >
> >    u64 sum_mem = pci_region_sum(&r64_mem);
> >
> > @@ -774,16 +816,24 @@ static void pci_bios_map_devices(struct pci_bus *busses)
> >          u64 align_mem = pci_region_align(&r64_mem);
> >          u64 align_pref = pci_region_align(&r64_pref);
> >    r64_mem.base = ALIGN(0x100000000LL + RamSizeOver4G, align_mem);
> >    r64_mem.base = ALIGN(mem->start64, align_mem);
> >    /*
> >     * We don't know where the window ends, so pack all 64 bit memory
> >     * at start of window.
> >     */
> >    r64_pref.base = ALIGN(r64_mem.base + sum_mem, align_pref);
> >    pcimem64_start = r64_mem.base;
> >    pcimem64_end = r64_pref.base + sum_pref;
> >    mem->start64 = r64_mem.base;
> >    mem->end64 = r64_pref.base + sum_pref;
> >    if (mem->end64 < mem->start64)
> >        panic("PCI: out of 64bit address space\n");
> >
> >    pci_region_map_entries(busses, &r64_mem);
> >    pci_region_map_entries(busses, &r64_pref);
> >
> > } else {
> >        // no bars mapped high -> drop 64bit window (see dsdt)
> >    pcimem64_start = 0;
> >    mem->start64 = mem->end64 = 0;
> > }
> >    // Map regions on each device.
> >    int bus;
> >
> > @@ -802,6 +852,8 @@ static void pci_bios_map_devices(struct pci_bus *busses)
> >  void
> >  pci_setup(void)
> >  {
> > struct pci_mem mem;
> > if (!CONFIG_QEMU)
> >        return;
> >
> > @@ -816,7 +868,7 @@ pci_setup(void)
> >      dprintf(1, "=== PCI device probing ===\n");
> >      pci_probe_devices();
> > pcimem_start = RamSize;
> >
> > pci_bios_init_mem_addr(&mem);
> > pci_bios_init_platform();
> >
> > dprintf(1, "=== PCI new allocation pass #1 ===\n");
> >
> > @@ -830,7 +882,11 @@ pci_setup(void)
> >          return;
> >  dprintf(1, "=== PCI new allocation pass #2 ===\n");
> > pci_bios_map_devices(busses);
> >
> > pci_bios_map_devices(busses, &mem);
> >
> > pcimem_start = mem.start32;
> >
> > pcimem_end = mem.end32;
> >
> > pcimem64_start = mem.start64;
> >
> > pcimem64_end = mem.end64;
> >
> > pci_bios_init_devices();

[PATCH v4 1/2] pciinit: refactor mem init code

[Kevin O'Connor]

On Mon, Jul 29, 2013 at 12:33:46PM +0300, Michael S. Tsirkin wrote:
> Refactor memory initialization code:
> split window initialization from other setup
> pass all window data around by pointer
>
> This is in preparation for getting window data
> from qemu.

I'm struggling to understand this patch.  Why does it setup the ranges
in 'struct pci_mem mem' and then pass 'mem' around to a bunch of
functions, when it ultimately populates the four global pcimem*
variables anyway?  It seems the patch would be simpler if it just
populated the globals at the start and used the global variables
everywhere.  (The subsequent "load memory window" patch can read
etc/pci-info and populate the globals as well.)

-Kevin

[PATCH v4 1/2] pciinit: refactor mem init code

[Michael S. Tsirkin]

On Wed, Aug 07, 2013 at 09:59:09PM -0400, Kevin O'Connor wrote:
> On Mon, Jul 29, 2013 at 12:33:46PM +0300, Michael S. Tsirkin wrote:
> > Refactor memory initialization code:
> > split window initialization from other setup
> > pass all window data around by pointer
> >
> > This is in preparation for getting window data
> > from qemu.
>
> I'm struggling to understand this patch.  Why does it setup the ranges
> in 'struct pci_mem mem' and then pass 'mem' around to a bunch of
> functions, when it ultimately populates the four global pcimem*
> variables anyway?  It seems the patch would be simpler if it just
> populated the globals at the start and used the global variables
> everywhere.  (The subsequent "load memory window" patch can read
> etc/pci-info and populate the globals as well.)
>
> -Kevin

I generally dislike globals so I thought it's a step in the correct
direction.  A subsequent patch can get rid of the globals completely and
pass everything by pointer.
But OK, I'll split this change out and leave it for another day.

[PATCH v4 2/2] pci: load memory window setup from host

[Michael S. Tsirkin]

Load memory window setup for pci from host.
This makes it possible for host to make sure
setup matches hardware exactly: especially important
for when ACPI tables are loaded from host.
This will also make it easier to add more chipsets
down the road.

Signed-off-by: Michael S. Tsirkin 
---
 src/pciinit.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index ef5e33b..cc12040 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -15,6 +15,7 @@
 #include "paravirt.h" // RamSize
 #include "dev-q35.h" // Q35_HOST_BRIDGE_PCIEXBAR_ADDR
 #include "list.h" // struct hlist_node
+#include "byteorder.h" // le64_to_cpu

#define PCI_DEVICE_MEM_MIN     0x1000
 #define PCI_BRIDGE_IO_MIN      0x1000
@@ -844,15 +845,45 @@ static void pci_bios_map_devices(struct pci_bus *busses, struct pci_mem *mem)
     }
 }

+static
+int pci_mem_get(struct pci_mem *mem)
+{
+    int ret;
+    struct romfile_s *file = romfile_find("etc/pci-info");
+    if (!file)
+        return -1;
+
+    ret = file->copy(file, mem, sizeof(*mem));
+    if (ret < (int)sizeof(*mem)) {
+        dprintf(1, "failed to read %d bytes from etc/pci-info: result %d\n",
+                (int)sizeof *mem, ret);
+        return -1;
+    }
+
+    mem->start32 = le64_to_cpu(mem->start32);
+    mem->end32 = le64_to_cpu(mem->end32);
+    mem->start64 = le64_to_cpu(mem->start64);
+    mem->end64 = le64_to_cpu(mem->end64);
+
+    dprintf(3, "loaded pci setup from etc/pci-info: %llx-%llx %llx-%llx\n",
+            (long long)mem->start32,
+            (long long)mem->end32,
+            (long long)mem->start64,
+            (long long)mem->end64
+           );
+
+    return 0;
+}
+

/****************************************************************
  * Main setup code
  ****************************************************************/
-
 void
 pci_setup(void)
 {
     struct pci_mem mem;
+    int pcimem_is_set;

if (!CONFIG_QEMU)
         return;
@@ -868,7 +899,13 @@ pci_setup(void)
     dprintf(1, "=== PCI device probing ===\n");
     pci_probe_devices();

-    pci_bios_init_mem_addr(&mem);
+    if (pci_mem_get(&mem) < 0) {
+        pci_bios_init_mem_addr(&mem);
+        pcimem_is_set = 0;
+    } else {
+        pcimem_is_set = 1;
+    }
+
     pci_bios_init_platform();

dprintf(1, "=== PCI new allocation pass #1 ===\n");
@@ -882,11 +919,20 @@ pci_setup(void)
         return;

dprintf(1, "=== PCI new allocation pass #2 ===\n");
+    if (pcimem_is_set) {
+        pcimem_start = mem.start32;
+        pcimem_end = mem.end32;
+        pcimem64_start = mem.start64;
+        pcimem64_end = mem.end64;
+    }
     pci_bios_map_devices(busses, &mem);
-    pcimem_start = mem.start32;
-    pcimem_end = mem.end32;
-    pcimem64_start = mem.start64;
-    pcimem64_end = mem.end64;
+    /* If memory window is not yet set, set it to result of bus scan */
+    if (!pcimem_is_set) {
+        pcimem_start = mem.start32;
+        pcimem_end = mem.end32;
+        pcimem64_start = mem.start64;
+        pcimem64_end = mem.end64;
+    }

pci_bios_init_devices();

[Igor Mammedov]

On Mon, 29 Jul 2013 12:33:44 +0300
"Michael S. Tsirkin"  wrote:
> This is a refactoring of patch
> PATCH v2: pci: load memory window setup from host.
>
> Please note qemu merged the required interface already.
> Please review, and consider for release.
>
> Changes from v3:
> fix bug causing windows to crash
> better debug output (when enabled)
>
> Changes from v2:
> address comments by Kevin:
> avoid dynamic memory allocation
> refactor code to avoid special-casing loaded windows
> split out to two patch series
>
> Changes from v1:
>         - fix bug in 64 bit range check
>         - address Kevin's comments:
>                 move file load into pciinit.c
>                 dont reorder initialization
>                 sizeof style fix
>
> Michael S. Tsirkin (2):
>   pciinit: refactor mem init code
>   pci: load memory window setup from host
>
> src/pciinit.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 121 insertions(+), 24 deletions(-)

Install and boot tested with i440fx machine:
 * Windows 7 ultimate x32
 * Windows server 2008 r2 x64
 * Windows Vista x32
 * Windows server 2012 x64

Following OSes install/boot with 64-bit PCI hole hidden using
 -global i440FX-pcihost.pci-hole64-size=0

* Windows server 2003 x32|x64
 * Windows server 2003 R2 x32|x64
 * Windows XP x32|x64

[Michael S. Tsirkin]

On Mon, Jul 29, 2013 at 12:33:44PM +0300, Michael S. Tsirkin wrote:
> This is a refactoring of patch
> PATCH v2: pci: load memory window setup from host.
>
> Please note qemu merged the required interface already.
> Please review, and consider for release.
>
> Changes from v3:
> fix bug causing windows to crash
> better debug output (when enabled)
>
> Changes from v2:
> address comments by Kevin:
> avoid dynamic memory allocation
> refactor code to avoid special-casing loaded windows
> split out to two patch series
>
> Changes from v1:
>         - fix bug in 64 bit range check
>         - address Kevin's comments:
>                 move file load into pciinit.c
>                 dont reorder initialization
>                 sizeof style fix
>
> Michael S. Tsirkin (2):
>   pciinit: refactor mem init code
>   pci: load memory window setup from host
>
> src/pciinit.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 121 insertions(+), 24 deletions(-)

Kevin, could you please comment on this patchset?

I have some enhancement ideas like disabling devices selectively instead
of a panic if we can't allocate memory, but it will be easier to build
it on top.
> SeaBIOS mailing list
> [e-mail]
> http://www.seabios.org/mailman/listinfo/seabios