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

[Marcel Apfelbaum]

On 05/09/2016 07:50 PM, Laszlo Ersek wrote:
> 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.

Hi Laszlo,

Sorry for all that, I'll rebase and re-send. Next time just ping me and I'll do 
the homework.

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

Good!

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


The only interesting patch with regard to the above range is Patch 4/4. 
However, it deals only with 64-bit MMIO ranges.
Looking closer, it is possible we had a bug before this patch: The code assumed 
all the ranges assigned to PXBs are 32-bit. Adding a 64-bit
may cause a mismatch beyond the wrong ACPI table.

The way it works is (for each resource type IO/MEM/ and now 64-bit MEM):
1. build_crs is called for each PXB, the firmware assigned ranges are added 
placed temp list CrsRangeSet
2. we call crs_range_merge to try to merge adjacent ranges to shrink the CRS 
(we don't want a resource for each devices)
3. finally we add the ranges to the ACPI tables
4. once all the PXBs are 'done', we call  crs_replace_with_free_ranges to replace the 
ranges "used" by PXBs with free ones
   to be assigned to bus 0.
5. We add the bus 0 resources to ACPI tables.

It is possible that steps 2 and/or 4 may be affected by (not anticipated) 
64-bit ranges, but this patch completely separates them.


> +                ,, , 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?

Exactly. For the moment all this range is allocated to bus 0, but once PXBs 
will support hotplug we'll need to divide it somehow between them.
I tried to hotplug an ivshmem device with a huge BAR (sadly I don't have a real 
device) and after this series it can be hot-plugged like a charm.

Thanks,
Marcel
> Thanks,
> Laszlo