[Qemu-devel] [PATCH V3] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole

Marcel Apfelbaum posted 1 patch 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171108144809.56305-1-marcel@redhat.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
There is a newer version of this series
hw/i386/pc.c              | 18 ++++++++++++++++++
hw/pci-host/piix.c        | 13 ++++++++++++-
hw/pci-host/q35.c         | 17 +++++++++++++++--
include/hw/i386/pc.h      | 10 +++++++++-
include/hw/pci-host/q35.h |  1 +
5 files changed, 55 insertions(+), 4 deletions(-)
[Qemu-devel] [PATCH V3] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole
Posted by Marcel Apfelbaum 6 years, 4 months ago
Currently there is no MMIO range over 4G
reserved for PCI hotplug. Since the 32bit PCI hole
depends on the number of cold-plugged PCI devices
and other factors, it is very possible is too small
to hotplug PCI devices with large BARs.

Fix it by reserving 2G for I4400FX chipset
in order to comply with older Win32 Guest OSes
and 32G for Q35 chipset.

Note this is a regression since prev QEMU versions had
some range reserved for 64bit PCI hotplug.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---

V2 -> V3:
 - Addressed Gerd's and others comments and re-enabled the pci-hole64-size
   property defaulting it to 2G for I440FX and 32g for Q35.
 - Even if the new defaults of pci-hole64-size will appear in "info qtree"
   also for older machines, the property was not implemented so
   no changes will be visible to guests.

V1 -> V2:
 Addressed Igor's comments:
    - aligned the hole64 start to 1Gb
     (I think all the computations took care of it already,
      but it can't hurt)
    - Init compat props to "off" instead of "false"

Thanks,
Marcel 


hw/i386/pc.c              | 18 ++++++++++++++++++
 hw/pci-host/piix.c        | 13 ++++++++++++-
 hw/pci-host/q35.c         | 17 +++++++++++++++--
 include/hw/i386/pc.h      | 10 +++++++++-
 include/hw/pci-host/q35.h |  1 +
 5 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e11a65b545..af3177cca5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1448,6 +1448,24 @@ void pc_memory_init(PCMachineState *pcms,
     pcms->ioapic_as = &address_space_memory;
 }
 
+uint64_t pc_pci_hole64_start(void)
+{
+    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    uint64_t hole64_start = 0;
+
+    if (pcmc->has_reserved_memory && pcms->hotplug_memory.base) {
+        hole64_start = pcms->hotplug_memory.base;
+        if (!pcmc->broken_reserved_end) {
+            hole64_start += memory_region_size(&pcms->hotplug_memory.mr);
+        }
+    } else {
+        hole64_start = 0x100000000ULL + pcms->above_4g_mem_size;
+    }
+
+    return ROUND_UP(hole64_start, 1ULL << 30);
+}
+
 qemu_irq pc_allocate_cpu_irq(void)
 {
     return qemu_allocate_irq(pic_irq_request, NULL, 0);
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index a7e2256870..e033ee7df8 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -50,6 +50,7 @@ typedef struct I440FXState {
     PCIHostState parent_obj;
     Range pci_hole;
     uint64_t pci_hole64_size;
+    bool pci_hole64_fix;
     uint32_t short_root_bus;
 } I440FXState;
 
@@ -243,11 +244,15 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
                                                 void *opaque, Error **errp)
 {
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
+    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
     Range w64;
     uint64_t value;
 
     pci_bus_get_w64_range(h->bus, &w64);
     value = range_is_empty(&w64) ? 0 : range_lob(&w64);
+    if (!value && s->pci_hole64_fix) {
+        value = pc_pci_hole64_start();
+    }
     visit_type_uint64(v, name, &value, errp);
 }
 
@@ -256,11 +261,16 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
                                               Error **errp)
 {
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
+    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
+    uint64_t hole64_start = pc_pci_hole64_start();
     Range w64;
     uint64_t value;
 
     pci_bus_get_w64_range(h->bus, &w64);
     value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
+    if (s->pci_hole64_fix && value < (hole64_start + s->pci_hole64_size)) {
+        value = hole64_start + s->pci_hole64_size;
+    }
     visit_type_uint64(v, name, &value, errp);
 }
 
@@ -863,8 +873,9 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
 
 static Property i440fx_props[] = {
     DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, I440FXState,
-                     pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
+                     pci_hole64_size, 1ULL << 31),
     DEFINE_PROP_UINT32("short_root_bus", I440FXState, short_root_bus, 0),
+    DEFINE_PROP_BOOL("x-pci-hole64-fix", I440FXState, pci_hole64_fix, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index ddaa7d1b44..f95337c13a 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -104,11 +104,15 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
                                           Error **errp)
 {
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
+    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
     Range w64;
     uint64_t value;
 
     pci_bus_get_w64_range(h->bus, &w64);
     value = range_is_empty(&w64) ? 0 : range_lob(&w64);
+    if (!value && s->pci_hole64_fix) {
+        value = pc_pci_hole64_start();
+    }
     visit_type_uint64(v, name, &value, errp);
 }
 
