[Qemu-devel] [PATCH for-1.6 v3 0/7] pc: limit 64 bit hole to 2G by default

[Qemu-devel] [PATCH for-1.6 v3 0/7] 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.

v1-v2:
 * redone using QOM properties to pass value around
v2-v3:
 * make pci_hole[64]-(start|end) properties read-only
 * add pci-hole64-size property to manage hole size
 
git-tree for testing:
https://github.com/imammedo/qemu/commits/pcihole64_fix_v3

Igor Mammedov (4):
  pc: add I440FX QOM cast macro
  pc: replace i440fx_common_init() with i440fx_init()
  pc: add Q35 to QOM composition tree under /machine
  pc: limit 64 bit hole to 2G by default

Michael S. Tsirkin (1):
  pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h

Vasilis Liaskovitis (2):
  qapi: make visit_type_size fallback to type_int
  qdev: Add SIZE type to qdev properties

 hw/core/qdev-properties.c    |   55 +++++++++++++++
 hw/i386/pc.c                 |   58 ++++++++++-------
 hw/i386/pc_piix.c            |   14 +----
 hw/i386/pc_q35.c             |    1 +
 hw/pci-host/piix.c           |  149 ++++++++++++++++++++++++++++++------------
 hw/pci-host/q35.c            |   88 ++++++++++++++++++++----
 include/hw/i386/ioapic.h     |    1 +
 include/hw/i386/pc.h         |   14 +++-
 include/hw/pci-host/q35.h    |    2 +
 include/hw/qdev-properties.h |    3 +
 include/qemu/option.h        |    2 +
 qapi/qapi-visit-core.c       |   11 +++-
 util/qemu-option.c           |    4 +-
 13 files changed, 302 insertions(+), 100 deletions(-)

[Qemu-devel] [PATCH 1/7] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h

[Igor Mammedov]

From: Michael S. Tsirkin <mst@redhat.com>

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 hw/i386/pc.c             |    2 --
 include/hw/i386/ioapic.h |    1 +
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2a87563..b0b98a8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -75,8 +75,6 @@
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
-#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
-
 #define E820_NR_ENTRIES		16
 
 struct e820_entry {
diff --git a/include/hw/i386/ioapic.h b/include/hw/i386/ioapic.h
index 86e63da..6245388 100644
--- a/include/hw/i386/ioapic.h
+++ b/include/hw/i386/ioapic.h
@@ -21,6 +21,7 @@
 #define HW_IOAPIC_H
 
 #define IOAPIC_NUM_PINS 24
+#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
 
 void ioapic_eoi_broadcast(int vector);
 
-- 
1.7.1

[Qemu-devel] [PATCH 2/7] pc: add I440FX QOM cast macro

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * add _BRIDGE suffix to type and cast macros
---
 hw/pci-host/piix.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 3908860..39e9c55 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -38,6 +38,10 @@
  * http://download.intel.com/design/chipsets/datashts/29054901.pdf
  */
 
+#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
+#define I440FX_PCI_HOST_BRIDGE(obj) \
+    OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
+
 typedef struct I440FXState {
     PCIHostState parent_obj;
 } I440FXState;
@@ -257,7 +261,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
     PCII440FXState *f;
     unsigned i;
 
-    dev = qdev_create(NULL, "i440FX-pcihost");
+    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
     s = PCI_HOST_BRIDGE(dev);
     b = pci_bus_new(dev, NULL, pci_address_space,
                     address_space_io, 0, TYPE_PCI_BUS);
@@ -661,7 +665,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo i440fx_pcihost_info = {
-    .name          = "i440FX-pcihost",
+    .name          = TYPE_I440FX_PCI_HOST_BRIDGE,
     .parent        = TYPE_PCI_HOST_BRIDGE,
     .instance_size = sizeof(I440FXState),
     .instance_init = i440fx_pcihost_initfn,
-- 
1.7.1

[Qemu-devel] [PATCH 3/7] pc: replace i440fx_common_init() with i440fx_init()

[Igor Mammedov]

It isn't used anywhere else.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 hw/pci-host/piix.c |   50 +++++++++++++-------------------------------------
 1 files changed, 13 insertions(+), 37 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 39e9c55..3f539ec 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -239,19 +239,18 @@ static int i440fx_initfn(PCIDevice *dev)
     return 0;
 }
 
-static PCIBus *i440fx_common_init(const char *device_name,
-                                  PCII440FXState **pi440fx_state,
-                                  int *piix3_devfn,
-                                  ISABus **isa_bus, qemu_irq *pic,
-                                  MemoryRegion *address_space_mem,
-                                  MemoryRegion *address_space_io,
-                                  ram_addr_t ram_size,
-                                  hwaddr pci_hole_start,
-                                  hwaddr pci_hole_size,
-                                  hwaddr pci_hole64_start,
-                                  hwaddr pci_hole64_size,
-                                  MemoryRegion *pci_address_space,
-                                  MemoryRegion *ram_memory)
+PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
+                    int *piix3_devfn,
+                    ISABus **isa_bus, qemu_irq *pic,
+                    MemoryRegion *address_space_mem,
+                    MemoryRegion *address_space_io,
+                    ram_addr_t ram_size,
+                    hwaddr pci_hole_start,
+                    hwaddr pci_hole_size,
+                    hwaddr pci_hole64_start,
+                    hwaddr pci_hole64_size,
+                    MemoryRegion *pci_address_space,
+                    MemoryRegion *ram_memory)
 {
     DeviceState *dev;
     PCIBus *b;
@@ -269,7 +268,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
     object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
     qdev_init_nofail(dev);
 
-    d = pci_create_simple(b, 0, device_name);
+    d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE);
     *pi440fx_state = I440FX_PCI_DEVICE(d);
     f = *pi440fx_state;
     f->system_memory = address_space_mem;
