[edk2-devel] [PATCH 3/4] OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on q35
Laszlo Ersek
lersek at redhat.com
Thu May 16 19:15:36 UTC 2019
On 05/16/19 14:24, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
>
> On 5/16/19 2:18 PM, Laszlo Ersek wrote:
>> On 05/16/19 10:00, Ard Biesheuvel wrote:
>>> On Sat, 4 May 2019 at 02:07, Laszlo Ersek <lersek at redhat.com> wrote:
>>>>
>>>> Commit 7b8fe63561b4 ("OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCONFIG
>>>> / ECAM) on Q35", 2016-03-10) claimed that,
>>>>
>>>> On Q35 machine types that QEMU intends to support in the long term, QEMU
>>>> never lets the RAM below 4 GB exceed 2 GB.
>>>>
>>>> Alas, this statement came from a misunderstanding that occurred while we
>>>> worked out the interface contract. In fact QEMU does allow the 32-bit RAM
>>>> extend up to 0xB000_0000 (exclusive), in case the RAM size falls in the
>>>> range (0x8000_0000, 0xB000_0000) (i.e., the RAM size is greater than
>>>> 2048MB and smaller than 2816MB).
>>>>
>>>> In turn, such a RAM size (justifiedly) triggers
>>>>
>>>> ASSERT (TopOfLowRam <= PciExBarBase);
>>>>
>>>> in MemMapInitialization(), because we placed the 256MB PCIEXBAR at
>>>> 0x8000_0000 (2GB) exactly, relying on the interface contract. (And, the
>>>> 32-bit PCI hole would follow the PCIEXBAR, covering the [0x9000_0000,
>>>> 0xFC00_0000) range.)
>>>>
>>>> In order to fix this, reorder the 32-bit PCI hole against the PCIEXBAR, as
>>>> follows:
>>>>
>>>> - start the 32-bit PCI hole where it starts on i440fx as well, that is, at
>>>> 2GB or TopOfLowRam, whichever is higher;
>>>>
>>>> - unlike on i440fx, where the 32-bit PCI hole extends up to 0xFC00_0000,
>>>> stop it at 0xE000_0000 on q35,
>>>>
>>>> - place the PCIEXBAR at 0xE000_0000.
>>>>
>>>> (We cannot place the PCIEXBAR at 0xF000_0000 because the 256MB MMIO area
>>>> that starts there is not entirely free.)
>>>>
>>>> Before this patch, the 32-bit PCI hole used to only *end* at the same spot
>>>> (namely, 0xFC00_0000) between i440fx and q35; now it will only *start* at
>>>> the same spot (namely, 2GB or TopOfLowRam, whichever is higher) between
>>>> both boards.
>>>>
>>>> On q35, the maximal hole shrinks from
>>>>
>>>> 0xFC00_0000 - 0x9000_0000 = 0x6C00_0000 == 1728 MB
>>>>
>>>> to
>>>>
>>>> 0xE000_0000 - 0x8000_0000 == 1536 MB.
>>>>
>>>> We lose 192 MB of the aperture; however, the aperture is now aligned at
>>>> 1GB, rather than 256 MB, and so it could fit a 1GB BAR even.
>>>>
>>>> Regarding the minimal hole (triggered by RAM size 2815MB), its size is
>>>>
>>>> 0xE000_0000 - 0xAFF0_0000 = 769 MB
>>>>
>>>> which is not great, but probably better than a failed ASSERT.
>>>>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>>>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>>>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>>>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1666941
>>>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1701710
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>>>
>>> I take it the 32-bit PCI 'hole' is in fact the 32-bit MMIO BAR window?
>>
>> Yes, that's correct. We also call it "32-bit PCI MMIO aperture",
>> sometimes. The interval that PciHostBridgeLib passes to
>> PciHostBridgeDxe, to allocate BARs out of, when PciBusDxe asks.
>
> Maybe you can amend that comment, ...
>
>>
>>> That could do with a clarification. I guess it may be an x86-ism to
>>> consider any physical memory region not used by RAM to be a hole, but
>>> it is not terribly helpful.
>>
>> The "PCI hole" expressions is a QEMU-ism; it is frequently used in
>
> ... and this one in the commit description.
>
> Regardless:
> Reviewed-by: Philippe Mathieu-Daude <philmd at redhat.com>
I've replaced all occurrences of the word "hole" in this commit message
(including the subject line) with "window". That allowed me to fit into
74 characters even with the modified subject.
Thanks!
Laszlo
>> connection with the i440fx and q35 (x86) machine types, and not much
>> elsewhere:
>>
>> $ git grep -c pci_hole
>>
>> hw/i386/acpi-build.c:10
>> hw/i386/pc.c:1
>> hw/pci-host/grackle.c:3
>> hw/pci-host/piix.c:23
>> hw/pci-host/q35.c:24
>> hw/pci-host/uninorth.c:4
>> include/hw/i386/pc.h:1
>> include/hw/pci-host/q35.h:3
>> include/hw/pci-host/uninorth.h:1
>>
>>> In any case,
>>>
>>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>>
>> Thank you!
>> Laszlo
>>
>>>
>>>> ---
>>>> OvmfPkg/OvmfPkgIa32.dsc | 5 +----
>>>> OvmfPkg/OvmfPkgIa32X64.dsc | 5 +----
>>>> OvmfPkg/OvmfPkgX64.dsc | 5 +----
>>>> OvmfPkg/PlatformPei/Platform.c | 9 ++++-----
>>>> 4 files changed, 7 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>>> index 36a0f87258dd..bb55b6fa58e1 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>>> @@ -491,10 +491,7 @@ [PcdsFixedAtBuild]
>>>> # This PCD is used to set the base address of the PCI express hierarchy. It
>>>> # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>>> # the PCIEXBAR register.
>>>> - #
>>>> - # On Q35 machine types that QEMU intends to support in the long term, QEMU
>>>> - # never lets the RAM below 4 GB exceed 2 GB.
>>>> - gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>>>> + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>>>
>>>> !ifdef $(SOURCE_DEBUG_ENABLE)
>>>> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> index 9b341e17d7ff..06c394a6fb1f 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>>>> # This PCD is used to set the base address of the PCI express hierarchy. It
>>>> # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>>> # the PCIEXBAR register.
>>>> - #
>>>> - # On Q35 machine types that QEMU intends to support in the long term, QEMU
>>>> - # never lets the RAM below 4 GB exceed 2 GB.
>>>> - gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>>>> + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>>>
>>>> !ifdef $(SOURCE_DEBUG_ENABLE)
>>>> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>>> index a0f87f74dab9..5e0eb043fab9 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>> @@ -496,10 +496,7 @@ [PcdsFixedAtBuild]
>>>> # This PCD is used to set the base address of the PCI express hierarchy. It
>>>> # is only consulted when OVMF runs on Q35. In that case it is programmed into
>>>> # the PCIEXBAR register.
>>>> - #
>>>> - # On Q35 machine types that QEMU intends to support in the long term, QEMU
>>>> - # never lets the RAM below 4 GB exceed 2 GB.
>>>> - gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x80000000
>>>> + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>>>>
>>>> !ifdef $(SOURCE_DEBUG_ENABLE)
>>>> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>>>> index 9c013613a1a0..fd8eccaf3e50 100644
>>>> --- a/OvmfPkg/PlatformPei/Platform.c
>>>> +++ b/OvmfPkg/PlatformPei/Platform.c
>>>> @@ -184,14 +184,13 @@ MemMapInitialization (
>>>> PciBase = (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam;
>>>> if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>>>> //
>>>> - // The MMCONFIG area is expected to fall between the top of low RAM and
>>>> - // the base of the 32-bit PCI host aperture.
>>>> + // The 32-bit PCI host aperture is expected to fall between the top of
>>>> + // low RAM and the base of the MMCONFIG area.
>>>> //
>>>> PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
>>>> - ASSERT (TopOfLowRam <= PciExBarBase);
>>>> + ASSERT (PciBase < PciExBarBase);
>>>> ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
>>>> - PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
>>>> - PciSize = 0xFC000000 - PciBase;
>>>> + PciSize = (UINT32)(PciExBarBase - PciBase);
>>>> } else {
>>>> PciSize = 0xFC000000 - PciBase;
>>>> }
>>>> --
>>>> 2.19.1.3.g30247aa5d201
>>>>
>>>>
>>>>
>>>> ------------
>>>> Groups.io Links: You receive all messages sent to this group.
>>>>
>>>> View/Reply Online (#39968): https://edk2.groups.io/g/devel/message/39968
>>>> Mute This Topic: https://groups.io/mt/31489698/1761188
>>>> Group Owner: devel+owner at edk2.groups.io
>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ard.biesheuvel at linaro.org]
>>>> ------------
>>>>
>>
>>
>>
>>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#40825): https://edk2.groups.io/g/devel/message/40825
Mute This Topic: https://groups.io/mt/31489698/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list