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 3E79F81CB8 for ; Wed, 2 Nov 2016 16:26:24 -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 64C877F7D5; Wed, 2 Nov 2016 23:26:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-23.phx2.redhat.com [10.3.116.23]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uA2NQNnL008862; Wed, 2 Nov 2016 19:26:24 -0400 To: Jordan Justen , =?UTF-8?Q?Marvin_H=c3=a4user?= References: <147812587193.13829.15857238630348474715@jljusten-ivb> From: Laszlo Ersek Cc: "edk2-devel@lists.01.org" Message-ID: <1444f463-0605-ef32-74d4-20413285f9fa@redhat.com> Date: Thu, 3 Nov 2016 00:26:23 +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: <147812587193.13829.15857238630348474715@jljusten-ivb> 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.28]); Wed, 02 Nov 2016 23:26:25 +0000 (UTC) Subject: Re: [PATCH v1 1/1] 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: Wed, 02 Nov 2016 23:26:24 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 11/02/16 23:31, Jordan Justen wrote: > The "v1 1/1" isn't needed in the subject. For the first version of a > single patch series, I would expect to just see [PATCH]. (Obviously > this is not too important.) (I think the "v1" implies that Marvin expects to send a v2.) > Email headers seem to indicate that you aren't using git send-email. > This will cause troubles if someday you have a multi-patch series. > > On 2016-11-02 11:00:34, Marvin Häuser wrote: >> >> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm >> >> ; >> ; Top level Page Directory Pointers (1 * 512GB entry) >> ; >> - mov dword[0x800000], 0x801000 + PAGE_PDP_ATTR >> + mov dword[OVMF_SEC_PAGE_TABLES_BASE], OVMF_SEC_PAGE_TABLES_BASE + 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[OVMF_SEC_PAGE_TABLES_BASE + 0x1000], OVMF_SEC_PAGE_TABLES_BASE + 0x2000 + PAGE_PDP_ATTR >> + mov dword[OVMF_SEC_PAGE_TABLES_BASE + 0x1008], OVMF_SEC_PAGE_TABLES_BASE + 0x3000 + PAGE_PDP_ATTR >> + mov dword[OVMF_SEC_PAGE_TABLES_BASE + 0x1010], OVMF_SEC_PAGE_TABLES_BASE + 0x4000 + PAGE_PDP_ATTR >> + mov dword[OVMF_SEC_PAGE_TABLES_BASE + 0x1018], OVMF_SEC_PAGE_TABLES_BASE + 0x5000 + PAGE_PDP_ATTR > > These line are too long. I guess you can use '\' at the end of a line > to continue it, or maybe add a PT_ADDR() macro that adds in > OVMF_SEC_PAGE_TABLES_BASE. > >> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb >> index 31ac06a..b47f647 100644 >> --- a/OvmfPkg/ResetVector/ResetVector.nasmb >> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb >> @@ -53,6 +53,12 @@ >> %include "Ia32/SearchForSecEntry.asm" >> >> %ifdef ARCH_X64 >> + %ifndef OVMF_SEC_PAGE_TABLES_BASE >> + #include >> + %define OVMF_SEC_PAGE_TABLES_BASE FixedPcdGet32 (PcdOvmfSecPageTablesBase) >> + %define OVMF_SEC_PAGE_TABLES_SIZE FixedPcdGet32 (PcdOvmfSecPageTablesSize) >> + %endif >> + >> %include "Ia32/Flat32ToFlat64.asm" >> %include "Ia32/PageTables64.asm" >> %endif >> diff --git a/OvmfPkg/ResetVector/ResetVectorCode.asm b/OvmfPkg/ResetVector/ResetVectorCode.asm >> index 052c821..5b49387 100644 >> --- a/OvmfPkg/ResetVector/ResetVectorCode.asm >> +++ b/OvmfPkg/ResetVector/ResetVectorCode.asm >> @@ -40,6 +40,15 @@ >> %include "Ia32/SearchForSecEntry.asm" >> >> %ifdef ARCH_X64 >> + %ifndef OVMF_SEC_PAGE_TABLES_BASE >> + ; >> + ; This range should match with PcdOvmfSecPageTablesBase and >> + ; PcdOvmfSecPageTablesSize which are declared in the FDF files. >> + ; >> + %define OVMF_SEC_PAGE_TABLES_BASE 0x800000 >> + %define OVMF_SEC_PAGE_TABLES_SIZE 6 * 0x1000 > > I thought we were using the PCDs? We were, yes, except where we couldn't. For the reset vector, our "root" file is OvmfPkg/ResetVector/ResetVector.nasmb (referenced by OvmfPkg/ResetVector/ResetVector.inf ) That nasmb file %include's a bunch of other files; among those, OvmfPkg/ResetVector/Ia32/PageTables64.asm Notice that the %include and %define directives in these files are NASM-native directives; they aren't C preprocessor directives. So we couldn't use FixedPcdGetXX() in the source code. If I remember correctly, in commit b382ede3864e ("OvmfPkg X64 ResetVector: Move page tables from 512KB to 8MB"), you added the PCD references in a source code comment because this way at least a "grep" would find them, and then the programmer could update the constants (if necessary). Note that in the era of commit b382ede3864e (2014-Jan-21), we used to compile the reset vector manually, with the utility OvmfPkg/ResetVector/Build.py On 2014-Aug-19 however, you added native NASMB support to BaseTools, and adapted OvmfPkg/ResetVector: abb158ded41f BaseTools: Add rules to build NASM source file into a binary 5a1f324d946c UefiCpuPkg: Support building VTF0 ResetVector during the EDK II build eee1d2ca9078 UefiCpuPkg VTF0 X64: Build page tables in NASM code 9b9fdbfa7059 OvmfPkg: Support building OVMF's ResetVector during the EDK II build 497cbb530a58 OvmfPkg: Build OVMF ResetVector during EDK II build process 70e46f44cd13 OvmfPkg/ResetVector: Remove pre-built binaries 3449f56dac9c UefiCpuPkg: Add ResetVector/FixupVtf The BaseTools support would imply automatic preprocessing (with the C preprocessor, $(PP)) for *.nasmb files. We both failed to notice :) that this enabled us to #include in the assembly source -- despite the fact that commit 9b9fdbfa7059 #included ! So, we forgot to take advantage of the preprocessor, and to replace the open-coded constants with FixedPcdGetXX(). I think the idea in Marvin's patch is solid: it converts the FixedPcdGetXX() macros into NASM-level macros, with #include %define OVMF_SEC_PAGE_TABLES_BASE FixedPcdGet32 (PcdOvmfSecPageTablesBase) %define OVMF_SEC_PAGE_TABLES_SIZE FixedPcdGet32 (PcdOvmfSecPageTablesSize) Namely, $(PP) will preprocess the %define directives themselves (see commit abb158ded41f), and then NASM will %define the NASM-level macros with the expanded constants already. At this point, I think we could simply use PT_ADDR(), as you suggest: leave it all to the C preprocessor (i.e., $(PP)), and not bother with NASM-level %define's at all. Something like: #include #define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset)) I think NASM should be happy with the replacement text. And, this should be done unconditionally. Regarding the size of the area: I think that's a harder question, because the content is not just a plain array. The code inherently open-codes the size constant. So, rather than introducing OVMF_SEC_PAGE_TABLES_SIZE (purely for documentation purposes), how about: #if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x6000) #error "This implementation inherently depends on PcdOvmfSecPageTablesSize" #endif Consequences: - When PcdOvmfSecPageTablesBase is changed in the FDF files, the reset vector code follows it automatically. - When PcdOvmfSecPageTablesSize is changed in the FDF files, the reset vector code needs a manual rewrite -- and now the build will break loudly, accordingly. Thanks Laszlo