@@ -117,11 +121,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
                                         Error **errp)
 {
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
+    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
+    uint64_t hole64_start = pc_pci_hole64_start();
     Range w64;
     uint64_t value;
 
     pci_bus_get_w64_range(h->bus, &w64);
     value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
+    if (s->pci_hole64_fix && value < (hole64_start + s->mch.pci_hole64_size)) {
+        value = hole64_start + s->mch.pci_hole64_size;
+    }
     visit_type_uint64(v, name, &value, errp);
 }
 
@@ -136,13 +145,15 @@ static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
 static Property q35_host_props[] = {
     DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, parent_obj.base_addr,
                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
+    /* The default value is overriden in q35_host_initfn */
     DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
-                     mch.pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
+                     mch.pci_hole64_size, 1ULL << 35),
     DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, mch.short_root_bus, 0),
     DEFINE_PROP_SIZE(PCI_HOST_BELOW_4G_MEM_SIZE, Q35PCIHost,
                      mch.below_4g_mem_size, 0),
     DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost,
                      mch.above_4g_mem_size, 0),
+    DEFINE_PROP_BOOL("x-pci-hole64-fix", Q35PCIHost, pci_hole64_fix, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -174,7 +185,9 @@ static void q35_host_initfn(Object *obj)
     object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
     qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
     qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
-
+    /* mch's object_initialize resets the default value, set it again */
+    qdev_prop_set_uint64(DEVICE(s), PCI_HOST_PROP_PCI_HOLE64_SIZE,
+                         1ULL << 35);
     object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
                         q35_host_get_pci_hole_start,
                         NULL, NULL, NULL, NULL);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 087d184ef5..ef438bd765 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -238,7 +238,6 @@ void pc_guest_info_init(PCMachineState *pcms);
 #define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
 #define PCI_HOST_BELOW_4G_MEM_SIZE     "below-4g-mem-size"
 #define PCI_HOST_ABOVE_4G_MEM_SIZE     "above-4g-mem-size"
-#define DEFAULT_PCI_HOLE64_SIZE (~0x0ULL)
 
 
 void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
@@ -249,6 +248,7 @@ void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *system_memory,
                     MemoryRegion *rom_memory,
                     MemoryRegion **ram_memory);
+uint64_t pc_pci_hole64_start(void);
 qemu_irq pc_allocate_cpu_irq(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
@@ -375,6 +375,14 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = TYPE_X86_CPU,\
         .property = "x-hv-max-vps",\
         .value    = "0x40",\
+    },{\
+        .driver   = "i440FX-pcihost",\
+        .property = "x-pci-hole64-fix",\
+        .value    = "off",\
+    },{\
+        .driver   = "q35-pcihost",\
+        .property = "x-pci-hole64-fix",\
+        .value    = "off",\
     },
 
 #define PC_COMPAT_2_9 \
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 58983c00b3..8f4ddde393 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -68,6 +68,7 @@ typedef struct Q35PCIHost {
     PCIExpressHost parent_obj;
     /*< public >*/
 
+    bool pci_hole64_fix;
     MCHPCIState mch;
 } Q35PCIHost;
 
-- 
2.13.5


Re: [Qemu-devel] [PATCH V3] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole
Posted by Gerd Hoffmann 6 years, 4 months ago
On Wed, Nov 08, 2017 at 04:48:09PM +0200, Marcel Apfelbaum wrote:
> Currently there is no MMIO range over 4G
> reserved for PCI hotplug. Since the 32bit PCI hole
> depends on the number of cold-plugged PCI devices
> and other factors, it is very possible is too small
> to hotplug PCI devices with large BARs.
> 
> Fix it by reserving 2G for I4400FX chipset
> in order to comply with older Win32 Guest OSes
> and 32G for Q35 chipset.
> 
> Note this is a regression since prev QEMU versions had
> some range reserved for 64bit PCI hotplug.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>


