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 <mar...@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
