qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH V5] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole
Date: Wed, 15 Nov 2017 13:51:00 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

On 15/11/2017 4:16, Michael S. Tsirkin wrote:
On Mon, Nov 13, 2017 at 03:07:45PM +0200, Marcel Apfelbaum wrote:
On 11/11/2017 17:25, 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.

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.

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

Reviewed-by: Laszlo Ersek <address@hidden>
Reviewed-by: Gerd Hoffmann <address@hidden>
Signed-off-by: Marcel Apfelbaum <address@hidden>
---


Hi Michael,

Can you please merge the patch for QEMU 2.11
if you have no further comments?
I think is an important fix and it worth
having it in 2.11 .

Thanks,
Marcel


Hi Michael,

Just a note: this changes the DSDT to use
QWordMemory in the _CRS.

I expected it to cause trouble for old windows such as winXP
but it does not seem to cause them - at least not boot crashes.

Worth checking that 32 bit hotplug still works though.
Could you pls confirm?


I installed a fresh WinXpSp3 32bit image and I can confirm
there is no issue with both the installation and
PCI hotplug/hot-unplug of a E1000 NIC.

Thanks,
Marcel

V4 -> V5:
   - Renamed a local variable (Laszlo)
   - Added a comment to q35 props (Eduardo)

V3 -> V4:
   - Addressed Laszlo's comments:
      - Added defines for pci-hole64 default size props.
      - Rounded the hole64_end to 1G
      - Moved some info to commit message
   - Addressed Michael's comments:
      - Added more comments.
   - I kept Gerd's "review-by" tag since no functional changes were made.

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"

   hw/i386/pc.c              | 22 ++++++++++++++++++++++
   hw/pci-host/piix.c        | 32 ++++++++++++++++++++++++++++++--
   hw/pci-host/q35.c         | 42 +++++++++++++++++++++++++++++++++++++++---
   include/hw/i386/pc.h      | 10 +++++++++-
   include/hw/pci-host/q35.h |  1 +
   5 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e11a65b545..fafe5ba5cd 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1448,6 +1448,28 @@ void pc_memory_init(PCMachineState *pcms,
       pcms->ioapic_as = &address_space_memory;
   }
+/*
+ * The 64bit pci hole starts after "above 4G RAM" and
+ * potentially the space reserved for memory hotplug.
+ */
+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..a684a7cca9 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;
@@ -112,6 +113,9 @@ struct PCII440FXState {
   #define I440FX_PAM_SIZE 7
   #define I440FX_SMRAM    0x72
+/* Keep it 2G to comply with older win32 guests */
+#define I440FX_PCI_HOST_HOLE64_SIZE_DEFAULT (1ULL << 31)
+
   /* Older coreboot versions (4.0 and older) read a config register that 
doesn't
    * exist in real hardware, to get the RAM size from QEMU.
    */
@@ -238,29 +242,52 @@ static void i440fx_pcihost_get_pci_hole_end(Object *obj, 
Visitor *v,
       visit_type_uint32(v, name, &value, errp);
   }
+/*
+ * The 64bit PCI hole start is set by the Guest firmware
+ * as the address of the first 64bit PCI MEM resource.
+ * If no PCI device has resources on the 64bit area,
+ * the 64bit PCI hole will start after "over 4G RAM" and the
+ * reserved space for memory hotplug if any.
+ */
   static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
                                                   const char *name,
                                                   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);
   }
+/*
+ * The 64bit PCI hole end is set by the Guest firmware
+ * as the address of the last 64bit PCI MEM resource.
+ * Then it is expanded to the PCI_HOST_PROP_PCI_HOLE64_SIZE
+ * that can be configured by the user.
+ */
   static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
                                                 const char *name, void *opaque,
                                                 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;
+    uint64_t value, hole64_end;
       pci_bus_get_w64_range(h->bus, &w64);
       value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
+    hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL << 30);
+    if (s->pci_hole64_fix && value < hole64_end) {
+        value = hole64_end;
+    }
       visit_type_uint64(v, name, &value, errp);
   }
@@ -863,8 +890,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, I440FX_PCI_HOST_HOLE64_SIZE_DEFAULT),
       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..6cb9a8d121 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -37,6 +37,8 @@
    * Q35 host
    */
+#define Q35_PCI_HOST_HOLE64_SIZE_DEFAULT (1ULL << 35)
+
   static void q35_host_realize(DeviceState *dev, Error **errp)
   {
       PCIHostState *pci = PCI_HOST_BRIDGE(dev);
@@ -99,29 +101,52 @@ static void q35_host_get_pci_hole_end(Object *obj, Visitor 
*v,
       visit_type_uint32(v, name, &value, errp);
   }
+/*
+ * The 64bit PCI hole start is set by the Guest firmware
+ * as the address of the first 64bit PCI MEM resource.
+ * If no PCI device has resources on the 64bit area,
+ * the 64bit PCI hole will start after "over 4G RAM" and the
+ * reserved space for memory hotplug if any.
+ */
   static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
                                             const char *name, void *opaque,
                                             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);
   }
+/*
+ * The 64bit PCI hole end is set by the Guest firmware
+ * as the address of the last 64bit PCI MEM resource.
+ * Then it is expanded to the PCI_HOST_PROP_PCI_HOLE64_SIZE
+ * that can be configured by the user.
+ */
   static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
                                           const char *name, void *opaque,
                                           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;
+    uint64_t value, hole64_end;
       pci_bus_get_w64_range(h->bus, &w64);
       value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
+    hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 1ULL << 30);
+    if (s->pci_hole64_fix && value < hole64_end) {
+        value = hole64_end;
+    }
       visit_type_uint64(v, name, &value, errp);
   }
@@ -133,16 +158,25 @@ static void q35_host_get_mmcfg_size(Object *obj, Visitor 
*v, const char *name,
       visit_type_uint64(v, name, &e->size, errp);
   }
+/*
+ * NOTE: setting defaults for the mch.* fields in this table
+ * doesn'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.
+ */
   static Property q35_host_props[] = {
       DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, 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),
+                     mch.pci_hole64_size, Q35_PCI_HOST_HOLE64_SIZE_DEFAULT),
       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 +208,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,
+                         Q35_PCI_HOST_HOLE64_SIZE_DEFAULT);
       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;





reply via email to

[Prev in Thread] Current Thread [Next in Thread]