Re: [Qemu-devel] [PATCH V3] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole
Posted by Laszlo Ersek 6 years, 4 months ago
On 11/08/17 15:48, Marcel Apfelbaum wrote:
> Currently there is no MMIO range over 4G
> reserved for PCI hotplug. Since the 32bit PCI hole
> depends on the number of cold-plugged PCI devices
> and other factors, it is very possible is too small
> to hotplug PCI devices with large BARs.
> 
> Fix it by reserving 2G for I4400FX chipset
> in order to comply with older Win32 Guest OSes
> and 32G for Q35 chipset.
> 
> Note this is a regression since prev QEMU versions had
> some range reserved for 64bit PCI hotplug.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> 
> V2 -> V3:
>  - Addressed Gerd's and others comments and re-enabled the pci-hole64-size
>    property defaulting it to 2G for I440FX and 32g for Q35.
>  - Even if the new defaults of pci-hole64-size will appear in "info qtree"
>    also for older machines, the property was not implemented so
>    no changes will be visible to guests.
> 
> V1 -> V2:
>  Addressed Igor's comments:
>     - aligned the hole64 start to 1Gb
>      (I think all the computations took care of it already,
>       but it can't hurt)
>     - Init compat props to "off" instead of "false"
> 
> Thanks,
> Marcel 
> 
> 
> hw/i386/pc.c              | 18 ++++++++++++++++++
>  hw/pci-host/piix.c        | 13 ++++++++++++-
>  hw/pci-host/q35.c         | 17 +++++++++++++++--
>  include/hw/i386/pc.h      | 10 +++++++++-
>  include/hw/pci-host/q35.h |  1 +
>  5 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e11a65b545..af3177cca5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1448,6 +1448,24 @@ void pc_memory_init(PCMachineState *pcms,
>      pcms->ioapic_as = &address_space_memory;
>  }
>  
> +uint64_t pc_pci_hole64_start(void)
> +{
> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    uint64_t hole64_start = 0;
> +
> +    if (pcmc->has_reserved_memory && pcms->hotplug_memory.base) {
> +        hole64_start = pcms->hotplug_memory.base;
> +        if (!pcmc->broken_reserved_end) {
> +            hole64_start += memory_region_size(&pcms->hotplug_memory.mr);
> +        }
> +    } else {
> +        hole64_start = 0x100000000ULL + pcms->above_4g_mem_size;
> +    }
> +
> +    return ROUND_UP(hole64_start, 1ULL << 30);
> +}
> +
>  qemu_irq pc_allocate_cpu_irq(void)
>  {
>      return qemu_allocate_irq(pic_irq_request, NULL, 0);
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index a7e2256870..e033ee7df8 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -50,6 +50,7 @@ typedef struct I440FXState {
>      PCIHostState parent_obj;
>      Range pci_hole;
>      uint64_t pci_hole64_size;
> +    bool pci_hole64_fix;
>      uint32_t short_root_bus;
>  } I440FXState;
>  
> @@ -243,11 +244,15 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
>                                                  void *opaque, Error **errp)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>      Range w64;
>      uint64_t value;
>  
>      pci_bus_get_w64_range(h->bus, &w64);
>      value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    if (!value && s->pci_hole64_fix) {
> +        value = pc_pci_hole64_start();
> +    }
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> @@ -256,11 +261,16 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>                                                Error **errp)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> +    uint64_t hole64_start = pc_pci_hole64_start();
>      Range w64;
>      uint64_t value;
>  
>      pci_bus_get_w64_range(h->bus, &w64);
>      value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    if (s->pci_hole64_fix && value < (hole64_start + s->pci_hole64_size)) {
> +        value = hole64_start + s->pci_hole64_size;
> +    }
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> @@ -863,8 +873,9 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>  
>  static Property i440fx_props[] = {
>      DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, I440FXState,
> -                     pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
> +                     pci_hole64_size, 1ULL << 31),
>      DEFINE_PROP_UINT32("short_root_bus", I440FXState, short_root_bus, 0),
> +    DEFINE_PROP_BOOL("x-pci-hole64-fix", I440FXState, pci_hole64_fix, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index ddaa7d1b44..f95337c13a 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -104,11 +104,15 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
>                                            Error **errp)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>      Range w64;
>      uint64_t value;
>  
>      pci_bus_get_w64_range(h->bus, &w64);
>      value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    if (!value && s->pci_hole64_fix) {
> +        value = pc_pci_hole64_start();
> +    }
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> @@ -117,11 +121,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>                                          Error **errp)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +    uint64_t hole64_start = pc_pci_hole64_start();
>      Range w64;
>      uint64_t value;
>  
>      pci_bus_get_w64_range(h->bus, &w64);
>      value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    if (s->pci_hole64_fix && value < (hole64_start + s->mch.pci_hole64_size)) {
> +        value = hole64_start + s->mch.pci_hole64_size;
> +    }
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> @@ -136,13 +145,15 @@ static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
>  static Property q35_host_props[] = {
>      DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, parent_obj.base_addr,
>                          MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> +    /* The default value is overriden in q35_host_initfn */
>      DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
> -                     mch.pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
> +                     mch.pci_hole64_size, 1ULL << 35),
>      DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, mch.short_root_bus, 0),
>      DEFINE_PROP_SIZE(PCI_HOST_BELOW_4G_MEM_SIZE, Q35PCIHost,
>                       mch.below_4g_mem_size, 0),
>      DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost,
>                       mch.above_4g_mem_size, 0),
> +    DEFINE_PROP_BOOL("x-pci-hole64-fix", Q35PCIHost, pci_hole64_fix, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -174,7 +185,9 @@ static void q35_host_initfn(Object *obj)
>      object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
>      qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
>      qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
> -
> +    /* mch's object_initialize resets the default value, set it again */
> +    qdev_prop_set_uint64(DEVICE(s), PCI_HOST_PROP_PCI_HOLE64_SIZE,
> +                         1ULL << 35);
>      object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
>                          q35_host_get_pci_hole_start,
>                          NULL, NULL, NULL, NULL);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 087d184ef5..ef438bd765 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -238,7 +238,6 @@ void pc_guest_info_init(PCMachineState *pcms);
>  #define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
>  #define PCI_HOST_BELOW_4G_MEM_SIZE     "below-4g-mem-size"
>  #define PCI_HOST_ABOVE_4G_MEM_SIZE     "above-4g-mem-size"
> -#define DEFAULT_PCI_HOLE64_SIZE (~0x0ULL)
>  
>  
>  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> @@ -249,6 +248,7 @@ void pc_memory_init(PCMachineState *pcms,
>                      MemoryRegion *system_memory,
>                      MemoryRegion *rom_memory,
>                      MemoryRegion **ram_memory);
> +uint64_t pc_pci_hole64_start(void);
>  qemu_irq pc_allocate_cpu_irq(void);
>  DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> @@ -375,6 +375,14 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>          .driver   = TYPE_X86_CPU,\
>          .property = "x-hv-max-vps",\
>          .value    = "0x40",\
> +    },{\
> +        .driver   = "i440FX-pcihost",\
> +        .property = "x-pci-hole64-fix",\
> +        .value    = "off",\
> +    },{\
> +        .driver   = "q35-pcihost",\
> +        .property = "x-pci-hole64-fix",\
> +        .value    = "off",\
>      },
>  
>  #define PC_COMPAT_2_9 \
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index 58983c00b3..8f4ddde393 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -68,6 +68,7 @@ typedef struct Q35PCIHost {
>      PCIExpressHost parent_obj;
>      /*< public >*/
>  
> +    bool pci_hole64_fix;
>      MCHPCIState mch;
>  } Q35PCIHost;
>  
> 

