Re: [Qemu-devel] [PATCH 0/4] pci: better support for 64-bit MMIO allocation

[Laszlo Ersek]

Hi Marcel,

On 05/02/16 17:37, Marcel Apfelbaum wrote:
> Hi,
> 
> First two patches allocate (max_reserved_ram - max_addr_cpu_addressable) 
> range for PCI hotplug
> (for PC Machines) instead of the previous 64-bit PCI window that included only
> the ranges allocated by the firmware.
> 
> The next two patches fix 64-bit CRS computations.
> 
> Thank you,
> Marcel
> 
> Marcel Apfelbaum (4):
>   hw/pc: extract reserved memory end computation to a standalone
>     function
>   pci: reserve 64 bit MMIO range for PCI hotplug
>   acpi: refactor pxb crs computation
>   hw/apci: handle 64-bit MMIO regions correctly
> 
>  hw/i386/acpi-build.c | 135 
> ++++++++++++++++++++++++++++++++++++---------------
>  hw/i386/pc.c         |  29 ++++++++---
>  hw/pci/pci.c         |  16 +++++-
>  include/hw/i386/pc.h |   1 +
>  4 files changed, 131 insertions(+), 50 deletions(-)
> 

I just tried to test this series, on top of v2.6.0-rc5. (My intent was to dump 
and decompile the DSDT in the guest, and to compare the versions before vs. 
after.)

The series doesn't apply.

You had posted it on May 2nd, so I tried to apply the series to v2.6.0-rc4 
(2016-05-02) and v2.6.0-rc3 too (2016-04-21); neither worked.

The error message from "git am --reject" is, for patch #4:

Checking patch hw/i386/acpi-build.c...
error: while searching for:
    }

    if (pci->w64.begin) {
        aml_append(crs,
            aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
                             AML_CACHEABLE, AML_READ_WRITE,
                             0, pci->w64.begin, pci->w64.end - 1, 0,
                             pci->w64.end - pci->w64.begin));
    }
    aml_append(scope, aml_name_decl("_CRS", crs));

Looking at the trailing context in your patch #4, in particular

    aml_append(scope, aml_name_decl("_CRS", crs));

I think that the conflict is caused by this intervening commit:

commit 2b1c2e8e5f1990f0a201a8cbf9d366fca60f4aa8
Author: Igor Mammedov <address@hidden>
Date:   Fri Apr 8 13:23:13 2016 +0200

    pc: acpi: tpm: add missing MMIO resource to PCI0._CRS

Which was part of v2.6.0-rc2.

... Actually, that's a pretty small patch, so maybe I can apply your series at 
2b1c2e8e5f19^, then rebase it to v2.6.0-rc5 from there... Yes, that works.

