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; Wed, 08 May 2019 02:10:47 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5CAB53097020; Wed, 8 May 2019 09:10:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-255.rdu2.redhat.com [10.10.120.255]) by smtp.corp.redhat.com (Postfix) with ESMTP id 55C4E176B8; Wed, 8 May 2019 09:10:36 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 4/4] OvmfPkg/PlatformPei: fix MTRR for low-RAM sizes that have many bits clear To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , devel@edk2.groups.io Cc: Ard Biesheuvel , Gerd Hoffmann , Jordan Justen References: <20190504000716.7525-1-lersek@redhat.com> <20190504000716.7525-5-lersek@redhat.com> <46dccf0d-6ab6-11e6-d473-187cf8ce7114@redhat.com> From: "Laszlo Ersek" Message-ID: <4680d4c5-6028-dee6-10d4-023289bf4dc8@redhat.com> Date: Wed, 8 May 2019 11:10: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: <46dccf0d-6ab6-11e6-d473-187cf8ce7114@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.43]); Wed, 08 May 2019 09:10:46 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi Phil, On 05/08/19 09:33, Philippe Mathieu-Daud=C3=A9 wrote: > On 5/4/19 2:07 AM, Laszlo Ersek wrote: >> Assume that we boot OVMF in a QEMU guest with 1025 MB of RAM. The >> following assertion will fire: >> >>> ASSERT_EFI_ERROR (Status =3D Out of Resources) >>> ASSERT OvmfPkg/PlatformPei/MemDetect.c(696): !EFI_ERROR (Status) >> >> That's because the range [1025 MB, 4 GB) that we try to mark as >> uncacheable with MTRRs has size 3071 MB: >> >> 0x1_0000_0000 >> -0x0_4010_0000 >> -------------- >> 0x0_BFF0_0000 >> >> The integer that stands for the uncacheable area size has 11 (eleven) = bits >> set to 1. As a result, covering this size requires 11 variable MTRRs (= each >> MTRR must cover a naturally aligned, power-of-two sized area). But, if= we >> need more variable MTRRs than the CPU can muster (such as 8), then >> MtrrSetMemoryAttribute() fails, and we refuse to continue booting (whi= ch >> is justified, in itself). >> >> Unfortunately, this is not difficult to trigger, and the error message= is >> well-hidden from end-users, in the OVMF debug log. The following >> mitigation is inspired by SeaBIOS: >> >> Truncate the uncacheable area size to a power-of-two, while keeping th= e >> end fixed at 4 GB. Such an interval can be covered by just one variabl= e >> MTRR. >> >> This may leave such an MMIO gap, between the end of low-RAM and the st= art >> of the uncacheable area, that is marked as WB (through the MTRR defaul= t). >> Raise the base of the 32-bit PCI MMIO aperture accordingly -- the gap = will >> not be used for anything. >=20 > I had to draw it to be sure I understood correctly: >=20 > +-------------+ +-------------+ <-- 4GB > | | | | > | | | | > | | | PCI MMIO | > | | | | > | | | uncacheable | > | uncacheable | | | > | | | | > | | ----> +-------------+ <-- mQemuUc32Base > | | | | | (pow2 aligned) > | | | | GAP | > | | | | (cacheable) | > +-------------+ ---- +-------------+ <-- TopOfLowRam > | | | | (not pow2 aligned) > | | | | > | | | | > | | | | > | LowerMemory | | LowerMemory | > | (cacheable) | | (cacheable) | > | | | | > | | | | > | | | | > +-------------+ +-------------+ Correct. "mQemuUc32Base" is not itself a whole power of two, but it is pow2 aligned, where the alignment must not be smaller than "size" *is*. This natural alignment is ensured because 4GB is itself a power of two (2^32). Thus, if "size" is 2^m, then mQemuUc32Base =3D=3D (4GB - size) =3D=3D (2^32 - 2^m) =3D=3D 2^m * (2^(= 32-m) - 1) and that is divisible by 2^m. (We know for sure that (m < 32).) Therefore the natural alignment for the base is satisfied. For example, consider base=3D3GB, size=3D1GB. Then m=3D30. >=20 >> On Q35, the minimal 32-bit PCI MMIO aperture (triggered by RAM size 28= 15 >> MB) shrinks from >> >> 0xE000_0000 - 0xAFF0_0000 =3D 769 MB >> >> to >> >> 0xE000_0000 - 0xC000_0000 =3D 512 MB >> >> On i440fx, the minimal 32-bit PCI MMIO aperture (triggered by RAM size >> 3583 MB) shrinks from >> >> 0xFC00_0000 - 0xDFF0_0000 =3D 449 MB >> >> to >> >> 0xFC00_0000 - 0xE000_0000 =3D 448 MB >> >> 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 >> Signed-off-by: Laszlo Ersek >> --- >> OvmfPkg/PlatformPei/Platform.h | 2 ++ >> OvmfPkg/PlatformPei/MemDetect.c | 23 +++++++++++++++++--- >> OvmfPkg/PlatformPei/Platform.c | 4 +--- >> 3 files changed, 23 insertions(+), 6 deletions(-) >> >> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Plat= form.h >> index 81af8b71480f..4476ddd871cd 100644 >> --- a/OvmfPkg/PlatformPei/Platform.h >> +++ b/OvmfPkg/PlatformPei/Platform.h >> @@ -114,4 +114,6 @@ extern UINT32 mMaxCpuCount; >> =20 >> extern UINT16 mHostBridgeDevId; >> =20 >> +extern UINT32 mQemuUc32Base; >> + >> #endif // _PLATFORM_PEI_H_INCLUDED_ >> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/Mem= Detect.c >> index e890e36408a6..ae73c63d27d5 100644 >> --- a/OvmfPkg/PlatformPei/MemDetect.c >> +++ b/OvmfPkg/PlatformPei/MemDetect.c >> @@ -42,6 +42,8 @@ STATIC UINT32 mS3AcpiReservedMemorySize; >> =20 >> STATIC UINT16 mQ35TsegMbytes; >> =20 >> +UINT32 mQemuUc32Base; >> + >> VOID >> Q35TsegMbytesInitialization ( >> VOID >> @@ -663,6 +665,8 @@ QemuInitializeRam ( >> // cover it exactly. >> // >> if (IsMtrrSupported ()) { >> + UINT32 Uc32Size; >> + >> MtrrGetAllMtrrs (&MtrrSettings); >> =20 >> // >> @@ -689,11 +693,24 @@ QemuInitializeRam ( >> =20 >> // >> // Set memory range from the "top of lower RAM" (RAM below 4GB) t= o 4GB as >> - // uncacheable >> + // uncacheable. Make sure one variable MTRR suffices by truncatin= g the size >> + // to a whole power of two. This will round the base *up*, and a = gap (not >> + // used for either RAM or MMIO) may stay in the middle, marked as >> + // cacheable-by-default. >> // >> - Status =3D MtrrSetMemoryAttribute (LowerMemorySize, >> - SIZE_4GB - LowerMemorySize, CacheUncacheable); >> + Uc32Size =3D GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize= )); >> + mQemuUc32Base =3D (UINT32)(SIZE_4GB - Uc32Size); >> + if (mQemuUc32Base !=3D LowerMemorySize) { >> + DEBUG ((DEBUG_VERBOSE, "%a: rounded UC32 base from 0x%x up to 0= x%x, for " >> + "an UC32 size of 0x%x\n", __FUNCTION__, (UINT32)LowerMemorySi= ze, >> + mQemuUc32Base, Uc32Size)); >> + } >> + >> + Status =3D MtrrSetMemoryAttribute (mQemuUc32Base, Uc32Size, >> + CacheUncacheable); >> ASSERT_EFI_ERROR (Status); >> + } else { >> + mQemuUc32Base =3D (UINT32)LowerMemorySize; >> } >> } >> =20 >> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Plat= form.c >> index fd8eccaf3e50..c064b4ed9b8f 100644 >> --- a/OvmfPkg/PlatformPei/Platform.c >> +++ b/OvmfPkg/PlatformPei/Platform.c >> @@ -174,14 +174,12 @@ MemMapInitialization ( >> AddIoMemoryRangeHob (0x0A0000, BASE_1MB); >> =20 >> if (!mXen) { >> - UINT32 TopOfLowRam; >> UINT64 PciExBarBase; >> UINT32 PciBase; >> UINT32 PciSize; >> =20 >> - TopOfLowRam =3D GetSystemMemorySizeBelow4gb (); >> PciExBarBase =3D 0; >> - PciBase =3D (TopOfLowRam < BASE_2GB) ? BASE_2GB : TopOfLowRam; >> + PciBase =3D (mQemuUc32Base < BASE_2GB) ? BASE_2GB : mQemuUc32Base= ; >> if (mHostBridgeDevId =3D=3D INTEL_Q35_MCH_DEVICE_ID) { >> // >> // The 32-bit PCI host aperture is expected to fall between the= top of >> >=20 > Reviewed-by: Philippe Mathieu-Daude >=20 Thank you! Laszlo