@@ -330,29 +329,6 @@ static PCIBus *i440fx_common_init(const char *device_name,
     return b;
 }
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
-                    ISABus **isa_bus, qemu_irq *pic,
-                    MemoryRegion *address_space_mem,
-                    MemoryRegion *address_space_io,
-                    ram_addr_t ram_size,
-                    hwaddr pci_hole_start,
-                    hwaddr pci_hole_size,
-                    hwaddr pci_hole64_start,
-                    hwaddr pci_hole64_size,
-                    MemoryRegion *pci_memory, MemoryRegion *ram_memory)
-
-{
-    PCIBus *b;
-
-    b = i440fx_common_init(TYPE_I440FX_PCI_DEVICE, pi440fx_state,
-                           piix3_devfn, isa_bus, pic,
-                           address_space_mem, address_space_io, ram_size,
-                           pci_hole_start, pci_hole_size,
-                           pci_hole64_start, pci_hole64_size,
-                           pci_memory, ram_memory);
-    return b;
-}
-
 /* PIIX3 PCI to ISA bridge */
 static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
 {
-- 
1.7.1

[Qemu-devel] [PATCH 4/7] pc: add Q35 to QOM composition tree under /machine

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 hw/i386/pc_q35.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0b1d2e3..2f35d12 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -131,6 +131,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     /* create pci host bus */
     q35_host = Q35_HOST_DEVICE(qdev_create(NULL, TYPE_Q35_HOST_DEVICE));
 
+    object_property_add_child(qdev_get_machine(), "q35", OBJECT(q35_host), NULL);
     q35_host->mch.ram_memory = ram_memory;
     q35_host->mch.pci_address_space = pci_memory;
     q35_host->mch.system_memory = get_system_memory();
-- 
1.7.1

[Qemu-devel] [PATCH 5/7] qapi: make visit_type_size fallback to type_int

[Igor Mammedov]

From: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>

Currently visit_type_size checks if the visitor's type_size function pointer is
NULL. If not, it calls it, otherwise it calls v->type_uint64(). But neither of
these pointers are ever set. Fallback to calling v->type_int() in this third
(default) case.

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qapi/qapi-visit-core.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 401ee6e..fcacaff 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -238,8 +238,17 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
 
 void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 {
+    int64_t value;
     if (!error_is_set(errp)) {
-        (v->type_size ? v->type_size : v->type_uint64)(v, obj, name, errp);
+        if (v->type_size) {
+            v->type_size(v, obj, name, errp);
+        } else if (v->type_uint64) {
+            v->type_uint64(v, obj, name, errp);
+        } else {
+            value = *obj;
+            v->type_int(v, &value, name, errp);
+            *obj = value;
+        }
     }
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 6/7] qdev: Add SIZE type to qdev properties

[Igor Mammedov]

From: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>

This patch adds a 'SIZE' type property to qdev.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
 *  ident fixes
---
 hw/core/qdev-properties.c    |   55 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-properties.h |    3 ++
 include/qemu/option.h        |    2 +
 util/qemu-option.c           |    4 +-
 4 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 3a324fb..26ad046 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1134,3 +1134,58 @@ void qdev_prop_set_globals(DeviceState *dev, Error **errp)
         class = object_class_get_parent(class);
     } while (class);
 }
