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

[Marcel Apfelbaum <marcel@redhat.com>]

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 <imammedo@redhat.com>
> 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
>