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

[Marcel Apfelbaum <marcel@redhat.com>]

On 05/18/2016 05:26 PM, Igor Mammedov wrote:
> On Wed, 18 May 2016 17:07:05 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> 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.
> it's just nice to have cleanup, but I don't insist on it.
>
> the thing here is that if pc_machine_get_reserved_memory_end() were used
> directly for acpi-build.c then properties as is would give old/different values.

You are right, I'll remove at least the 64-bit properties.

Thanks,
Marcel

>
>>
>> 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)
>>>>>
>>>>
>>>
>>
>