+
+/* --- 64bit unsigned int 'size' type --- */
+
+static void get_size(Object *obj, Visitor *v, void *opaque,
+                     const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+    visit_type_size(v, ptr, name, errp);
+}
+
+static void set_size(Object *obj, Visitor *v, void *opaque,
+                     const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+    visit_type_size(v, ptr, name, errp);
+}
+
+static int parse_size(DeviceState *dev, Property *prop, const char *str)
+{
+    uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
+    Error *errp = NULL;
+
+    if (str != NULL) {
+        parse_option_size(prop->name, str, ptr, &errp);
+    }
+    assert_no_error(errp);
+    return 0;
+}
+
+static int print_size(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
+    char suffixes[] = {'T', 'G', 'M', 'K', 'B'};
+    int i = 0;
+    uint64_t div;
+
+    for (div = (long int)1 << 40; !(*ptr / div) ; div >>= 10) {
+        i++;
+    }
+    return snprintf(dest, len, "%0.03f%c", (double)*ptr/div, suffixes[i]);
+}
+
+PropertyInfo qdev_prop_size = {
+    .name  = "size",
+    .parse = parse_size,
+    .print = print_size,
+    .get = get_size,
+    .set = set_size,
+};
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 39448b7..692f82e 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -15,6 +15,7 @@ extern PropertyInfo qdev_prop_uint64;
 extern PropertyInfo qdev_prop_hex8;
 extern PropertyInfo qdev_prop_hex32;
 extern PropertyInfo qdev_prop_hex64;
+extern PropertyInfo qdev_prop_size;
 extern PropertyInfo qdev_prop_string;
 extern PropertyInfo qdev_prop_chr;
 extern PropertyInfo qdev_prop_ptr;
@@ -116,6 +117,8 @@ extern PropertyInfo qdev_prop_arraylen;
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex32, uint32_t)
 #define DEFINE_PROP_HEX64(_n, _s, _f, _d)                       \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex64, uint64_t)
+#define DEFINE_PROP_SIZE(_n, _s, _f, _d)                       \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_size, uint64_t)
 #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
 
diff --git a/include/qemu/option.h b/include/qemu/option.h
index a83c700..39b8fe0 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -73,6 +73,8 @@ QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
     QEMUOptionParameter *list);
 QEMUOptionParameter *parse_option_parameters(const char *param,
     QEMUOptionParameter *list, QEMUOptionParameter *dest);
+void parse_option_size(const char *name, const char *value,
+                       uint64_t *ret, Error **errp);
 void free_option_parameters(QEMUOptionParameter *list);
 void print_option_parameters(QEMUOptionParameter *list);
 void print_option_help(QEMUOptionParameter *list);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index e0ef426..98c37f0 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -173,8 +173,8 @@ static void parse_option_number(const char *name, const char *value,
     }
 }
 
