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

[Alex Williamson]

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,

Alex