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