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

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 04:31:46PM +0200, Igor Mammedov wrote:
> On Wed, 18 May 2016 17:12:09 +0300
> Marcel Apfelbaum <address@hidden> wrote:
> 
> > On 05/18/2016 05:11 PM, Michael S. Tsirkin wrote:
> > > On Wed, May 18, 2016 at 03:59:29PM +0200, 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>  
> > >> that patch also has side effect of unconditionally adding
> > >> QWordMemory() resource in PCI0._CRS
> > >> on all machine types with QEMU generated ACPI tables.
> > >>
> > >> Have you tested that it won't break boot of legacy OSes
> > >> (XP, WS2003, old linux with 32bit kernel)?  
> > >
> > > It's almost sure it break it.
> > > Maybe you can check _REV in _CRS to work around this for XP.  
> > 
> > I'll try it.
> but only after you check if just presence of QWord would crash XP,
> so in case it doesn't crash we would keep _CRS simple static
> structure.
> 
> I very vaguely recall that XP ignored QWord in PCI0._CRS,
> but it was long time ago so it won't hurt to recheck.

I played with different guests (32 and 64 bit)
at some point.

Generally, windows tends to crash when CRS resources exceed the
supported limits
of physical memory (sometimes with weird off by one errors,
e.g. win7 32 bit seems to survive with a 36 bit pci hole even though
this means the max address is 2^37-1 which it can't address).

This might depend on CPU as well.

Which makes me ask: why don't we fix this in BIOS?
If you want it to allocate a large window, do it.

> > 
> > Thanks,
> > Marcel
> > 
> > >  
> > >>> ---
> > >>>   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)
> > >>>
> > >>>   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);
> > >>> +        if (!range->begin) {
> > >>> +            range->begin = ROUND_UP(0x100000000ULL + 
> > >>> pcms->above_4g_mem_size,
> > >>> +                                    1ULL << 30);
> > >>> +        }
> > >>> +        range->end = 1ULL << 40; /* 40 bits physical */
> > >>> +    } 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)  
> >