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 52B8081CF0 for ; Thu, 3 Nov 2016 16:55:55 -0700 (PDT) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 B7C8331B321; Thu, 3 Nov 2016 23:55:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-178.phx2.redhat.com [10.3.116.178]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uA3NtsWG024299; Thu, 3 Nov 2016 19:55:55 -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: <92036971-fcd8-487c-552b-0750d648cfe3@redhat.com> Date: Fri, 4 Nov 2016 00:55:54 +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.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 03 Nov 2016 23:55:56 +0000 (UTC) Subject: Re: [PATCH v3] 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: Thu, 03 Nov 2016 23:55:55 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Three comments: On 11/03/16 22:41, 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 | 6 +++++ > 3 files changed, 23 insertions(+), 11 deletions(-) > > diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > index b5a4cf8d7187..0b95a7fa9a86 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 I think this is incorrect (or dubious at least); the NASM-level replacement text for this will be (0x800000 + (0 - 4)) and I strongly dislike the (0 - 4) subexpression. The type in which NASM evaluates (0 - 4) is not specified, and I'd rather not risk it being UINT64, for example. > 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..4bf8c97d8158 100644 > --- a/OvmfPkg/ResetVector/ResetVector.nasmb > +++ b/OvmfPkg/ResetVector/ResetVector.nasmb > @@ -53,6 +53,12 @@ > %include "Ia32/SearchForSecEntry.asm" > > %ifdef ARCH_X64 > + %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x6000) > + %error "This implementation inherently depends on PcdOvmfSecPageTablesSize" > + %endif > + > + #include I think this order is wrong. The C preprocessor won't fully expand FixedPcdGet32 (PcdOvmfSecPageTablesSize) I believe, because AutoGen.h is included after the use of the macro. In turn NASM won't find a plain numeric constant on the LHS of the inequality. > + %define PT_ADDR(Offset) FixedPcdGet32 (PcdOvmfSecPageTablesBase) This misses the Offset macro param in the replacement text. > %include "Ia32/Flat32ToFlat64.asm" > %include "Ia32/PageTables64.asm" > %endif > Marvin, please send a v4, or else please respond with your R-b to "my" v3 (and then we can commit that series, if Jordan agrees) -- either works for me. Thanks Laszlo