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

[Igor Mammedov <imammedo@redhat.com>]

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.

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