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

[Marcel Apfelbaum]

On 05/16/2016 05:19 PM, Igor Mammedov wrote:
> On Mon, 16 May 2016 13:14:51 +0300
> Marcel Apfelbaum <address@hidden> wrote:
>
> > On 05/16/2016 11:24 AM, Igor Mammedov wrote:
> > > On Sun, 15 May 2016 22:23:32 +0300
> > > Marcel Apfelbaum <address@hidden> 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 <address@hidden>
> > > > ---
> > > >    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)