[Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default

[Igor Mammedov]

It turns out that some 32 bit windows guests crash
if 64 bit PCI hole size is >2G.
Limit it to 2G for piix and q35 by default.
User may override default boundaries by using
"pci_hole64_end " property.

Examples:
-global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff

-global q35-pcihost.pci_hole64_end=0xffffffffffffffff

Reported-by: Igor Mammedov <address@hidden>,
Signed-off-by: Michael S. Tsirkin <address@hidden>
Signed-off-by: Igor Mammedov <address@hidden>
---
 hw/i386/pc.c              | 59 +++++++++++++++++++++++++++++------------------
 hw/i386/pc_piix.c         | 14 +----------
 hw/pci-host/piix.c        | 37 +++++++++++++++++++++++++----
 hw/pci-host/q35.c         | 32 +++++++++++++++----------
 include/hw/i386/pc.h      |  5 ++--
 include/hw/pci-host/q35.h |  1 +
 6 files changed, 94 insertions(+), 54 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b0b98a8..acaeb6c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
 static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
 {
     PcRomPciInfo *info;
+    Object *pci_info;
+
     if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
         return;
     }
+    pci_info = object_resolve_path("/machine/i440fx", NULL);
+    if (!pci_info) {
+        pci_info = object_resolve_path("/machine/q35", NULL);
+        if (!pci_info) {
+            return;
+        }
+    }
 
     info = g_malloc(sizeof *info);
-    info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
-    info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
-    info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
-    info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
+    info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
+                                "pci_hole_start", NULL));
+    info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
+                                "pci_hole_end", NULL));
+    info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
+                                "pci_hole64_start", NULL));
+    info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
+                                "pci_hole64_end", NULL));
     /* Pass PCI hole info to guest via a side channel.
      * Required so guest PCI enumeration does the right thing. */
     fw_cfg_add_file(guest_info->fw_cfg, "etc/pci-info", info, sizeof *info);
@@ -1037,29 +1050,31 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t 
below_4g_mem_size,
     PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
     PcGuestInfo *guest_info = &guest_info_state->info;
 
-    guest_info->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
-    if (sizeof(hwaddr) == 4) {
-        guest_info->pci_info.w64.begin = 0;
-        guest_info->pci_info.w64.end = 0;
-    } else {
-        /*
-         * BIOS does not set MTRR entries for the 64 bit window, so no need to
-         * align address to power of two.  Align address at 1G, this makes sure
-         * it can be exactly covered with a PAT entry even when using huge
-         * pages.
-         */
-        guest_info->pci_info.w64.begin =
-            ROUND_UP((0x1ULL << 32) + above_4g_mem_size, 0x1ULL << 30);
-        guest_info->pci_info.w64.end = guest_info->pci_info.w64.begin +
-            (0x1ULL << 62);
-        assert(guest_info->pci_info.w64.begin <= guest_info->pci_info.w64.end);
-    }
-
     guest_info_state->machine_done.notify = pc_guest_info_machine_done;
     qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
     return guest_info;
 }
 
+void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start)
+{
+    if (sizeof(hwaddr) == 4) {
+        memset(&pci_info->w64, sizeof(pci_info->w64), 0);
+        return;
+    }
+    /*
+     * BIOS does not set MTRR entries for the 64 bit window, so no need to
+     * align address to power of two.  Align address at 1G, this makes sure
+     * it can be exactly covered with a PAT entry even when using huge
+     * pages.
+     */
+    pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
+    if (!pci_info->w64.end) {
+        /* set default end if it wasn't specified, + 2 Gb past start */
+        pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31);
+    }
+    assert(pci_info->w64.begin < pci_info->w64.end);
+}
+
 void pc_acpi_init(const char *default_dsdt)
 {
     char *filename;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b58c255..ab25458 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -129,15 +129,6 @@ static void pc_init1(MemoryRegion *system_memory,
     guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
     guest_info->has_pci_info = has_pci_info;
 
-    /* Set PCI window size the way seabios has always done it. */
-    /* Power of 2 so bios can cover it with a single MTRR */
-    if (ram_size <= 0x80000000)
-        guest_info->pci_info.w32.begin = 0x80000000;
-    else if (ram_size <= 0xc0000000)
-        guest_info->pci_info.w32.begin = 0xc0000000;
-    else
-        guest_info->pci_info.w32.begin = 0xe0000000;
-
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
         fw_cfg = pc_memory_init(system_memory,
@@ -160,10 +151,7 @@ static void pc_init1(MemoryRegion *system_memory,
                               system_memory, system_io, ram_size,
                               below_4g_mem_size,
                               0x100000000ULL - below_4g_mem_size,
-                              0x100000000ULL + above_4g_mem_size,
-                              (sizeof(hwaddr) == 4
-                               ? 0
-                               : ((uint64_t)1 << 62)),
+                              above_4g_mem_size,
                               pci_memory, ram_memory);
     } else {
         pci_bus = NULL;
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 4624d04..59a42c5 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -32,6 +32,7 @@
 #include "hw/xen/xen.h"
 #include "hw/pci-host/pam.h"
 #include "sysemu/sysemu.h"
+#include "hw/i386/ioapic.h"
 
 /*
  * I440FX chipset data sheet.
@@ -44,6 +45,7 @@
 
 typedef struct I440FXState {
     PCIHostState parent_obj;
+    PcPciInfo pci_info;
 } I440FXState;
 
 #define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
@@ -247,8 +249,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                     ram_addr_t ram_size,
                     hwaddr pci_hole_start,
                     hwaddr pci_hole_size,
-                    hwaddr pci_hole64_start,
-                    hwaddr pci_hole64_size,
+                    ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_address_space,
                     MemoryRegion *ram_memory)
 {
@@ -259,6 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     PIIX3State *piix3;
     PCII440FXState *f;
     unsigned i;
+    I440FXState *i440fx;
+    uint64_t pci_hole64_size;
 
     dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
     s = PCI_HOST_BRIDGE(dev);
@@ -274,14 +277,30 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     f->system_memory = address_space_mem;
     f->pci_address_space = pci_address_space;
     f->ram_memory = ram_memory;
+
+    i440fx = I440FX_PCI_HOST(dev);
+    /* Set PCI window size the way seabios has always done it. */
+    /* Power of 2 so bios can cover it with a single MTRR */
+    if (ram_size <= 0x80000000) {
+        i440fx->pci_info.w32.begin = 0x80000000;
+    } else if (ram_size <= 0xc0000000) {
+        i440fx->pci_info.w32.begin = 0xc0000000;
+    } else {
+        i440fx->pci_info.w32.begin = 0xe0000000;
+    }
+
     memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", 
f->pci_address_space,
                              pci_hole_start, pci_hole_size);
     memory_region_add_subregion(f->system_memory, pci_hole_start, 
&f->pci_hole);
+
+    pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size);
+    pci_hole64_size = range_size(i440fx->pci_info.w64);
     memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
                              f->pci_address_space,
-                             pci_hole64_start, pci_hole64_size);
+                             i440fx->pci_info.w64.begin, pci_hole64_size);
     if (pci_hole64_size) {
-        memory_region_add_subregion(f->system_memory, pci_hole64_start,
+        memory_region_add_subregion(f->system_memory,
+                                    i440fx->pci_info.w64.begin,
                                     &f->pci_hole_64bit);
     }
     memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
@@ -629,6 +648,15 @@ static const char 
*i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
     return "0000";
 }
 
