From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from BAY004-OMC1S11.hotmail.com (bay004-omc1s11.hotmail.com [65.54.190.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2CE5C81D3D for ; Fri, 4 Nov 2016 06:34:42 -0700 (PDT) Received: from EUR01-DB5-obe.outbound.protection.outlook.com ([65.54.190.60]) by BAY004-OMC1S11.hotmail.com over TLS secured channel with Microsoft SMTPSVC(7.5.7601.23008); Fri, 4 Nov 2016 06:34:43 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=16KeP4ktHl/aWdKtqa6iQBmy70urPJqPF65Ik+Q9cSM=; b=RY29qmqTULJWvTJyYZ6vrwtNm5akWKlhnJq/bI7fmWpTM+y+m1iwb6pdPI7wG8NP67eWVH3nf5sRzpbRyGQZMK+apkchmDTflDnWKXaWgOw5zSTIBASxu4tr8RWLFzeiLk1/UlRAGYl2ak/1Umn25dAaMlIe98eJ1bycHj9fdeKEwJFhlp6wrJwF05ecWysbdQuog3ROEIrqYayM0TJIm1ZY4+9SinXdUbWTRFgwqAJrOjJ6pRp3XAioLIPHka5fQbAFbNigzPoGeLA1o/W2J0/4wM2AMQiFisRdS0g5tDGJxeq2NzjgN+aHzJ6iHuS+dK+KIkPCmWWYuatptfdZzg== Received: from DB5EUR01FT027.eop-EUR01.prod.protection.outlook.com (10.152.4.52) by DB5EUR01HT183.eop-EUR01.prod.protection.outlook.com (10.152.5.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.693.6; Fri, 4 Nov 2016 13:34:41 +0000 Received: from AM5PR0601MB2579.eurprd06.prod.outlook.com (10.152.4.60) by DB5EUR01FT027.mail.protection.outlook.com (10.152.5.1) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.693.6 via Frontend Transport; Fri, 4 Nov 2016 13:34:41 +0000 Received: from AM5PR0601MB2579.eurprd06.prod.outlook.com ([10.168.154.13]) by AM5PR0601MB2579.eurprd06.prod.outlook.com ([10.168.154.13]) with mapi id 15.01.0693.016; Fri, 4 Nov 2016 13:34:41 +0000 From: =?iso-8859-1?Q?Marvin_H=E4user?= To: "edk2-devel@lists.01.org" CC: Laszlo Ersek Thread-Topic: [PATCH v3] OvmfPkg/ResetVector: Depend on PCD values of the page tables. Thread-Index: AQHSNhsKFB9ptxKET0KDdBGfDpSPTaDH72oAgADkJcA= Date: Fri, 4 Nov 2016 13:34:41 +0000 Message-ID: References: <92036971-fcd8-487c-552b-0750d648cfe3@redhat.com> In-Reply-To: <92036971-fcd8-487c-552b-0750d648cfe3@redhat.com> Accept-Language: de-DE, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: lists.01.org; dkim=none (message not signed) header.d=none;lists.01.org; dmarc=none action=none header.from=outlook.com; x-incomingtopheadermarker: OriginalChecksum:; UpperCasedChecksum:; SizeAsReceived:7681; Count:40 x-ms-exchange-messagesentrepresentingtype: 1 x-tmn: [SCZXAdDC1GpUq7CGDwiO4Cglc2n6p3kL6Uap0Z23bSk+hRGDQUeYH5xhM+wj1AAk] x-incomingheadercount: 40 x-eopattributedmessage: 0 x-microsoft-exchange-diagnostics: 1; DB5EUR01HT183; 7:5UJtcYgeiRzfrVuRhYyH2cqgK9AY05idN8xMnY1erYncN+69usvjWMSrVorPjgnbRfX5Pj7tLzHiQ0jD+zjJORrBPIFmA5PhbRbpWy6pwIdRolnuxi25ibRGkyKDPyaSD9Wf8jPVMS5tEQK/Ssc1N0M1E7BPQk2KI46FZuUu7H7lj6zMydWFUXbsojsvsr3gKXwlNdr9n6FjRUHazAMFg6CTV1PSfzfp2hqNJK23SUXDF39wdUW5aFP6krD3H8lUUtPVu8Wh3sWWJTAt7ZKFhy7bs9MXbiZN1dzKoXIlkIg2WsjPhIkQBkkJjB1x6LoLHwlOpvG0fauabdHooWdTd9O/kHyS0c1JYk6qlbtRrTo= x-forefront-antispam-report: EFV:NLI; SFV:NSPM; SFS:(10019020)(98900003); DIR:OUT; SFP:1102; SCL:1; SRVR:DB5EUR01HT183; H:AM5PR0601MB2579.eurprd06.prod.outlook.com; FPR:; SPF:None; LANG:en; x-ms-office365-filtering-correlation-id: ed23c1a7-a419-4929-3686-08d404b754e9 x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(1603103113)(1601125047); SRVR:DB5EUR01HT183; x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(432015012)(82015046); SRVR:DB5EUR01HT183; BCL:0; PCL:0; RULEID:; SRVR:DB5EUR01HT183; x-forefront-prvs: 01165471DB spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Nov 2016 13:34:41.3626 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5EUR01HT183 X-OriginalArrivalTime: 04 Nov 2016 13:34:44.0093 (UTC) FILETIME=[33EBE2D0:01D236A0] 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: Fri, 04 Nov 2016 13:34:42 -0000 Content-Language: de-DE Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Hey Laszlo, I'm terribly sorry for the mistakes in v3, I made it in a hurry because it = was late - should have postponded for today. Because I didn't see 'your v3' (or didn't you post it yet?), I posted a v4 = as you said, which should have fixed the three comments you had. Thank you very much for your input! Regards, Marvin. > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Friday, November 4, 2016 12:56 AM > To: Marvin H=E4user ; edk2- > devel@lists.01.org > Cc: jordan.l.justen@intel.com > Subject: Re: [PATCH v3] OvmfPkg/ResetVector: Depend on PCD values of the > page tables. >=20 > Three comments: >=20 > On 11/03/16 22:41, Marvin H=E4user 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 >=20 > I think this is incorrect (or dubious at least); the NASM-level replaceme= nt text > for this will be >=20 > (0x800000 + (0 - 4)) >=20 > 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 UINT= 64, for > example. >=20 > > 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,1= 3 > > @@ 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/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) !=3D 0x6000) > > + %error "This implementation inherently depends on > PcdOvmfSecPageTablesSize" > > + %endif > > + > > + #include >=20 > 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. >=20 > > + %define PT_ADDR(Offset) FixedPcdGet32 (PcdOvmfSecPageTablesBase) >=20 > This misses the Offset macro param in the replacement text. >=20 > > %include "Ia32/Flat32ToFlat64.asm" > > %include "Ia32/PageTables64.asm" > > %endif > > >=20 > 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. >=20 > Thanks > Laszlo