From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5948481CCC for ; Fri, 4 Nov 2016 15:38:28 -0700 (PDT) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 04 Nov 2016 15:38:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,445,1473145200"; d="scan'208";a="1064148845" Received: from poojakri-mobl.amr.corp.intel.com (HELO localhost) ([10.252.132.157]) by fmsmga001.fm.intel.com with ESMTP; 04 Nov 2016 15:38:28 -0700 MIME-Version: 1.0 To: Laszlo Ersek , =?utf-8?q?Marvin_H=C3=A4user?= , "edk2-devel@lists.01.org" Message-ID: <147829910764.11627.2691984297559573830@jljusten-ivb> From: Jordan Justen In-Reply-To: <53f2a4d0-84b6-6831-9f13-6e1b53d4303a@redhat.com> References: <53f2a4d0-84b6-6831-9f13-6e1b53d4303a@redhat.com> User-Agent: alot/0.3.7 Date: Fri, 04 Nov 2016 15:38:27 -0700 Subject: Re: [PATCH v4] OvmfPkg/ResetVector: Depend on PCD values of the page tables. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Nov 2016 22:38:28 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2016-11-04 15:27:48, Laszlo Ersek wrote: > On 11/04/16 14:32, Marvin H=C3=A4user wrote: > > Currently, the value of the page tables' address is hard-coded in the > > ResetVector. This patch replaces these values with a PCD dependency. > > = > > A check for the size has been added to alert the developer to rewrite > > the ASM according to the new size, if it has been changed. > > = > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Marvin Haeuser > > --- > > OvmfPkg/ResetVector/Ia32/PageTables64.asm | 23 ++++++++++---------- > > OvmfPkg/ResetVector/ResetVector.inf | 5 +++++ > > OvmfPkg/ResetVector/ResetVector.nasmb | 7 ++++++ > > 3 files changed, 24 insertions(+), 11 deletions(-) > > = > > diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetV= ector/Ia32/PageTables64.asm > > index b5a4cf8d7187..6201cad1f5dc 100644 > > --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm > > +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > > @@ -44,10 +44,11 @@ BITS 32 > > SetCr3ForPageTables64: > > = > > ; > > - ; For OVMF, build some initial page tables at 0x800000-0x806000. > > + ; For OVMF, build some initial page tables at > > + ; PcdOvmfSecPageTablesBase - (PcdOvmfSecPageTablesBase + 0x6000). > > ; > > - ; This range should match with PcdOvmfSecPageTablesBase and > > - ; PcdOvmfSecPageTablesSize which are declared in the FDF files. > > + ; This range should match with PcdOvmfSecPageTablesSize which is > > + ; declared in the FDF files. > > ; > > ; At the end of PEI, the pages tables will be rebuilt into a > > ; more permanent location by DxeIpl. > > @@ -56,21 +57,21 @@ SetCr3ForPageTables64: > > mov ecx, 6 * 0x1000 / 4 > > xor eax, eax > > clearPageTablesMemoryLoop: > > - mov dword[ecx * 4 + 0x800000 - 4], eax > > + mov dword[ecx * 4 + PT_ADDR (0) - 4], eax > > loop clearPageTablesMemoryLoop > > = > > ; > > ; Top level Page Directory Pointers (1 * 512GB entry) > > ; > > - mov dword[0x800000], 0x801000 + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (0)], PT_ADDR (0x1000) + PAGE_PDP_ATTR > > = > > ; > > ; Next level Page Directory Pointers (4 * 1GB entries =3D> 4GB) > > ; > > - mov dword[0x801000], 0x802000 + PAGE_PDP_ATTR > > - mov dword[0x801008], 0x803000 + PAGE_PDP_ATTR > > - mov dword[0x801010], 0x804000 + PAGE_PDP_ATTR > > - mov dword[0x801018], 0x805000 + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (0x1000)], PT_ADDR (0x2000) + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (0x1008)], PT_ADDR (0x3000) + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (0x1010)], PT_ADDR (0x4000) + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (0x1018)], PT_ADDR (0x5000) + PAGE_PDP_ATTR > > = > > ; > > ; Page Table Entries (2048 * 2MB entries =3D> 4GB) > > @@ -81,13 +82,13 @@ pageTableEntriesLoop: > > dec eax > > shl eax, 21 > > add eax, PAGE_2M_PDE_ATTR > > - mov [ecx * 8 + 0x802000 - 8], eax > > + mov [ecx * 8 + PT_ADDR (0x2000 - 8)], eax > > loop pageTableEntriesLoop > > = > > ; > > ; Set CR3 now that the paging structures are available > > ; > > - mov eax, 0x800000 > > + mov eax, PT_ADDR (0) > > mov cr3, eax > > = > > OneTimeCallRet SetCr3ForPageTables64 > > diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/= ResetVector.inf > > index 46610d243ecf..d1e5d4d9bdea 100644 > > --- a/OvmfPkg/ResetVector/ResetVector.inf > > +++ b/OvmfPkg/ResetVector/ResetVector.inf > > @@ -29,9 +29,14 @@ [Sources] > > ResetVector.nasmb > > = > > [Packages] > > + OvmfPkg/OvmfPkg.dec > > MdePkg/MdePkg.dec > > UefiCpuPkg/UefiCpuPkg.dec > > = > > [BuildOptions] > > *_*_IA32_NASMB_FLAGS =3D -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/ > > *_*_X64_NASMB_FLAGS =3D -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/ > > + > > +[Pcd] > > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase > > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > > diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVecto= r/ResetVector.nasmb > > index 31ac06ae4a8c..29cbad367711 100644 > > --- a/OvmfPkg/ResetVector/ResetVector.nasmb > > +++ b/OvmfPkg/ResetVector/ResetVector.nasmb > > @@ -53,6 +53,13 @@ > > %include "Ia32/SearchForSecEntry.asm" > > = > > %ifdef ARCH_X64 > > + #include > > + > > + %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) !=3D 0x6000) > > + %error "This implementation inherently depends on PcdOvmfSecPageTa= blesSize" > > + %endif > > + > > + %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + = (Offset)) > > %include "Ia32/Flat32ToFlat64.asm" > > %include "Ia32/PageTables64.asm" > > %endif > > = > = > I compared this patch to the one that I worked out / simplified from > your v2 patch and posted as our shared v3: > = > https://lists.01.org/pipermail/edk2-devel/2016-November/004140.html > = > The differences are cosmetic. > = > Reviewed-by: Laszlo Ersek > Tested-by: Laszlo Ersek > = > I could commit & push this now (I could even find the energy in me for > that), but I'd like Jordan to okay the patch as well, in this form. > = Reviewed-by: Jordan Justen