+static Property i440fx_props[] = {
+    DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
+    DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
+    DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
+    DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
+                       IO_APIC_DEFAULT_ADDRESS),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -638,6 +666,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, 
void *data)
     dc->realize = i440fx_pcihost_realize;
     dc->fw_name = "pci";
     dc->no_user = 1;
+    dc->props = i440fx_props;
 }
 
 static const TypeInfo i440fx_pcihost_info = {
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 6b1b3b7..a6936e6 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState 
*host_bridge,
 static Property mch_props[] = {
     DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
+    DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost,
+                       mch.pci_info.w64.begin, 0),
+    DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost,
+                       mch.pci_info.w64.end, 0),
+    /* Leave enough space for the biggest MCFG BAR */
+    /* TODO: this matches current bios behaviour, but
+     * it's not a power of two, which means an MTRR
+     * can't cover it exactly.
+     */
+    DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost,
+                       mch.pci_info.w32.begin,
+                       MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
+                       MCH_HOST_BRIDGE_PCIEXBAR_MAX),
+    DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost,
+                       mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d)
     hwaddr pci_hole64_size;
     MCHPCIState *mch = MCH_PCI_DEVICE(d);
 
-    /* Leave enough space for the biggest MCFG BAR */
-    /* TODO: this matches current bios behaviour, but
-     * it's not a power of two, which means an MTRR
-     * can't cover it exactly.
-     */
-    mch->guest_info->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
-        MCH_HOST_BRIDGE_PCIEXBAR_MAX;
-
     /* setup pci memory regions */
     memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
                              mch->pci_address_space,
@@ -270,15 +277,16 @@ static int mch_init(PCIDevice *d)
                              0x100000000ULL - mch->below_4g_mem_size);
     memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
                                 &mch->pci_hole);
-    pci_hole64_size = (sizeof(hwaddr) == 4 ? 0 :
-                       ((uint64_t)1 << 62));
+
+    pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + 
mch->above_4g_mem_size);
+    pci_hole64_size = range_size(mch->pci_info.w64);
     memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
                              mch->pci_address_space,
-                             0x100000000ULL + mch->above_4g_mem_size,
+                             mch->pci_info.w64.begin,
                              pci_hole64_size);
     if (pci_hole64_size) {
         memory_region_add_subregion(mch->system_memory,
-                                    0x100000000ULL + mch->above_4g_mem_size,
+                                    mch->pci_info.w64.begin,
                                     &mch->pci_hole_64bit);
     }
     /* smram */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 7fb97b0..ab747b7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -18,7 +18,6 @@ typedef struct PcPciInfo {
 } PcPciInfo;
 
 struct PcGuestInfo {
-    PcPciInfo pci_info;
     bool has_pci_info;
     FWCfgState *fw_cfg;
 };
@@ -100,6 +99,7 @@ void pc_acpi_init(const char *default_dsdt);
 
 PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
                                 ram_addr_t above_4g_mem_size);
+void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start);
 
 FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                            const char *kernel_filename,
@@ -150,8 +150,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int 
*piix_devfn,
                     ram_addr_t ram_size,
                     hwaddr pci_hole_start,
                     hwaddr pci_hole_size,
-                    hwaddr pci_hole64_start,
-                    hwaddr pci_hole64_size,
+                    ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
 
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 3cb631e..3a9e04b 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -55,6 +55,7 @@ typedef struct MCHPCIState {
     MemoryRegion smram_region;
     MemoryRegion pci_hole;
     MemoryRegion pci_hole_64bit;
+    PcPciInfo pci_info;
     uint8_t smm_enabled;
     ram_addr_t below_4g_mem_size;
     ram_addr_t above_4g_mem_size;
-- 
1.8.3.1