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 93A2B81C4B for ; Fri, 4 Nov 2016 15:50:10 -0700 (PDT) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (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 731AF4E4DE; Fri, 4 Nov 2016 22:50:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-156.phx2.redhat.com [10.3.116.156]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uA4MoAQ5016380; Fri, 4 Nov 2016 18:50:11 -0400 To: Jordan Justen , =?UTF-8?Q?Marvin_H=c3=a4user?= , "edk2-devel@lists.01.org" References: <53f2a4d0-84b6-6831-9f13-6e1b53d4303a@redhat.com> <147829910764.11627.2691984297559573830@jljusten-ivb> From: Laszlo Ersek Message-ID: Date: Fri, 4 Nov 2016 23:50:10 +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: <147829910764.11627.2691984297559573830@jljusten-ivb> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Fri, 04 Nov 2016 22:50:12 +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:50:10 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 11/04/16 23:38, Jordan Justen wrote: > On 2016-11-04 15:27:48, Laszlo Ersek wrote: >> 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. >> > > Reviewed-by: Jordan Justen Thanks, pushed as commit 73d66c5871cc. Thank you for the contribution, Marvin! Laszlo