This is a tricky patch, I stared at it for long. I also re-read the v1
discussion.

Two "logic improvements" I could imagine are:

(1) Failing the "hole64_end" getter functions -- via errp -- if the
user-set pci_hole64_size property is zero (because otherwise we could
configure a zero-sized 64-bit hole if the guest didn't configure any),

(2) in the "hole64_end" getter functions, perhaps we want to round up
the pci_hole64_size property to a multiple of 1GB.

But, we can also say that setting sensible values for the property is
the responsibility of the user / management layer. So I'll leave (1)
and/or (2) up to you.


(3) An idea for the property defaults: you remove
DEFAULT_PCI_HOLE64_SIZE, which is cool. How about introducing (in the
proper header files)

#define DEFAULT_I440FX_PCI_HOLE64_SIZE (1ULL << 31)
#define DEFAULT_Q35_PCI_HOLE64_SIZE    (1ULL << 35)

The main reason for my suggestion is that (1ULL << 35) is used twice in
the code, for an obscure qdev/qom reason. The comments definitely help,
so keep them, but a greppable macro would help even more, IMO.

And, once we add DEFAULT_Q35_PCI_HOLE64_SIZE, we should add
DEFAULT_I440FX_PCI_HOLE64_SIZE too, for consistency.

Anyway, I'll leave this up to you as well.


(4) I also have a suggestion for the commit message: please move the
paragraph that starts with

  "Even if the new defaults..."

from the v2->v3 changelog to the commit message proper. IMO it is
important information.


With (4) addressed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH V3] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole
Posted by Marcel Apfelbaum 6 years, 4 months ago
On 09/11/2017 13:02, Laszlo Ersek wrote:
> On 11/08/17 15:48, Marcel Apfelbaum wrote:
>> Currently there is no MMIO range over 4G
>> reserved for PCI hotplug. Since the 32bit PCI hole
>> depends on the number of cold-plugged PCI devices
>> and other factors, it is very possible is too small
>> to hotplug PCI devices with large BARs.
>>
>> Fix it by reserving 2G for I4400FX chipset
>> in order to comply with older Win32 Guest OSes
>> and 32G for Q35 chipset.
>>
>> Note this is a regression since prev QEMU versions had
>> some range reserved for 64bit PCI hotplug.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>
>> V2 -> V3:
>>   - Addressed Gerd's and others comments and re-enabled the pci-hole64-size
>>     property defaulting it to 2G for I440FX and 32g for Q35.
>>   - Even if the new defaults of pci-hole64-size will appear in "info qtree"
>>     also for older machines, the property was not implemented so
>>     no changes will be visible to guests.
>>
>> V1 -> V2:
>>   Addressed Igor's comments:
>>      - aligned the hole64 start to 1Gb
>>       (I think all the computations took care of it already,
>>        but it can't hurt)
>>      - Init compat props to "off" instead of "false"
>>
>> Thanks,
>> Marcel
>>
>>
>> hw/i386/pc.c              | 18 ++++++++++++++++++
>>   hw/pci-host/piix.c        | 13 ++++++++++++-
>>   hw/pci-host/q35.c         | 17 +++++++++++++++--
>>   include/hw/i386/pc.h      | 10 +++++++++-
>>   include/hw/pci-host/q35.h |  1 +
>>   5 files changed, 55 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index e11a65b545..af3177cca5 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1448,6 +1448,24 @@ void pc_memory_init(PCMachineState *pcms,
>>       pcms->ioapic_as = &address_space_memory;
>>   }
>>   
>> +uint64_t pc_pci_hole64_start(void)
>> +{
>> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> +    uint64_t hole64_start = 0;
>> +
>> +    if (pcmc->has_reserved_memory && pcms->hotplug_memory.base) {
>> +        hole64_start = pcms->hotplug_memory.base;
>> +        if (!pcmc->broken_reserved_end) {
>> +            hole64_start += memory_region_size(&pcms->hotplug_memory.mr);
>> +        }
>> +    } else {
>> +        hole64_start = 0x100000000ULL + pcms->above_4g_mem_size;
>> +    }
>> +
>> +    return ROUND_UP(hole64_start, 1ULL << 30);
>> +}
>> +
>>   qemu_irq pc_allocate_cpu_irq(void)
>>   {
>>       return qemu_allocate_irq(pic_irq_request, NULL, 0);
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index a7e2256870..e033ee7df8 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -50,6 +50,7 @@ typedef struct I440FXState {
>>       PCIHostState parent_obj;
>>       Range pci_hole;
>>       uint64_t pci_hole64_size;
>> +    bool pci_hole64_fix;
>>       uint32_t short_root_bus;
>>   } I440FXState;
>>   
>> @@ -243,11 +244,15 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
>>                                                   void *opaque, Error **errp)
>>   {
>>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>>       Range w64;
>>       uint64_t value;
>>   
>>       pci_bus_get_w64_range(h->bus, &w64);
>>       value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    if (!value && s->pci_hole64_fix) {
>> +        value = pc_pci_hole64_start();
>> +    }
>>       visit_type_uint64(v, name, &value, errp);
>>   }
>>   
>> @@ -256,11 +261,16 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>>                                                 Error **errp)
>>   {
>>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>> +    uint64_t hole64_start = pc_pci_hole64_start();
>>       Range w64;
>>       uint64_t value;
>>   
>>       pci_bus_get_w64_range(h->bus, &w64);
>>       value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    if (s->pci_hole64_fix && value < (hole64_start + s->pci_hole64_size)) {
>> +        value = hole64_start + s->pci_hole64_size;
>> +    }
>>       visit_type_uint64(v, name, &value, errp);
>>   }
>>   
>> @@ -863,8 +873,9 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>>   
>>   static Property i440fx_props[] = {
>>       DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, I440FXState,
>> -                     pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
>> +                     pci_hole64_size, 1ULL << 31),
>>       DEFINE_PROP_UINT32("short_root_bus", I440FXState, short_root_bus, 0),
>> +    DEFINE_PROP_BOOL("x-pci-hole64-fix", I440FXState, pci_hole64_fix, true),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index ddaa7d1b44..f95337c13a 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -104,11 +104,15 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
>>                                             Error **errp)
>>   {
>>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>>       Range w64;
>>       uint64_t value;
>>   
>>       pci_bus_get_w64_range(h->bus, &w64);
>>       value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    if (!value && s->pci_hole64_fix) {
>> +        value = pc_pci_hole64_start();
>> +    }
>>       visit_type_uint64(v, name, &value, errp);
>>   }
>>   
>> @@ -117,11 +121,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>>                                           Error **errp)
>>   {
>>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>> +    uint64_t hole64_start = pc_pci_hole64_start();
>>       Range w64;
>>       uint64_t value;
>>   
>>       pci_bus_get_w64_range(h->bus, &w64);
>>       value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    if (s->pci_hole64_fix && value < (hole64_start + s->mch.pci_hole64_size)) {
>> +        value = hole64_start + s->mch.pci_hole64_size;
>> +    }
>>       visit_type_uint64(v, name, &value, errp);
>>   }
>>   
>> @@ -136,13 +145,15 @@ static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
>>   static Property q35_host_props[] = {
>>       DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, parent_obj.base_addr,
>>                           MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
>> +    /* The default value is overriden in q35_host_initfn */
>>       DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
>> -                     mch.pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
>> +                     mch.pci_hole64_size, 1ULL << 35),
>>       DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, mch.short_root_bus, 0),
>>       DEFINE_PROP_SIZE(PCI_HOST_BELOW_4G_MEM_SIZE, Q35PCIHost,
>>                        mch.below_4g_mem_size, 0),
>>       DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost,
>>                        mch.above_4g_mem_size, 0),
>> +    DEFINE_PROP_BOOL("x-pci-hole64-fix", Q35PCIHost, pci_hole64_fix, true),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> @@ -174,7 +185,9 @@ static void q35_host_initfn(Object *obj)
>>       object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
>>       qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
>>       qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
>> -
>> +    /* mch's object_initialize resets the default value, set it again */
>> +    qdev_prop_set_uint64(DEVICE(s), PCI_HOST_PROP_PCI_HOLE64_SIZE,
>> +                         1ULL << 35);
>>       object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
>>                           q35_host_get_pci_hole_start,
>>                           NULL, NULL, NULL, NULL);
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 087d184ef5..ef438bd765 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -238,7 +238,6 @@ void pc_guest_info_init(PCMachineState *pcms);
>>   #define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
>>   #define PCI_HOST_BELOW_4G_MEM_SIZE     "below-4g-mem-size"
>>   #define PCI_HOST_ABOVE_4G_MEM_SIZE     "above-4g-mem-size"
>> -#define DEFAULT_PCI_HOLE64_SIZE (~0x0ULL)
>>   
>>   
>>   void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
>> @@ -249,6 +248,7 @@ void pc_memory_init(PCMachineState *pcms,
>>                       MemoryRegion *system_memory,
>>                       MemoryRegion *rom_memory,
>>                       MemoryRegion **ram_memory);
>> +uint64_t pc_pci_hole64_start(void);
>>   qemu_irq pc_allocate_cpu_irq(void);
>>   DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>>   void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>> @@ -375,6 +375,14 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>>           .driver   = TYPE_X86_CPU,\
>>           .property = "x-hv-max-vps",\
>>           .value    = "0x40",\
>> +    },{\
>> +        .driver   = "i440FX-pcihost",\
>> +        .property = "x-pci-hole64-fix",\
>> +        .value    = "off",\
>> +    },{\
>> +        .driver   = "q35-pcihost",\
>> +        .property = "x-pci-hole64-fix",\
>> +        .value    = "off",\
>>       },
>>   
>>   #define PC_COMPAT_2_9 \
>> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
>> index 58983c00b3..8f4ddde393 100644
>> --- a/include/hw/pci-host/q35.h
>> +++ b/include/hw/pci-host/q35.h
>> @@ -68,6 +68,7 @@ typedef struct Q35PCIHost {
>>       PCIExpressHost parent_obj;
>>       /*< public >*/
>>   
>> +    bool pci_hole64_fix;
>>       MCHPCIState mch;
>>   } Q35PCIHost;
>>   
>>

