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

[Igor Mammedov]

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

> 
> 
> >> +        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.

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.

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