-static void parse_option_size(const char *name, const char *value,
-                              uint64_t *ret, Error **errp)
+void parse_option_size(const char *name, const char *value,
+                       uint64_t *ret, Error **errp)
 {
     char *postfix;
     double sizef;
-- 
1.7.1

[Qemu-devel] [PATCH 7/7] 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 64-bit PCI hole size by
using "pci-hole64-size" property.

Examples:
-global i440FX-pcihost.pci-hole64-size=4G

-global q35-pcihost.pci-hole64-size=4G

Reported-by: Igor Mammedov <imammedo@redhat.com>,
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * make pcihole[64]-(start|end) properties read-only
  * add user manageable pci-hole64-size property
  * use object_resolve_path_type(TYPE_PCI_HOST_BRIDGE)
    to avoid hard codding paths for each supported
    chipset
  * use macros for property names to avoid hadr-codded
    names duplication
---
 hw/i386/pc.c              |   56 ++++++++++++++++----------
 hw/i386/pc_piix.c         |   14 +------
 hw/pci-host/piix.c        |   95 ++++++++++++++++++++++++++++++++++++++++++--
 hw/pci-host/q35.c         |   88 ++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h      |   14 +++++-
 include/hw/pci-host/q35.h |    2 +
 6 files changed, 211 insertions(+), 58 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b0b98a8..a2b9d88 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -55,6 +55,7 @@
 #include "hw/acpi/acpi.h"
 #include "hw/cpu/icc_bus.h"
 #include "hw/boards.h"
+#include "hw/pci/pci_host.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -1003,15 +1004,27 @@ typedef struct PcRomPciInfo {
 static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
 {
     PcRomPciInfo *info;
+    Object *pci_info;
+    bool ambiguous = false;
+
     if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
         return;
     }
+    pci_info = object_resolve_path_type("", TYPE_PCI_HOST_BRIDGE, &ambiguous);
+    g_assert(!ambiguous);
+    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_HOST_PROP_PCI_HOLE_START, NULL));
+    info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
+                                PCI_HOST_PROP_PCI_HOLE_END, NULL));
+    info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
+                                PCI_HOST_PROP_PCI_HOLE64_START, NULL));
+    info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
+                                PCI_HOST_PROP_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,28 @@ 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,
+                        uint64_t pci_hole64_size)
+{
+    if ((sizeof(hwaddr) == 4) || (!pci_hole64_size)) {
+        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);
+    pci_info->w64.end = pci_info->w64.begin + pci_hole64_size;
+    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 3f539ec..dc1718f 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -32,6 +32,8 @@
 #include "hw/xen/xen.h"
 #include "hw/pci-host/pam.h"
 #include "sysemu/sysemu.h"
+#include "hw/i386/ioapic.h"
+#include "qapi/visitor.h"
 
 /*
  * I440FX chipset data sheet.
@@ -44,6 +46,8 @@
 
 typedef struct I440FXState {
     PCIHostState parent_obj;
+    PcPciInfo pci_info;
+    uint64_t pci_hole64_size;
 } I440FXState;
 
 #define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
@@ -207,14 +211,71 @@ static const VMStateDescription vmstate_i440fx = {
     }
 };
 
+static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
+                                              void *opaque, const char *name,
+                                              Error **errp)
+{
+    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
+    uint32_t value = s->pci_info.w32.begin;
+
+    visit_type_uint32(v, &value, name, errp);
+}
+
+static void i440fx_pcihost_get_pci_hole_end(Object *obj, Visitor *v,
+                                            void *opaque, const char *name,
+                                            Error **errp)
+{
+    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
+    uint32_t value = s->pci_info.w32.end;
+
+    visit_type_uint32(v, &value, name, errp);
+}
+
+static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
+                                                void *opaque, const char *name,
+                                                Error **errp)
+{
+    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
+
+    visit_type_uint64(v, &s->pci_info.w64.begin, name, errp);
+}
+
+static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
+                                              void *opaque, const char *name,
+                                              Error **errp)
+{
+    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
+
+    visit_type_uint64(v, &s->pci_info.w64.end, name, errp);
+}
+
 static void i440fx_pcihost_initfn(Object *obj)
 {
     PCIHostState *s = PCI_HOST_BRIDGE(obj);
+    I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
 
     memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
                           "pci-conf-idx", 4);
     memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
                           "pci-conf-data", 4);
+
+    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
+                        i440fx_pcihost_get_pci_hole_start,
+                        NULL, NULL, NULL, NULL);
+
+    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "int",
+                        i440fx_pcihost_get_pci_hole_end,
+                        NULL, NULL, NULL, NULL);
+
+    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "int",
+                        i440fx_pcihost_get_pci_hole64_start,
+                        NULL, NULL, NULL, NULL);
+
+    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
+                        i440fx_pcihost_get_pci_hole64_end,
+                        NULL, NULL, NULL, NULL);
+
+    d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
 static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
@@ -247,8 +308,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 +319,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     PIIX3State *piix3;
     PCII440FXState *f;
     unsigned i;
+    I440FXState *i440fx;
 
     dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
     s = PCI_HOST_BRIDGE(dev);
@@ -274,14 +335,31 @@ 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_BRIDGE(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,
+                       i440fx->pci_hole64_size);
     memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
                              f->pci_address_space,
-                             pci_hole64_start, pci_hole64_size);
-    if (pci_hole64_size) {
-        memory_region_add_subregion(f->system_memory, pci_hole64_start,
+                             i440fx->pci_info.w64.begin,
+                             i440fx->pci_hole64_size);
+    if (i440fx->pci_hole64_size) {
+        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 +707,12 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
     return "0000";
 }
 
+static Property i440fx_props[] = {
+    DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, I440FXState,
+                     pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -638,6 +722,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..6ae432b 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -29,6 +29,7 @@
  */
 #include "hw/hw.h"
 #include "hw/pci-host/q35.h"
+#include "qapi/visitor.h"
 
 /****************************************************************************
  * Q35 host
@@ -64,9 +65,49 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
     return "0000";
 }
 
+static void q35_host_get_pci_hole_start(Object *obj, Visitor *v,
+                                        void *opaque, const char *name,
+                                        Error **errp)
+{
+    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
+    uint32_t value = s->mch.pci_info.w32.begin;
+
+    visit_type_uint32(v, &value, name, errp);
+}
+
+static void q35_host_get_pci_hole_end(Object *obj, Visitor *v,
+                                      void *opaque, const char *name,
+                                      Error **errp)
+{
+    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
+    uint32_t value = s->mch.pci_info.w32.end;
+
+    visit_type_uint32(v, &value, name, errp);
+}
+
+static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
+                                          void *opaque, const char *name,
+                                          Error **errp)
+{
+    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
+
+    visit_type_uint64(v, &s->mch.pci_info.w64.begin, name, errp);
+}
+
+static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
+                                        void *opaque, const char *name,
+                                        Error **errp)
+{
+    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
+
+    visit_type_uint64(v, &s->mch.pci_info.w64.end, name, errp);
+}
+
 static Property mch_props[] = {
     DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
+    DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
+                     mch.pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -95,6 +136,31 @@ static void q35_host_initfn(Object *obj)
     object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
     qdev_prop_set_uint32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
     qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
+
+    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
+                        q35_host_get_pci_hole_start,
+                        NULL, NULL, NULL, NULL);
+
+    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "int",
+                        q35_host_get_pci_hole_end,
+                        NULL, NULL, NULL, NULL);
+
+    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "int",
+                        q35_host_get_pci_hole64_start,
+                        NULL, NULL, NULL, NULL);
+
+    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
+                        q35_host_get_pci_hole64_end,
+                        NULL, NULL, NULL, NULL);
+
+    /* 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.
+     */
+    s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
+        MCH_HOST_BRIDGE_PCIEXBAR_MAX;
+    s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
 static const TypeInfo q35_host_info = {
@@ -252,17 +318,8 @@ static void mch_reset(DeviceState *qdev)
 static int mch_init(PCIDevice *d)
 {
     int i;
-    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 +327,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,
+                       mch->pci_hole64_size);
     memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
                              mch->pci_address_space,
-                             0x100000000ULL + mch->above_4g_mem_size,
-                             pci_hole64_size);
-    if (pci_hole64_size) {
+                             mch->pci_info.w64.begin,
+                             mch->pci_hole64_size);
+    if (mch->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..e375b4f 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;
 };
@@ -101,6 +100,16 @@ 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);
 
