* [PATCH v3] OvmfPkg/ResetVector: Depend on PCD values of the page tables. @ 2016-11-03 21:41 Marvin Häuser 2016-11-03 23:55 ` Laszlo Ersek 0 siblings, 1 reply; 3+ messages in thread From: Marvin Häuser @ 2016-11-03 21:41 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: jordan.l.justen@intel.com, lersek@redhat.com 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 <Marvin.Haeuser@outlook.com> --- 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 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 <AutoGen.h> + %define PT_ADDR(Offset) FixedPcdGet32 (PcdOvmfSecPageTablesBase) %include "Ia32/Flat32ToFlat64.asm" %include "Ia32/PageTables64.asm" %endif -- 2.10.1.windows.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] OvmfPkg/ResetVector: Depend on PCD values of the page tables. 2016-11-03 21:41 [PATCH v3] OvmfPkg/ResetVector: Depend on PCD values of the page tables Marvin Häuser @ 2016-11-03 23:55 ` Laszlo Ersek 2016-11-04 13:34 ` Marvin Häuser 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Ersek @ 2016-11-03 23:55 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org; +Cc: jordan.l.justen@intel.com 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 <Marvin.Haeuser@outlook.com> > --- > 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 <AutoGen.h> 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] OvmfPkg/ResetVector: Depend on PCD values of the page tables. 2016-11-03 23:55 ` Laszlo Ersek @ 2016-11-04 13:34 ` Marvin Häuser 0 siblings, 0 replies; 3+ messages in thread From: Marvin Häuser @ 2016-11-04 13:34 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: Laszlo Ersek 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äuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org <edk2-devel@ml01.01.org> > Cc: jordan.l.justen@intel.com > Subject: Re: [PATCH v3] OvmfPkg/ResetVector: Depend on PCD values of the > page tables. > > 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 <Marvin.Haeuser@outlook.com> > > --- > > 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 <AutoGen.h> > > 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-11-04 13:34 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-03 21:41 [PATCH v3] OvmfPkg/ResetVector: Depend on PCD values of the page tables Marvin Häuser 2016-11-03 23:55 ` Laszlo Ersek 2016-11-04 13:34 ` Marvin Häuser
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox