From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 16 May 2019 12:15:44 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 980F43E2CD; Thu, 16 May 2019 19:15:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-88.rdu2.redhat.com [10.10.121.88]) by smtp.corp.redhat.com (Postfix) with ESMTP id 66F285D98E; Thu, 16 May 2019 19:15:37 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/4] OvmfPkg/PlatformPei: reorder the 32-bit PCI hole vs. the PCIEXBAR on q35 To: devel@edk2.groups.io, philmd@redhat.com, Ard Biesheuvel Cc: Gerd Hoffmann , Jordan Justen References: <20190504000716.7525-1-lersek@redhat.com> <20190504000716.7525-4-lersek@redhat.com> <8aff8ca3-85d8-e334-1c3e-d9536d4e26ff@redhat.com> From: "Laszlo Ersek" Message-ID: <537eb745-b803-b55d-514a-843b2faf4b02@redhat.com> Date: Thu, 16 May 2019 21:15:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 16 May 2019 19:15:43 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 05/16/19 14:24, Philippe Mathieu-Daud=C3=A9 wrote: > Hi Laszlo, >=20 > 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 wrote: >>>> >>>> Commit 7b8fe63561b4 ("OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCO= NFIG >>>> / 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 t= he >>>> 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 <=3D PciExBarBase); >>>> >>>> in MemMapInitialization(), because we placed the 256MB PCIEXBAR at >>>> 0x8000_0000 (2GB) exactly, relying on the interface contract. (And, t= he >>>> 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 PCIEXBA= R, as >>>> follows: >>>> >>>> - start the 32-bit PCI hole where it starts on i440fx as well, that i= s, at >>>> 2GB or TopOfLowRam, whichever is higher; >>>> >>>> - unlike on i440fx, where the 32-bit PCI hole extends up to 0xFC00_00= 00, >>>> 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 a= rea >>>> 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) betwe= en >>>> both boards. >>>> >>>> On q35, the maximal hole shrinks from >>>> >>>> 0xFC00_0000 - 0x9000_0000 =3D 0x6C00_0000 =3D=3D 1728 MB >>>> >>>> to >>>> >>>> 0xE000_0000 - 0x8000_0000 =3D=3D 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 i= s >>>> >>>> 0xE000_0000 - 0xAFF0_0000 =3D 769 MB >>>> >>>> which is not great, but probably better than a failed ASSERT. >>>> >>>> Cc: Ard Biesheuvel >>>> Cc: Gerd Hoffmann >>>> Cc: Jordan Justen >>>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=3D1666941 >>>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=3D1701710 >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Laszlo Ersek >>> >>> 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. >=20 > Maybe you can amend that comment, ... >=20 >> >>> 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 >=20 > ... and this one in the commit description. >=20 > Regardless: > Reviewed-by: Philippe Mathieu-Daude 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 >> >> 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 hier= archy. It >>>> # is only consulted when OVMF runs on Q35. In that case it is prog= rammed into >>>> # the PCIEXBAR register. >>>> - # >>>> - # On Q35 machine types that QEMU intends to support in the long te= rm, 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 hier= archy. It >>>> # is only consulted when OVMF runs on Q35. In that case it is prog= rammed into >>>> # the PCIEXBAR register. >>>> - # >>>> - # On Q35 machine types that QEMU intends to support in the long te= rm, 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 hier= archy. It >>>> # is only consulted when OVMF runs on Q35. In that case it is prog= rammed into >>>> # the PCIEXBAR register. >>>> - # >>>> - # On Q35 machine types that QEMU intends to support in the long te= rm, 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/Pla= tform.c >>>> index 9c013613a1a0..fd8eccaf3e50 100644 >>>> --- a/OvmfPkg/PlatformPei/Platform.c >>>> +++ b/OvmfPkg/PlatformPei/Platform.c >>>> @@ -184,14 +184,13 @@ MemMapInitialization ( >>>> PciBase =3D (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam; >>>> if (mHostBridgeDevId =3D=3D INTEL_Q35_MCH_DEVICE_ID) { >>>> // >>>> - // The MMCONFIG area is expected to fall between the top of lo= w RAM and >>>> - // the base of the 32-bit PCI host aperture. >>>> + // The 32-bit PCI host aperture is expected to fall between th= e top of >>>> + // low RAM and the base of the MMCONFIG area. >>>> // >>>> PciExBarBase =3D FixedPcdGet64 (PcdPciExpressBaseAddress); >>>> - ASSERT (TopOfLowRam <=3D PciExBarBase); >>>> + ASSERT (PciBase < PciExBarBase); >>>> ASSERT (PciExBarBase <=3D MAX_UINT32 - SIZE_256MB); >>>> - PciBase =3D (UINT32)(PciExBarBase + SIZE_256MB); >>>> - PciSize =3D 0xFC000000 - PciBase; >>>> + PciSize =3D (UINT32)(PciExBarBase - PciBase); >>>> } else { >>>> PciSize =3D 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/39= 968 >>>> Mute This Topic: https://groups.io/mt/31489698/1761188 >>>> Group Owner: devel+owner@edk2.groups.io >>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ard.biesheuvel@li= naro.org] >>>> ------------ >>>> >> >> >> >> >=20 >=20 >=20