+#define PCI_HOST_PROP_PCI_HOLE_START   "pci-hole-start"
+#define PCI_HOST_PROP_PCI_HOLE_END     "pci-hole-end"
+#define PCI_HOST_PROP_PCI_HOLE64_START "pci-hole64-start"
+#define PCI_HOST_PROP_PCI_HOLE64_END   "pci-hole64-end"
+#define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
+#define DEFAULT_PCI_HOLE64_SIZE (1ULL << 31)
+
+void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start,
+                        uint64_t pci_hole64_size);
+
 FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                            const char *kernel_filename,
                            const char *kernel_cmdline,
@@ -150,8 +159,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..6eb7ab6 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -55,9 +55,11 @@ 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;
+    uint64_t pci_hole64_size;
     PcGuestInfo *guest_info;
 } MCHPCIState;
 
-- 
1.7.1

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

[Michael S. Tsirkin]

On Mon, Jul 29, 2013 at 04:47:57PM +0200, Igor Mammedov wrote:
> 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 64-bit PCI hole size by
> using "pci-hole64-size" property.
> 
> Examples:
> -global i440FX-pcihost.pci-hole64-size=4G
> 
> -global q35-pcihost.pci-hole64-size=4G
> 
> Reported-by: Igor Mammedov <imammedo@redhat.com>,
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   * make pcihole[64]-(start|end) properties read-only
>   * add user manageable pci-hole64-size property
>   * use object_resolve_path_type(TYPE_PCI_HOST_BRIDGE)
>     to avoid hard codding paths for each supported
>     chipset
>   * use macros for property names to avoid hadr-codded
>     names duplication
> ---
>  hw/i386/pc.c              |   56 ++++++++++++++++----------
>  hw/i386/pc_piix.c         |   14 +------
>  hw/pci-host/piix.c        |   95 ++++++++++++++++++++++++++++++++++++++++++--
>  hw/pci-host/q35.c         |   88 ++++++++++++++++++++++++++++++++++-------
>  include/hw/i386/pc.h      |   14 +++++-
>  include/hw/pci-host/q35.h |    2 +
>  6 files changed, 211 insertions(+), 58 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b0b98a8..a2b9d88 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -55,6 +55,7 @@
>  #include "hw/acpi/acpi.h"
>  #include "hw/cpu/icc_bus.h"
>  #include "hw/boards.h"
> +#include "hw/pci/pci_host.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -1003,15 +1004,27 @@ typedef struct PcRomPciInfo {
>  static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
>  {
>      PcRomPciInfo *info;
> +    Object *pci_info;
> +    bool ambiguous = false;
> +
>      if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
>          return;
>      }
> +    pci_info = object_resolve_path_type("", TYPE_PCI_HOST_BRIDGE, &ambiguous);
> +    g_assert(!ambiguous);
> +    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_HOST_PROP_PCI_HOLE_START, NULL));
> +    info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> +                                PCI_HOST_PROP_PCI_HOLE_END, NULL));
> +    info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
> +                                PCI_HOST_PROP_PCI_HOLE64_START, NULL));
> +    info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
> +                                PCI_HOST_PROP_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,28 @@ 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,
> +                        uint64_t pci_hole64_size)
> +{
> +    if ((sizeof(hwaddr) == 4) || (!pci_hole64_size)) {
> +        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);
> +    pci_info->w64.end = pci_info->w64.begin + pci_hole64_size;

Experimenting with various windows versions,
it seems the following values also work, and
allow more space by default:
	begin = ROUND_UP(pci_hole64_start, 0x1ULL << 32)
	size = (1 << 35)

This might be because 32 bit x86 can use PAE to address 36
bit address space.
I haven't yet checked that resources are actually assigned
though.

> +    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 3f539ec..dc1718f 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -32,6 +32,8 @@
>  #include "hw/xen/xen.h"
>  #include "hw/pci-host/pam.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/i386/ioapic.h"
> +#include "qapi/visitor.h"
>  
>  /*
>   * I440FX chipset data sheet.
> @@ -44,6 +46,8 @@
>  
>  typedef struct I440FXState {
>      PCIHostState parent_obj;
> +    PcPciInfo pci_info;
> +    uint64_t pci_hole64_size;
>  } I440FXState;
>  
>  #define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
> @@ -207,14 +211,71 @@ static const VMStateDescription vmstate_i440fx = {
>      }
>  };
>  
> +static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
> +                                              void *opaque, const char *name,
> +                                              Error **errp)
> +{
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> +    uint32_t value = s->pci_info.w32.begin;
> +
> +    visit_type_uint32(v, &value, name, errp);
> +}
> +
> +static void i440fx_pcihost_get_pci_hole_end(Object *obj, Visitor *v,
> +                                            void *opaque, const char *name,
> +                                            Error **errp)
> +{
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> +    uint32_t value = s->pci_info.w32.end;
> +
> +    visit_type_uint32(v, &value, name, errp);
> +}
> +
> +static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
> +                                                void *opaque, const char *name,
> +                                                Error **errp)
> +{
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> +
> +    visit_type_uint64(v, &s->pci_info.w64.begin, name, errp);
> +}
> +
> +static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
> +                                              void *opaque, const char *name,
> +                                              Error **errp)
> +{
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> +
> +    visit_type_uint64(v, &s->pci_info.w64.end, name, errp);
> +}
> +
>  static void i440fx_pcihost_initfn(Object *obj)
>  {
>      PCIHostState *s = PCI_HOST_BRIDGE(obj);
> +    I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
>  
>      memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
>                            "pci-conf-idx", 4);
>      memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
>                            "pci-conf-data", 4);
> +
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
> +                        i440fx_pcihost_get_pci_hole_start,
> +                        NULL, NULL, NULL, NULL);
> +
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "int",
> +                        i440fx_pcihost_get_pci_hole_end,
> +                        NULL, NULL, NULL, NULL);
> +
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "int",
> +                        i440fx_pcihost_get_pci_hole64_start,
> +                        NULL, NULL, NULL, NULL);
> +
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
> +                        i440fx_pcihost_get_pci_hole64_end,
> +                        NULL, NULL, NULL, NULL);
> +
> +    d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>  }
>  
>  static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
> @@ -247,8 +308,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 +319,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      PIIX3State *piix3;
>      PCII440FXState *f;
>      unsigned i;
> +    I440FXState *i440fx;
>  
>      dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
>      s = PCI_HOST_BRIDGE(dev);
> @@ -274,14 +335,31 @@ 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_BRIDGE(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,
> +                       i440fx->pci_hole64_size);
>      memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
>                               f->pci_address_space,
> -                             pci_hole64_start, pci_hole64_size);
> -    if (pci_hole64_size) {
> -        memory_region_add_subregion(f->system_memory, pci_hole64_start,
> +                             i440fx->pci_info.w64.begin,
> +                             i440fx->pci_hole64_size);
> +    if (i440fx->pci_hole64_size) {
> +        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 +707,12 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>      return "0000";
>  }
>  
> +static Property i440fx_props[] = {
> +    DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, I440FXState,
> +                     pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -638,6 +722,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..6ae432b 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -29,6 +29,7 @@
>   */
>  #include "hw/hw.h"
>  #include "hw/pci-host/q35.h"
> +#include "qapi/visitor.h"
>  
>  /****************************************************************************
>   * Q35 host
> @@ -64,9 +65,49 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
>      return "0000";
>  }
>  
> +static void q35_host_get_pci_hole_start(Object *obj, Visitor *v,
> +                                        void *opaque, const char *name,
> +                                        Error **errp)
> +{
> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +    uint32_t value = s->mch.pci_info.w32.begin;
> +
> +    visit_type_uint32(v, &value, name, errp);
> +}
> +
> +static void q35_host_get_pci_hole_end(Object *obj, Visitor *v,
> +                                      void *opaque, const char *name,
> +                                      Error **errp)
> +{
> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +    uint32_t value = s->mch.pci_info.w32.end;
> +
> +    visit_type_uint32(v, &value, name, errp);
> +}
> +
> +static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
> +                                          void *opaque, const char *name,
> +                                          Error **errp)
> +{
> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +
> +    visit_type_uint64(v, &s->mch.pci_info.w64.begin, name, errp);
> +}
> +
> +static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
> +                                        void *opaque, const char *name,
> +                                        Error **errp)
> +{
> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +
> +    visit_type_uint64(v, &s->mch.pci_info.w64.end, name, errp);
> +}
> +
>  static Property mch_props[] = {
>      DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
>                          MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> +    DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
> +                     mch.pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -95,6 +136,31 @@ static void q35_host_initfn(Object *obj)
>      object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
>      qdev_prop_set_uint32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
>      qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
> +
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
> +                        q35_host_get_pci_hole_start,
> +                        NULL, NULL, NULL, NULL);
> +
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "int",
> +                        q35_host_get_pci_hole_end,
> +                        NULL, NULL, NULL, NULL);
> +
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "int",
> +                        q35_host_get_pci_hole64_start,
> +                        NULL, NULL, NULL, NULL);
> +
> +    object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
> +                        q35_host_get_pci_hole64_end,
> +                        NULL, NULL, NULL, NULL);
> +
> +    /* 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.
> +     */
> +    s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> +        MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> +    s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>  }
>  
>  static const TypeInfo q35_host_info = {
> @@ -252,17 +318,8 @@ static void mch_reset(DeviceState *qdev)
>  static int mch_init(PCIDevice *d)
>  {
>      int i;
> -    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 +327,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,
> +                       mch->pci_hole64_size);
>      memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
>                               mch->pci_address_space,
> -                             0x100000000ULL + mch->above_4g_mem_size,
> -                             pci_hole64_size);
> -    if (pci_hole64_size) {
> +                             mch->pci_info.w64.begin,
> +                             mch->pci_hole64_size);
> +    if (mch->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..e375b4f 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;
>  };
> @@ -101,6 +100,16 @@ 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);
>  
> +#define PCI_HOST_PROP_PCI_HOLE_START   "pci-hole-start"
> +#define PCI_HOST_PROP_PCI_HOLE_END     "pci-hole-end"
> +#define PCI_HOST_PROP_PCI_HOLE64_START "pci-hole64-start"
> +#define PCI_HOST_PROP_PCI_HOLE64_END   "pci-hole64-end"
> +#define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
> +#define DEFAULT_PCI_HOLE64_SIZE (1ULL << 31)
> +
> +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start,
> +                        uint64_t pci_hole64_size);
> +
>  FWCfgState *pc_memory_init(MemoryRegion *system_memory,
>                             const char *kernel_filename,
>                             const char *kernel_cmdline,
> @@ -150,8 +159,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..6eb7ab6 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -55,9 +55,11 @@ 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;
> +    uint64_t pci_hole64_size;
>      PcGuestInfo *guest_info;
>  } MCHPCIState;
>  
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH for-1.6 v3 0/7] pc: limit 64 bit hole to 2G by default