Hi Laszlo,

> 
> This is a tricky patch, I stared at it for long. I also re-read the v1
> discussion.
> 

Appreciated

> Two "logic improvements" I could imagine are:
> 
> (1) Failing the "hole64_end" getter functions -- via errp -- if the
> user-set pci_hole64_size property is zero (because otherwise we could
> configure a zero-sized 64-bit hole if the guest didn't configure any),
> 

I prefer to leave as it is, 0 is a fair value, and 0 size value
will not appear in the guest (the acpi build code cuts 0 size windows)

> (2) in the "hole64_end" getter functions, perhaps we want to round up
> the pci_hole64_size property to a multiple of 1GB.
> 

Sure, it can't hurt.

> But, we can also say that setting sensible values for the property is
> the responsibility of the user / management layer. So I'll leave (1)
> and/or (2) up to you.
> 
> 
> (3) An idea for the property defaults: you remove
> DEFAULT_PCI_HOLE64_SIZE, which is cool. How about introducing (in the
> proper header files)
> 
> #define DEFAULT_I440FX_PCI_HOLE64_SIZE (1ULL << 31)
> #define DEFAULT_Q35_PCI_HOLE64_SIZE    (1ULL << 35)
> 
> The main reason for my suggestion is that (1ULL << 35) is used twice in
> the code, for an obscure qdev/qom reason. The comments definitely help,
> so keep them, but a greppable macro would help even more, IMO.
> 
> And, once we add DEFAULT_Q35_PCI_HOLE64_SIZE, we should add
> DEFAULT_I440FX_PCI_HOLE64_SIZE too, for consistency.
> 
> Anyway, I'll leave this up to you as well.
> 

Will do, thanks.

> 
> (4) I also have a suggestion for the commit message: please move the
> paragraph that starts with
> 
>    "Even if the new defaults..."
> 
> from the v2->v3 changelog to the commit message proper. IMO it is
> important information.
> 

Sure

> 
> With (4) addressed:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 

Thanks!
Marcel


> Thanks!
> Laszlo
> 


Re: [Qemu-devel] [PATCH V3] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole
Posted by Eduardo Habkost 6 years, 4 months ago
On Thu, Nov 09, 2017 at 12:02:10PM +0100, Laszlo Ersek wrote:
[...]
> (3) An idea for the property defaults: you remove
> DEFAULT_PCI_HOLE64_SIZE, which is cool. How about introducing (in the
> proper header files)
> 
> #define DEFAULT_I440FX_PCI_HOLE64_SIZE (1ULL << 31)
> #define DEFAULT_Q35_PCI_HOLE64_SIZE    (1ULL << 35)
> 
> The main reason for my suggestion is that (1ULL << 35) is used twice in
> the code, for an obscure qdev/qom reason. The comments definitely help,
> so keep them, but a greppable macro would help even more, IMO.
> 
> And, once we add DEFAULT_Q35_PCI_HOLE64_SIZE, we should add
> DEFAULT_I440FX_PCI_HOLE64_SIZE too, for consistency.

Agreed, especially considering how the code that initializes the
defaults is non-obvious.

> 
> Anyway, I'll leave this up to you as well.

If we do that, I recommend adding a bigger warning to
q35_host_props.  The one in this patch is very easy to miss if
people try to touch the defaults for other mch.* properties in
the future.  I suggest something like:

    /*
     * NOTE: setting defaults for the mch.* fields in this table
     * don't work, because mch is a separate QOM object that is
     * zeroed by the object_initialize(&s->mch, ...) call inside
     * q35_host_initfn().  The default values for those
     * properties need to be initialized manually by
     * q35_host_initfn() after the object_initialize() call.
     */


> 
> 
> (4) I also have a suggestion for the commit message: please move the
> paragraph that starts with
> 
>   "Even if the new defaults..."
> 
> from the v2->v3 changelog to the commit message proper. IMO it is
> important information.
> 
> 
> With (4) addressed:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!
> Laszlo

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole
Posted by Michael S. Tsirkin 6 years, 4 months ago
On Wed, Nov 08, 2017 at 04:48:09PM +0200, Marcel Apfelbaum wrote:
> Currently there is no MMIO range over 4G
> reserved for PCI hotplug. Since the 32bit PCI hole
> depends on the number of cold-plugged PCI devices
> and other factors, it is very possible is too small
> to hotplug PCI devices with large BARs.
> 
> Fix it by reserving 2G for I4400FX chipset
> in order to comply with older Win32 Guest OSes
> and 32G for Q35 chipset.
> 
> Note this is a regression since prev QEMU versions had
> some range reserved for 64bit PCI hotplug.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Looks sane to me, but could we add some documentation please?
Short term code comments. Long term let's add a document.

> ---
> 
> V2 -> V3:
>  - Addressed Gerd's and others comments and re-enabled the pci-hole64-size
>    property defaulting it to 2G for I440FX and 32g for Q35.
>  - Even if the new defaults of pci-hole64-size will appear in "info qtree"
>    also for older machines, the property was not implemented so
>    no changes will be visible to guests.
> 
> V1 -> V2:
>  Addressed Igor's comments:
>     - aligned the hole64 start to 1Gb
>      (I think all the computations took care of it already,
>       but it can't hurt)
>     - Init compat props to "off" instead of "false"
> 
> Thanks,
> Marcel 
> 
> 
> hw/i386/pc.c              | 18 ++++++++++++++++++
>  hw/pci-host/piix.c        | 13 ++++++++++++-
>  hw/pci-host/q35.c         | 17 +++++++++++++++--
>  include/hw/i386/pc.h      | 10 +++++++++-
>  include/hw/pci-host/q35.h |  1 +
>  5 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e11a65b545..af3177cca5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1448,6 +1448,24 @@ void pc_memory_init(PCMachineState *pcms,
>      pcms->ioapic_as = &address_space_memory;
>  }
>  
> +uint64_t pc_pci_hole64_start(void)
> +{
> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    uint64_t hole64_start = 0;
> +
> +    if (pcmc->has_reserved_memory && pcms->hotplug_memory.base) {
> +        hole64_start = pcms->hotplug_memory.base;
> +        if (!pcmc->broken_reserved_end) {
> +            hole64_start += memory_region_size(&pcms->hotplug_memory.mr);
> +        }
> +    } else {
> +        hole64_start = 0x100000000ULL + pcms->above_4g_mem_size;
> +    }
> +
> +    return ROUND_UP(hole64_start, 1ULL << 30);
> +}
> +
>  qemu_irq pc_allocate_cpu_irq(void)
>  {
>      return qemu_allocate_irq(pic_irq_request, NULL, 0);
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index a7e2256870..e033ee7df8 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -50,6 +50,7 @@ typedef struct I440FXState {
>      PCIHostState parent_obj;
>      Range pci_hole;
>      uint64_t pci_hole64_size;
> +    bool pci_hole64_fix;
>      uint32_t short_root_bus;
>  } I440FXState;
>  
> @@ -243,11 +244,15 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
>                                                  void *opaque, Error **errp)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>      Range w64;
>      uint64_t value;
>  
>      pci_bus_get_w64_range(h->bus, &w64);
>      value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    if (!value && s->pci_hole64_fix) {
> +        value = pc_pci_hole64_start();
> +    }
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> @@ -256,11 +261,16 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>                                                Error **errp)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> +    uint64_t hole64_start = pc_pci_hole64_start();
>      Range w64;
>      uint64_t value;
>  
>      pci_bus_get_w64_range(h->bus, &w64);
>      value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    if (s->pci_hole64_fix && value < (hole64_start + s->pci_hole64_size)) {
> +        value = hole64_start + s->pci_hole64_size;
> +    }
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> @@ -863,8 +873,9 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>  
>  static Property i440fx_props[] = {
>      DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, I440FXState,
> -                     pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
> +                     pci_hole64_size, 1ULL << 31),
>      DEFINE_PROP_UINT32("short_root_bus", I440FXState, short_root_bus, 0),
> +    DEFINE_PROP_BOOL("x-pci-hole64-fix", I440FXState, pci_hole64_fix, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index ddaa7d1b44..f95337c13a 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -104,11 +104,15 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
>                                            Error **errp)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>      Range w64;
>      uint64_t value;
>  
>      pci_bus_get_w64_range(h->bus, &w64);
>      value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    if (!value && s->pci_hole64_fix) {
> +        value = pc_pci_hole64_start();
> +    }
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> @@ -117,11 +121,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>                                          Error **errp)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +    uint64_t hole64_start = pc_pci_hole64_start();
>      Range w64;
>      uint64_t value;
>  
>      pci_bus_get_w64_range(h->bus, &w64);
>      value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    if (s->pci_hole64_fix && value < (hole64_start + s->mch.pci_hole64_size)) {
> +        value = hole64_start + s->mch.pci_hole64_size;
> +    }
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> @@ -136,13 +145,15 @@ static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
>  static Property q35_host_props[] = {
>      DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, parent_obj.base_addr,
>                          MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> +    /* The default value is overriden in q35_host_initfn */
>      DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
> -                     mch.pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
> +                     mch.pci_hole64_size, 1ULL << 35),
>      DEFINE_PROP_UINT32("short_root_bus", Q35PCIHost, mch.short_root_bus, 0),
>      DEFINE_PROP_SIZE(PCI_HOST_BELOW_4G_MEM_SIZE, Q35PCIHost,
>                       mch.below_4g_mem_size, 0),
>      DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost,
>                       mch.above_4g_mem_size, 0),
> +    DEFINE_PROP_BOOL("x-pci-hole64-fix", Q35PCIHost, pci_hole64_fix, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -174,7 +185,9 @@ static void q35_host_initfn(Object *obj)
>      object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
>      qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
>      qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
> -
> +    /* mch's object_initialize resets the default value, set it again */
> +    qdev_prop_set_uint64(DEVICE(s), PCI_HOST_PROP_PCI_HOLE64_SIZE,
> +                         1ULL << 35);
>      object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",
>                          q35_host_get_pci_hole_start,
>                          NULL, NULL, NULL, NULL);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 087d184ef5..ef438bd765 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -238,7 +238,6 @@ void pc_guest_info_init(PCMachineState *pcms);
>  #define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
>  #define PCI_HOST_BELOW_4G_MEM_SIZE     "below-4g-mem-size"
>  #define PCI_HOST_ABOVE_4G_MEM_SIZE     "above-4g-mem-size"
> -#define DEFAULT_PCI_HOLE64_SIZE (~0x0ULL)
>  
>  
>  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> @@ -249,6 +248,7 @@ void pc_memory_init(PCMachineState *pcms,
>                      MemoryRegion *system_memory,
>                      MemoryRegion *rom_memory,
>                      MemoryRegion **ram_memory);
> +uint64_t pc_pci_hole64_start(void);
>  qemu_irq pc_allocate_cpu_irq(void);
>  DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> @@ -375,6 +375,14 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>          .driver   = TYPE_X86_CPU,\
>          .property = "x-hv-max-vps",\
>          .value    = "0x40",\
> +    },{\
> +        .driver   = "i440FX-pcihost",\
> +        .property = "x-pci-hole64-fix",\
> +        .value    = "off",\
> +    },{\
> +        .driver   = "q35-pcihost",\
> +        .property = "x-pci-hole64-fix",\
> +        .value    = "off",\
>      },
>  
>  #define PC_COMPAT_2_9 \
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index 58983c00b3..8f4ddde393 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -68,6 +68,7 @@ typedef struct Q35PCIHost {
>      PCIExpressHost parent_obj;
>      /*< public >*/
>  
> +    bool pci_hole64_fix;
>      MCHPCIState mch;
>  } Q35PCIHost;
>  
> -- 
> 2.13.5