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

[Marcel Apfelbaum <marcel@redhat.com>]

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
>