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