From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A8E0D81CCC for ; Fri, 4 Nov 2016 15:27:49 -0700 (PDT) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4F4DF804FA; Fri, 4 Nov 2016 22:27:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-156.phx2.redhat.com [10.3.116.156]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uA4MRnxn005735; Fri, 4 Nov 2016 18:27:50 -0400 To: =?UTF-8?Q?Marvin_H=c3=a4user?= , "edk2-devel@lists.01.org" References: Cc: "jordan.l.justen@intel.com" From: Laszlo Ersek Message-ID: <53f2a4d0-84b6-6831-9f13-6e1b53d4303a@redhat.com> Date: Fri, 4 Nov 2016 23:27:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 04 Nov 2016 22:27:51 +0000 (UTC) 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:27:49 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit On 11/04/16 14:32, Marvin Häuser 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/ResetVector/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 => 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 => 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 = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/ > *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/ > + > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/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) != 0x6000) > + %error "This implementation inherently depends on PcdOvmfSecPageTablesSize" > + %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. Thanks! Laszlo