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

[Laszlo Ersek]

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.

Indeed, this mistake in the commit message briefly crossed my mind, some
time after posting the series. Then I promptly forgot about it again. :/

(Because, if SeaBIOS really never picked 64-bit addresses, then commit
9fa99d2519cb would have been mostly untestable, in the first place.)

Thank you for pointing this out!

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

OK, I think I'll have to do those ivshmem tests, and rework the commit
message a bit.

Thanks!
Laszlo

> 
> Alex
> 
>> (Which is in
>> turn why ACPI test data for the "bios-tables-test" need not be refreshed.)
>>
>> Using an i440fx OVMF guest with 5GB RAM, an example _CRS change is:
>>
>>> @@ -881,9 +881,9 @@
>>>              QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
>>> Cacheable, ReadWrite,
>>>                  0x0000000000000000, // Granularity
>>>                  0x0000000800000000, // Range Minimum
>>> -                0x000000080001C0FF, // Range Maximum
>>> +                0x000000087FFFFFFF, // Range Maximum
>>>                  0x0000000000000000, // Translation Offset
>>> -                0x000000000001C100, // Length
>>> +                0x0000000080000000, // Length
>>>                  ,, , AddressRangeMemory, TypeStatic)
>>>          })
>>>          Device (GPE0)  
>>
>> (On i440fx, the low RAM split is at 3GB, in this case. Therefore, with 5GB
>> guest RAM and no DIMM hotplug range, pc_pci_hole64_start() returns 4 +
>> (5-3) = 6 GB. Adding the 2GB extension to that yields 8GB, which is below
>> the firmware-programmed base of 32GB, before the patch. Therefore, before
>> the patch, the extension is ineffective. After the patch, we add the 2GB
>> extension to the firmware-programmed base, namely 32GB.)
>>
>> Using a q35 OVMF guest with 5GB RAM, an example _CRS change is:
>>
>>> @@ -3162,9 +3162,9 @@
>>>              QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
>>> Cacheable, ReadWrite,
>>>                  0x0000000000000000, // Granularity
>>>                  0x0000000800000000, // Range Minimum
>>> -                0x00000009BFFFFFFF, // Range Maximum
>>> +                0x0000000FFFFFFFFF, // Range Maximum
>>>                  0x0000000000000000, // Translation Offset
>>> -                0x00000001C0000000, // Length
>>> +                0x0000000800000000, // Length
>>>                  ,, , AddressRangeMemory, TypeStatic)
>>>          })
>>>          Device (GPE0)  
>>
>> (On Q35, the low RAM split is at 2GB. Therefore, with 5GB guest RAM and no
>> DIMM hotplug range, pc_pci_hole64_start() returns 4 + (5-2) = 7 GB. Adding
>> the 32GB extension to that yields 39GB (0x0000_0009_BFFF_FFFF + 1), before
>> the patch. After the patch, we add the 32GB extension to the
>> firmware-programmed base, namely 32GB.)
>>
>> Cc: "Michael S. Tsirkin" <address@hidden>
>> Cc: Alex Williamson <address@hidden>
>> Cc: Marcel Apfelbaum <address@hidden>
>> Link: http://mid.mail-archive.com/address@hidden
>> Fixes: 9fa99d2519cbf71f871e46871df12cb446dc1c3e
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  hw/pci-host/piix.c | 2 +-
>>  hw/pci-host/q35.c  | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index 0df91e002076..fb4b0669ac9f 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -285,7 +285,7 @@ static void i440fx_pcihost_get_pci_hole64_end(Object 
>> *obj, Visitor *v,
>>  {
>>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
>>      I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>> -    uint64_t hole64_start = pc_pci_hole64_start();
>> +    uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
>>      Range w64;
>>      uint64_t value, hole64_end;
>>  
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 8acf942b5e65..1c5433161f14 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -145,7 +145,7 @@ static void q35_host_get_pci_hole64_end(Object *obj, 
>> Visitor *v,
>>  {
>>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
>>      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>> -    uint64_t hole64_start = pc_pci_hole64_start();
>> +    uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj);
>>      Range w64;
>>      uint64_t value, hole64_end;
>>  
>