Re: [Qemu-devel] [PATCH 2/2] hw/pci-host/x86: extend the 64-bit PCI hole relative to the fw-assigned base

[Laszlo Ersek]

(+Eric)

On 09/26/18 18:26, Alex Williamson wrote:
> On Wed, 26 Sep 2018 13:12:47 +0200
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 09/25/18 22:36, Alex Williamson wrote:
>>> On Tue, 25 Sep 2018 00:13:46 +0200
>>> Laszlo Ersek <address@hidden> wrote:
>>>   
>>>> In commit 9fa99d2519cb ("hw/pci-host: Fix x86 Host Bridges 64bit PCI
>>>> hole", 2017-11-16), we meant to expose such a 64-bit PCI MMIO aperture in
>>>> the ACPI DSDT that would be at least as large as the new "pci-hole64-size"
>>>> property (2GB on i440fx, 32GB on q35). The goal was to offer "enough"
>>>> 64-bit MMIO aperture to the guest OS for hotplug purposes.
>>>>
>>>> In that commit, we added or modified five functions:
>>>>
>>>> - pc_pci_hole64_start(): shared between i440fx and q35. Provides a default
>>>>   64-bit base, which starts beyond the cold-plugged 64-bit RAM, and skips
>>>>   the DIMM hotplug area too (if any).
>>>>
>>>> - i440fx_pcihost_get_pci_hole64_start(), q35_host_get_pci_hole64_start():
>>>>   board-specific 64-bit base property getters called abstractly by the
>>>>   ACPI generator. Both of these fall back to pc_pci_hole64_start() if the
>>>>   firmware didn't program any 64-bit hole (i.e. if the firmware didn't
>>>>   assign a 64-bit GPA to any MMIO BAR on any device). Otherwise, they
>>>>   honor the firmware's BAR assignments (i.e., they treat the lowest 64-bit
>>>>   GPA programmed by the firmware as the base address for the aperture).
>>>>
>>>> - i440fx_pcihost_get_pci_hole64_end(), q35_host_get_pci_hole64_end():
>>>>   these intended to extend the aperture to our size recommendation,
>>>>   calculated relative to the base of the aperture.
>>>>
>>>> Despite the original intent, i440fx_pcihost_get_pci_hole64_end() and
>>>> q35_host_get_pci_hole64_end() currently only extend the aperture relative
>>>> to the default base (pc_pci_hole64_start()), ignoring any programming done
>>>> by the firmware. This means that our size recommendation may not be met.
>>>> Fix it by honoring the firmware's address assignments.
>>>>
>>>> The strange extension sizes were spotted by Alex, in the log of a guest
>>>> kernel running on top of OVMF (which does assign 64-bit GPAs to BARs).
>>>>
>>>> This change only affects DSDT generation, therefore no new compat property
>>>> is being introduced. Also, because SeaBIOS never assigns 64-bit GPAs to
>>>> 64-bit BARs, the patch makes no difference to SeaBIOS guests.  
>>>
>>> This is not exactly true, SeaBIOS will make use of 64-bit MMIO, but
>>> only if it cannot satisfy all the BARs from 32-bit MMMIO, see
>>> src/fw/pciinit.c:pci_bios_map_devices.  Create a VM with several
>>> assigned GPUs and you'll eventually cross that threshold and all 64-bit
>>> BARs will be moved above 4G.  I'm sure a few sufficiently sized ivshmem
>>> devices could do the same.  Thanks,  
>>
>> The effect of this patch is not hard to demonstrate with SeaBIOS+Q35,
>> when using e.g. 5GB of guest RAM and a 4GB ivshmem-plain device.
>>
>> However, using SeaBIOS+i440fx, I can't show the difference. I've been
>> experimenting with various ivshmem devices (even multiple at the same
>> time, with different sizes). The "all or nothing" nature of SeaBIOS's
>> high allocation of the 64-bit BARs, combined with hugepage alignment
>> inside SeaBIOS, combined with the small (2GB) rounding size used in QEMU
>> for i440fx, seem to make it surprisingly difficult to trigger the issue.
>>
>> I figure I should:
>>
>> (1) remove the sentence "the patch makes no difference to SeaBIOS
>> guests" from the commit message,
>>
>> (2) include the DSDT diff on SeaBIOS/q35 in the commit message,
>>
>> (3) remain silent on SeaBIOS/i440fx, in the commit message,
>>
>> (4) append a new patch, for "bios-tables-test", so that the ACPI gen
>> change is validated as part of the test suite, on SeaBIOS/q35.
>>
>> Regarding (4):
>>
>> - is it OK if I add the test only for Q35?
>>
>> - what guest RAM size am I allowed to use in the test suite? In my own
>> SeaBIOS/Q35 reproducer I currently use 5GB, but I'm not sure if it's
>> acceptable for the test suite.
> 
> Seems like you've done due diligence, the plan looks ok to me.
> Regarding the test memory allocation, is it possible and reasonable to
> perhaps create a 256MB shared memory area and re-use it for multiple
> ivshmem devices?  ie. rather than 1, 4GB ivshmem device, use 16, 256MB
> devices, all with the same backing.  Thanks,

This too sounds useful. AIUI, ftruncate() is neither forbidden, nor
required, to allocate filesystem extents when increasing the size of a
file. Using one smaller regular temporary file as the common foundation
for multiple "memory-backend-file" objects will save space on the fs if
ftruncate() happens to allocate extents.

(I've also thought of passing the same "memory-backend-file" object to
multiple ivshmem-plain devices, but ivshmem_plain_realize()
[hw/misc/ivshmem.c] checks whether the HostMemoryBackend is already mapped.)

Thanks!
Laszlo