Re: [Qemu-devel] 64-bit MMIO aperture expansion

[Laszlo Ersek]

On 09/21/18 17:01, Marcel Apfelbaum wrote:
> On 09/20/2018 05:49 PM, Laszlo Ersek wrote:

> I had to read this mail a few times...

Sorry :)


>> Now consider the following scenario:
>>
>> - the firmware programs some BARs with 64-bit addresses such that the
>>    aperture that we deduce starts at 32GB,
>>
>> - the guest has 4GB of RAM, and no DIMM hotplug range.
>>
>> Consequences:
>>
>> - Because the "32-bit RAM split" for Q35 is at 2GB, the
>>    pc_pci_hole64_start() function will return 6GB.
>>
>> - The q35_host_get_pci_hole64_start() function will return 32GB. (It
>>    will not fall back to pc_pci_hole64_start() -- correctly --
>>    because the firmware has programmed some BARs with 64-bit
>>    addresses.)
>>
>> - The q35_host_get_pci_hole64_end() function *intends* to return
>>    64GB, because -- let's say -- the guest assigned BARs covering the
>>    32GB..34GB range, which is 2GB in size, and we *intend* to round
>>    that size up to 32GB, so that 30GB be left for hotplug purposes.
>>    (This is the original intent of commit 9fa99d2519cb.)
>> - However, because we initialize "hole64_start" from
>>    pc_pci_hole64_start(), and not from
>>    q35_host_get_pci_hole64_start(), we add "mch.pci_hole64_size"
>>    (32GB by default) to 6GB (the end of RAM), and not to 32GB (the
>>    aperture base deduced from the firmware's programming). As a
>>    result, we'll extend the aperture end address only to 38GB, and
>>    not to 64GB.
>
> Right, there is no reason to use pc_pci_hole64_start, it looks
> like a plain bug. We diverged from pc and the fact that
> q35_host_get_pci_hole64_star uses it is only an implementation
> detail.

Small correction (not affecting the main point):

If you compare q35_host_get_pci_hole64_end() and
i440fx_pcihost_get_pci_hole64_end(), you see that they do the exact same
thing, and pc_pci_hole64_start() is a common helper function that they
both call.

This is also matched by the files that define these functions:
- i440fx_pcihost_get_pci_hole64_end(): hw/pci-host/piix.c
- q35_host_get_pci_hole64_end():       hw/pci-host/q35.c
- pc_pci_hole64_start():               hw/i386/pc.c

In that sense, we didn't "diverge" from PC. Because, both i440fx and q35
received the exact same logic in 9fa99d2519cb. They both call the common
pc_pci_hole64_start() helper function as an internal / implementation
detail. (And both of them should be fixed.)

Current function call chains:

  i440fx_pcihost_get_pci_hole64_start()                    | 
q35_host_get_pci_hole64_start()
    pc_pci_hole64_start()                 [good call]      |   
pc_pci_hole64_start()           [good call]
                                                           |
  i440fx_pcihost_get_pci_hole64_end()                      | 
q35_host_get_pci_hole64_end()
    pc_pci_hole64_start()                 [bug]            |   
pc_pci_hole64_start()           [bug]

Proposed call chains:

  i440fx_pcihost_get_pci_hole64_start()                    | 
q35_host_get_pci_hole64_start()
    pc_pci_hole64_start()                 [unchanged call] |   
pc_pci_hole64_start()           [unchanged call]
                                                           |
  i440fx_pcihost_get_pci_hole64_end()                      | 
q35_host_get_pci_hole64_end()
    i440fx_pcihost_get_pci_hole64_start() [corrected call] |   
q35_host_get_pci_hole64_start() [corrected call]
      pc_pci_hole64_start()               [unchanged call] |     
pc_pci_hole64_start()         [unchanged call]


>> The same would apply to i440fx too.
>
> I am lost here. The q35 PCI 64bit hole computation issue starts from
> the miss-use of the PC conterpart functions.

I disagree. If q35 called an i440fx-specific function, that would indeed
be mis-use. However, in this case, the callee -- that is,
pc_pci_hole64_start() -- is not an i440fx-specific function. It is a
common helper for both boards. Both boards can rightfully call it.

Instead, the issue here is that *both*
i440fx_pcihost_get_pci_hole64_end() and q35_host_get_pci_hole64_end()
need a "hole64_start" value that accounts for the BAR addresses that
were programmed by the firmware. pc_pci_hole64_start() simply doesn't do
that.

> What is the problem with the PC?

i440fx_pcihost_get_pci_hole64_end() currently calls
pc_pci_hole64_start() directly, so the "hole64_start" value does not
honor the BAR addresses set by the firmware.

Thanks,
Laszlo