... as far as rebasing is concerned, but when it comes to compiling, the 
patches seem to break the arm-softmmu and aarch64-softmmu targets:

  hw/pci/pci.c:2474: undefined reference to `pc_machine_get_reserved_memory_end'

This is because patch #2 adds a pc_machine_get_reserved_memory_end() call to 
the generic pci_bus_get_w64_range() function, which is included for other 
targets as well.


Anyway, for now I'm testing with the x86_64-softmmu target. So this is my QEMU 
command line:

  VIRTIO10=1
  ISO=/mnt/data/isos/fedora/23/Fedora-Live-Workstation-x86_64-23-10.iso
  CODE=/home/virt-images/OVMF_CODE.fd
  TMPL=/home/virt-images/OVMF_VARS.fd
  TFTP=/var/lib/dnsmasq
  BF=shim.efi

  if [ $VIRTIO10 -eq 0 ]; then
    MODERN=disable-legacy=off,disable-modern=on
  else
    MODERN=disable-legacy=on,disable-modern=off
  fi

  cp $TMPL vars3.fd

  /opt/qemu-installed/bin/qemu-system-x86_64 \
    -m 2048 \
    -M pc \
    -enable-kvm \
    -device qxl-vga \
    -drive if=pflash,readonly,format=raw,file=$CODE \
    -drive if=pflash,format=raw,file=vars3.fd \
    -drive id=cdrom,if=none,readonly,format=raw,cache=writethrough,file=$ISO \
    \
    -debugcon file:debug3.log \
    -global isa-debugcon.iobase=0x402 \
    \
    -chardev stdio,signal=off,mux=on,id=char0 \
    -mon chardev=char0,mode=readline,default \
    -serial chardev:char0 \
    \
    -device pxb,bus=pci.0,id=bridge1,bus_nr=11 \
    -device pxb,bus=pci.0,id=bridge2,bus_nr=7 \
    -device pxb,bus=pci.0,id=bridge3,bus_nr=15 \
    \
    -device virtio-scsi-pci,id=scsi0,bus=bridge1 \
    -device scsi-cd,bus=scsi0.0,drive=cdrom,bootindex=1 \
    \
    -netdev 
user,id=netdev0,hostfwd=tcp:127.0.0.1:2227-:22,tftp=$TFTP,bootfile=$BF \
    -device 
virtio-net-pci,netdev=netdev0,romfile=,bootindex=2,bus=bridge1,addr=2 \
    \
    -netdev user,id=netdev1,hostfwd=tcp:127.0.0.1:2228-:22 \
    -device 
virtio-net-pci,netdev=netdev1,romfile=,$MODERN,bootindex=3,bus=bridge2,addr=3 \
    \
    -netdev user,id=netdev2,hostfwd=tcp:127.0.0.1:2229-:22 \
    -device 
virtio-net-pci,netdev=netdev2,romfile=,$MODERN,bootindex=4,bus=bridge3,addr=4 \
    \
    -global PIIX4_PM.disable_s3=0

Without your patches, the guest dmesg contains the messages

  pci 0000:0f:00.0: can't claim BAR 15 [mem 0x800800000-0x800ffffff 64bit 
pref]: no compatible bridge window
  pci 0000:07:00.0: can't claim BAR 15 [mem 0x800000000-0x8007fffff 64bit 
pref]: no compatible bridge window
  pci 0000:10:04.0: can't claim BAR 4 [mem 0x800800000-0x800ffffff 64bit pref]: 
no compatible bridge window
  pci 0000:08:03.0: can't claim BAR 4 [mem 0x800000000-0x8007fffff 64bit pref]: 
no compatible bridge window

With your patches, those messages are gone, so that looks fine.

Comparing the decompiled DSDTs, before and after, in the _CRS objects of the 
PC0F and PC07 devices (see above "id=bridge2,bus_nr=7" and 
"id=bridge3,bus_nr=15"), the truncation is fixed; the DWordMemory entries are 
replaced with the right QWordMemory entries. Great.

I also see some changes in the _CRS of the PCI0 device:

             DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
NonCacheable, ReadWrite,
                 0x00000000,         // Granularity
                 0x88400000,         // Range Minimum
-                0xFFFFFFFF,         // Range Maximum
+                0xFEBFFFFF,         // Range Maximum
                 0x00000000,         // Translation Offset
-                0x77C00000,         // Length
+                0x76800000,         // Length

This change looks correct (the IO-APIC area starts at 0xFEC00000), but I can't 
connect it to your patches. Can you explain what code does this?

+                ,, , AddressRangeMemory, TypeStatic)
+            QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
Cacheable, ReadWrite,
+                0x0000000000000000, // Granularity
+                0x0000000100000000, // Range Minimum
+                0x00000007FFFFFFFF, // Range Maximum
+                0x0000000000000000, // Translation Offset
+                0x0000000700000000, // Length
+                ,, , AddressRangeMemory, TypeStatic)
+            QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
Cacheable, ReadWrite,
+                0x0000000000000000, // Granularity
+                0x0000000801000000, // Range Minimum
+                0x000000FFFFFFFFFF, // Range Maximum
+                0x0000000000000000, // Translation Offset
+                0x000000F7FF000000, // Length
                 ,, , AddressRangeMemory, TypeStatic)
         })

This looks like the range from 4GB to 1TB, from which the allocated MMIO64 
resources have been punched out. This is the room for the hotplugged devices, 
right?

Thanks,
Laszlo