[Michael S. Tsirkin]

On Mon, Jul 29, 2013 at 04:47:50PM +0200, Igor Mammedov wrote:
> 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.

For the series
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> v1-v2:
>  * redone using QOM properties to pass value around
> v2-v3:
>  * make pci_hole[64]-(start|end) properties read-only
>  * add pci-hole64-size property to manage hole size
>  
> git-tree for testing:
> https://github.com/imammedo/qemu/commits/pcihole64_fix_v3
> 
> Igor Mammedov (4):
>   pc: add I440FX QOM cast macro
>   pc: replace i440fx_common_init() with i440fx_init()
>   pc: add Q35 to QOM composition tree under /machine
>   pc: limit 64 bit hole to 2G by default
> 
> Michael S. Tsirkin (1):
>   pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h
> 
> Vasilis Liaskovitis (2):
>   qapi: make visit_type_size fallback to type_int
>   qdev: Add SIZE type to qdev properties
> 
>  hw/core/qdev-properties.c    |   55 +++++++++++++++
>  hw/i386/pc.c                 |   58 ++++++++++-------
>  hw/i386/pc_piix.c            |   14 +----
>  hw/i386/pc_q35.c             |    1 +
>  hw/pci-host/piix.c           |  149 ++++++++++++++++++++++++++++++------------
>  hw/pci-host/q35.c            |   88 ++++++++++++++++++++----
>  include/hw/i386/ioapic.h     |    1 +
>  include/hw/i386/pc.h         |   14 +++-
>  include/hw/pci-host/q35.h    |    2 +
>  include/hw/qdev-properties.h |    3 +
>  include/qemu/option.h        |    2 +
>  qapi/qapi-visit-core.c       |   11 +++-
>  util/qemu-option.c           |    4 +-
>  13 files changed, 302 insertions(+), 100 deletions(-)

