Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum <marcel@redhat.com>]

On 05/16/2016 05:19 PM, Igor Mammedov wrote:
> On Mon, 16 May 2016 13:14:51 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 05/16/2016 11:24 AM, Igor Mammedov wrote:
>>> On Sun, 15 May 2016 22:23:32 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>
>>>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>>>> leads to zero space for hot-plugging PCI devices over 4G.
>>>>
>>>> PC machines can use the whole CPU addressable range after
>>>> the space reserved for memory-hotplug.
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> ---
>>>>    hw/pci/pci.c | 16 ++++++++++++++--
>>>>    1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index bb605ef..44dd949 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -41,6 +41,7 @@
>>>>    #include "hw/hotplug.h"
>>>>    #include "hw/boards.h"
>>>>    #include "qemu/cutils.h"
>>>> +#include "hw/i386/pc.h"
>>>>
>>>>    //#define DEBUG_PCI
>>>>    #ifdef DEBUG_PCI
>>>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>>>
>>
>>
>> Hi Igor,
>> Thanks for reviewing this series.
>>
>>>>    void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>>>    {
>>>> -    range->begin = range->end = 0;
>>>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>> +    Object *machine = qdev_get_machine();
>>>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>>>> +        PCMachineState *pcms = PC_MACHINE(machine);
>>>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
>>> that line should break linking on other targets which don't have
>>> pc_machine_get_reserved_memory_end()
>>> probably for got to add stub.
>>>
>>
>> I cross-compiled all the targets with no problem.  It seems no stub is required.
> ./configure && make
>
>    LINK  aarch64-softmmu/qemu-system-aarch64
> ../hw/pci/pci.o: In function `pci_bus_get_w64_range':
> /home/imammedo/builds/qemu/hw/pci/pci.c:2474: undefined reference to `pc_machine_get_reserved_memory_end'
> collect2: error: ld returned 1 exit status

Ooops, I did configured it to cross-compile everything, but I somehow missed it.
I'll take care of it.


>
>>
>>
>>>> +        if (!range->begin) {
>>>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>>>> +                                    1ULL << 30);
>>>> +        }
> mayby move above hunk to pc_machine_get_reserved_memory_end()
> so it would always return valid value.
>
>>>> +        range->end = 1ULL << 40; /* 40 bits physical */
>>> x86 specific in generic code
>>>
>>
>> I am aware I put x86 code in the generic pci code, but I limited it with
>>       if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>> So we have a generic rule for getting the w64 range and a specific one for PC machines.
>>
>> The alternative would be a w64 range per host-bridge, not bus.
>> Adding a pci_bus_get_w64_range function to PCIHostBridgeClass,
>> defaulting in current implementation for the base class and
>> overriding it for piix/q35 looks OK to you?
>>
>> I thought about it, but it seemed over-engineered as opposed
>> to the 'simple' if statement, however I can try it if you think is better.
>>
>>> ARM also has 64-bit PCI MMIO /git grep VIRT_PCIE_MMIO_HIGH/
>>
>> I had a look and ARM does not use this infrastructure, it has its own abstractions,
>> a memmap list. This is the other reason I selected this implementation,
>> because is not really used by other targets (but it can be used in the future)
>
> if it's only for x86 and whatever was programmed by BIOS is ignored,
> I'd drop PCI_HOST_PROP_PCI_HOLE64_*/pci_bus_get_w64_range() indirection
> and just directly use pc_machine_get_reserved_memory_end()
> from acpi-build.c
>
> in that case you won't need a stub for pc_machine_get_reserved_memory_end()
> as its usage will be limited only to hw/i386 scope.
>

Good point, I'll try this.

> Given opportunity, I'd drop PCI_HOST_PROP_PCI_HOLE* properties,
> but for it we need ACK from libvirt side in case they are using it
> for some reason.

It seems out of the scope of this series, however I can do it on top.

Thanks,
Marcel

>
>>
>>
>> Thanks,
>> Marcel
>>
>>> perhaps range should be a property of PCI bus,
>>> where a board sets its own values for start/size
>>>
>>>> +    } else {
>>>> +        range->begin = range->end = 0;
>>>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>> +    }
>>>>    }
>>>>
>>>>    static bool pcie_has_upstream_port(PCIDevice *dev)
>>>
>>
>