Re: [Qemu-devel] [PATCH for-1.6 v3 0/7] pc: limit 64 bit hole to 2G by default

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

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

[Andreas Färber]

Am 29.07.2013 16:47, schrieb Igor Mammedov:
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b0b98a8..a2b9d88 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
[...]
> @@ -1003,15 +1004,27 @@ typedef struct PcRomPciInfo {
>  static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
>  {
>      PcRomPciInfo *info;
> +    Object *pci_info;
> +    bool ambiguous = false;
> +
>      if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
>          return;
>      }
> +    pci_info = object_resolve_path_type("", TYPE_PCI_HOST_BRIDGE, &ambiguous);
> +    g_assert(!ambiguous);
> +    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_HOST_PROP_PCI_HOLE_START, NULL));
> +    info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> +                                PCI_HOST_PROP_PCI_HOLE_END, NULL));
> +    info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
> +                                PCI_HOST_PROP_PCI_HOLE64_START, NULL));
> +    info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
> +                                PCI_HOST_PROP_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);

I wonder, if we're passing PcRomInfo out to SeaBIOS via fw_cfg,
shouldn't it be QEMU_PACKED just in case?

Not strictly related to this patch, obviously.

Regards,
Andreas

/* pci-info ROM file. Little endian format */
typedef struct PcRomPciInfo {
    uint64_t w32_min;
    uint64_t w32_max;
    uint64_t w64_min;
    uint64_t w64_max;
} PcRomPciInfo;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg