* Re: [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the page tables.
[not found] <AM5PR0601MB25794FB82411C0C6AC2B0FF280A30@AM5PR0601MB2579.eurprd06.prod.outlook.com>
@ 2016-11-03 20:59 ` Jordan Justen
2016-11-03 21:46 ` Marvin Häuser
2016-11-03 22:55 ` Laszlo Ersek
1 sibling, 1 reply; 6+ messages in thread
From: Jordan Justen @ 2016-11-03 20:59 UTC (permalink / raw)
To: Marvin Häuser, edk2-devel@lists.01.org; +Cc: lersek@redhat.com
On 2016-11-03 12:18:56, Marvin Häuser wrote:
> Currently, the values of the page tables' address and size are
> hard-coded in the ResetVector. This patch replaces this with a PCD
> dependency for the NASM Reset Vector. The ASM Reset Vector remains
> using a hard-coded value due to the lack of C preprocessing.
>
> Checks for the size have 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 | 7 ++++++
> OvmfPkg/ResetVector/ResetVectorCode.asm | 16 ++++++++++++++
> 4 files changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index b5a4cf8d7187..7ff394b1afc3 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -37,6 +37,8 @@ BITS 32
> PAGE_READ_WRITE + \
> PAGE_PRESENT)
>
> +%define PT_ADDR(Offset) (OVMF_SEC_PAGE_TABLES_BASE + (Offset))
> +
>
> ;
> ; Modified: EAX, ECX
> @@ -46,31 +48,28 @@ SetCr3ForPageTables64:
> ;
> ; For OVMF, build some initial page tables at 0x800000-0x806000.
> ;
> - ; This range should match with PcdOvmfSecPageTablesBase and
> - ; PcdOvmfSecPageTablesSize which are declared in the FDF files.
> - ;
> ; At the end of PEI, the pages tables will be rebuilt into a
> ; more permanent location by DxeIpl.
> ;
>
> - mov ecx, 6 * 0x1000 / 4
> + mov ecx, OVMF_SEC_PAGE_TABLES_SIZE / 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 +80,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
I think the '- 8' should be in the PT_ADDR.
This patch is basically
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
except, like Laszlo pointed out, I think that ResetVectorCode.asm
should have been deleted in 70e46f44cd13.
So, before this patch, I think we should add a patch to delete it.
Then you won't need to modify it, and only the PCD's will be used.
Would you like to make these changes, or would you like me to?
Thanks,
-Jordan
> loop pageTableEntriesLoop
>
> ;
> ; Set CR3 now that the paging structures are available
> ;
> - mov eax, 0x800000
> + mov eax, OVMF_SEC_PAGE_TABLES_BASE
> 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..62597ea95cf5 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -53,6 +53,13 @@
> %include "Ia32/SearchForSecEntry.asm"
>
> %ifdef ARCH_X64
> + #include <AutoGen.h>
> + %define OVMF_SEC_PAGE_TABLES_BASE FixedPcdGet32 (PcdOvmfSecPageTablesBase)
> + %define OVMF_SEC_PAGE_TABLES_SIZE FixedPcdGet32 (PcdOvmfSecPageTablesSize)
> +
> + %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x006000)
> + %error "This implementation inherently depends on PcdOvmfSecPageTablesSize"
> + %endif
> %include "Ia32/Flat32ToFlat64.asm"
> %include "Ia32/PageTables64.asm"
> %endif
> diff --git a/OvmfPkg/ResetVector/ResetVectorCode.asm b/OvmfPkg/ResetVector/ResetVectorCode.asm
> index 052c821f212c..1a1d92fb1415 100644
> --- a/OvmfPkg/ResetVector/ResetVectorCode.asm
> +++ b/OvmfPkg/ResetVector/ResetVectorCode.asm
> @@ -40,6 +40,22 @@
> %include "Ia32/SearchForSecEntry.asm"
>
> %ifdef ARCH_X64
> + ;
> + ; These values should match with PcdOvmfSecPageTablesBase and
> + ; PcdOvmfSecPageTablesSize which are declared in the FDF files.
> + ;
> +
> + %ifndef OVMF_SEC_PAGE_TABLES_BASE
> + %define OVMF_SEC_PAGE_TABLES_BASE 0x800000
> + %endif
> +
> + %ifndef OVMF_SEC_PAGE_TABLES_SIZE
> + %define OVMF_SEC_PAGE_TABLES_SIZE 6 * 0x1000
> + %endif
> +
> + %if (OVMF_SEC_PAGE_TABLES_SIZE != 0x006000)
> + %error "This implementation inherently depends on OVMF_SEC_PAGE_TABLES_SIZE"
> + %endif
> %include "Ia32/Flat32ToFlat64.asm"
> %include "Ia32/PageTables64.asm"
> %endif
> --
> 2.10.1.windows.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the page tables.
2016-11-03 20:59 ` [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the page tables Jordan Justen
@ 2016-11-03 21:46 ` Marvin Häuser
0 siblings, 0 replies; 6+ messages in thread
From: Marvin Häuser @ 2016-11-03 21:46 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: Jordan Justen, Laszlo Ersek
Hey Jordan,
Hey Laszlo,
I just pushed a v3. Changes:
* Pushed a patch to remove ResetVector.asm. As this is not really connected to the change after, I made it a separate patch rather than a series.
* Don't abstract or use the PCD size as, as Laszlo pointed out, it makes little sense, because the rest of the ASM needs to be adapter to the size change anyway, if one changes it. Do you see a way to make this dynamic too? Maybe calculate the offsets by division?
* Make clear in the comment that the PCD value is used for the address.
* Move PT_ADDR() to the ResetVector file so there is no need to abstract the Address PCD for the PageTables file.
* Include addition/subtractions after PT_ADDR() into the brackets, as suggested by Jordan.
Regards,
Marvin.
> -----Original Message-----
> From: Jordan Justen [mailto:jordan.l.justen@intel.com]
> Sent: Thursday, November 3, 2016 9:59 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org
> Cc: lersek@redhat.com
> Subject: Re: [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the
> page tables.
>
> On 2016-11-03 12:18:56, Marvin Häuser wrote:
> > Currently, the values of the page tables' address and size are
> > hard-coded in the ResetVector. This patch replaces this with a PCD
> > dependency for the NASM Reset Vector. The ASM Reset Vector remains
> > using a hard-coded value due to the lack of C preprocessing.
> >
> > Checks for the size have 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 | 7 ++++++
> > OvmfPkg/ResetVector/ResetVectorCode.asm | 16 ++++++++++++++
> > 4 files changed, 39 insertions(+), 12 deletions(-)
> >
> > diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> > b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> > index b5a4cf8d7187..7ff394b1afc3 100644
> > --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> > +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> > @@ -37,6 +37,8 @@ BITS 32
> > PAGE_READ_WRITE + \
> > PAGE_PRESENT)
> >
> > +%define PT_ADDR(Offset) (OVMF_SEC_PAGE_TABLES_BASE + (Offset))
> > +
> >
> > ;
> > ; Modified: EAX, ECX
> > @@ -46,31 +48,28 @@ SetCr3ForPageTables64:
> > ;
> > ; For OVMF, build some initial page tables at 0x800000-0x806000.
> > ;
> > - ; This range should match with PcdOvmfSecPageTablesBase and
> > - ; PcdOvmfSecPageTablesSize which are declared in the FDF files.
> > - ;
> > ; At the end of PEI, the pages tables will be rebuilt into a
> > ; more permanent location by DxeIpl.
> > ;
> >
> > - mov ecx, 6 * 0x1000 / 4
> > + mov ecx, OVMF_SEC_PAGE_TABLES_SIZE / 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 +80,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
>
> I think the '- 8' should be in the PT_ADDR.
>
> This patch is basically
>
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>
> except, like Laszlo pointed out, I think that ResetVectorCode.asm should
> have been deleted in 70e46f44cd13.
>
> So, before this patch, I think we should add a patch to delete it.
> Then you won't need to modify it, and only the PCD's will be used.
>
> Would you like to make these changes, or would you like me to?
>
> Thanks,
>
> -Jordan
>
> > loop pageTableEntriesLoop
> >
> > ;
> > ; Set CR3 now that the paging structures are available
> > ;
> > - mov eax, 0x800000
> > + mov eax, OVMF_SEC_PAGE_TABLES_BASE
> > 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..62597ea95cf5 100644
> > --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> > +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> > @@ -53,6 +53,13 @@
> > %include "Ia32/SearchForSecEntry.asm"
> >
> > %ifdef ARCH_X64
> > + #include <AutoGen.h>
> > + %define OVMF_SEC_PAGE_TABLES_BASE FixedPcdGet32
> > + (PcdOvmfSecPageTablesBase) %define OVMF_SEC_PAGE_TABLES_SIZE
> > + FixedPcdGet32 (PcdOvmfSecPageTablesSize)
> > +
> > + %if (FixedPcdGet32 (PcdOvmfSecPageTablesSize) != 0x006000)
> > + %error "This implementation inherently depends on
> PcdOvmfSecPageTablesSize"
> > + %endif
> > %include "Ia32/Flat32ToFlat64.asm"
> > %include "Ia32/PageTables64.asm"
> > %endif
> > diff --git a/OvmfPkg/ResetVector/ResetVectorCode.asm
> > b/OvmfPkg/ResetVector/ResetVectorCode.asm
> > index 052c821f212c..1a1d92fb1415 100644
> > --- a/OvmfPkg/ResetVector/ResetVectorCode.asm
> > +++ b/OvmfPkg/ResetVector/ResetVectorCode.asm
> > @@ -40,6 +40,22 @@
> > %include "Ia32/SearchForSecEntry.asm"
> >
> > %ifdef ARCH_X64
> > + ;
> > + ; These values should match with PcdOvmfSecPageTablesBase and ;
> > + PcdOvmfSecPageTablesSize which are declared in the FDF files.
> > + ;
> > +
> > + %ifndef OVMF_SEC_PAGE_TABLES_BASE
> > + %define OVMF_SEC_PAGE_TABLES_BASE 0x800000 %endif
> > +
> > + %ifndef OVMF_SEC_PAGE_TABLES_SIZE
> > + %define OVMF_SEC_PAGE_TABLES_SIZE 6 * 0x1000 %endif
> > +
> > + %if (OVMF_SEC_PAGE_TABLES_SIZE != 0x006000)
> > + %error "This implementation inherently depends on
> OVMF_SEC_PAGE_TABLES_SIZE"
> > + %endif
> > %include "Ia32/Flat32ToFlat64.asm"
> > %include "Ia32/PageTables64.asm"
> > %endif
> > --
> > 2.10.1.windows.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the page tables.
[not found] <AM5PR0601MB25794FB82411C0C6AC2B0FF280A30@AM5PR0601MB2579.eurprd06.prod.outlook.com>
2016-11-03 20:59 ` [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the page tables Jordan Justen
@ 2016-11-03 22:55 ` Laszlo Ersek
2016-11-03 23:10 ` Jordan Justen
1 sibling, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2016-11-03 22:55 UTC (permalink / raw)
To: Marvin Häuser; +Cc: edk2-devel@lists.01.org, jordan.l.justen@intel.com
Hi Marvin,
On 11/03/16 20:18, Marvin Häuser wrote:
> Currently, the values of the page tables' address and size are
> hard-coded in the ResetVector. This patch replaces this with a PCD
> dependency for the NASM Reset Vector. The ASM Reset Vector remains
> using a hard-coded value due to the lack of C preprocessing.
>
> Checks for the size have 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 | 7 ++++++
> OvmfPkg/ResetVector/ResetVectorCode.asm | 16 ++++++++++++++
> 4 files changed, 39 insertions(+), 12 deletions(-)
I'll soon send a modified / simplified version of your patch (keeping
your authorship and S-o-b in the first place of course). I find that
simpler than struggling with an explanation :) I hope you're fine with this.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the page tables.
2016-11-03 22:55 ` Laszlo Ersek
@ 2016-11-03 23:10 ` Jordan Justen
2016-11-03 23:19 ` Marvin Häuser
0 siblings, 1 reply; 6+ messages in thread
From: Jordan Justen @ 2016-11-03 23:10 UTC (permalink / raw)
To: Laszlo Ersek, Marvin Häuser; +Cc: edk2-devel@lists.01.org
On 2016-11-03 15:55:10, Laszlo Ersek wrote:
> Hi Marvin,
>
> On 11/03/16 20:18, Marvin Häuser wrote:
> > Currently, the values of the page tables' address and size are
> > hard-coded in the ResetVector. This patch replaces this with a PCD
> > dependency for the NASM Reset Vector. The ASM Reset Vector remains
> > using a hard-coded value due to the lack of C preprocessing.
> >
> > Checks for the size have 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 | 7 ++++++
> > OvmfPkg/ResetVector/ResetVectorCode.asm | 16 ++++++++++++++
> > 4 files changed, 39 insertions(+), 12 deletions(-)
>
> I'll soon send a modified / simplified version of your patch (keeping
> your authorship and S-o-b in the first place of course). I find that
> simpler than struggling with an explanation :) I hope you're fine with this.
>
Marvin's v3 seemed okay, but of course, it should be a series,
together with his separate patch that removes the unused
ResetVectorCode.asm file.
If your tweaks are minor, you can add my
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
to both of his patches.
Thanks,
-Jordan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the page tables.
2016-11-03 23:10 ` Jordan Justen
@ 2016-11-03 23:19 ` Marvin Häuser
2016-11-03 23:43 ` Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Marvin Häuser @ 2016-11-03 23:19 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: Laszlo Ersek
Hey Laszlo,
I'm quoting Jordan's mail because, for some reason, I didn't receive yours.
Sure, I'm fine with you re-posting the patches, though I thought I considered everything you had said in v3 (not abstracting the size and adding the error message).
Sorry if I didn't consider something, in that case I must have overread it.
Thanks for 'fixing' the patches!
Regards,
Marvin.
> -----Original Message-----
> From: Jordan Justen [mailto:jordan.l.justen@intel.com]
> Sent: Friday, November 4, 2016 12:10 AM
> To: Laszlo Ersek <lersek@redhat.com>; Marvin Häuser
> <Marvin.Haeuser@outlook.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v2] OvmfPkg/ResetVector: Depend on PCD
> values of the page tables.
>
> On 2016-11-03 15:55:10, Laszlo Ersek wrote:
> > Hi Marvin,
> >
> > On 11/03/16 20:18, Marvin Häuser wrote:
> > > Currently, the values of the page tables' address and size are
> > > hard-coded in the ResetVector. This patch replaces this with a PCD
> > > dependency for the NASM Reset Vector. The ASM Reset Vector remains
> > > using a hard-coded value due to the lack of C preprocessing.
> > >
> > > Checks for the size have 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 | 7 ++++++
> > > OvmfPkg/ResetVector/ResetVectorCode.asm | 16 ++++++++++++++
> > > 4 files changed, 39 insertions(+), 12 deletions(-)
> >
> > I'll soon send a modified / simplified version of your patch (keeping
> > your authorship and S-o-b in the first place of course). I find that
> > simpler than struggling with an explanation :) I hope you're fine with this.
> >
>
> Marvin's v3 seemed okay, but of course, it should be a series, together with
> his separate patch that removes the unused ResetVectorCode.asm file.
>
> If your tweaks are minor, you can add my
>
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>
> to both of his patches.
>
> Thanks,
>
> -Jordan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the page tables.
2016-11-03 23:19 ` Marvin Häuser
@ 2016-11-03 23:43 ` Laszlo Ersek
0 siblings, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2016-11-03 23:43 UTC (permalink / raw)
To: Marvin Häuser; +Cc: edk2-devel@lists.01.org, Jordan Justen (Intel address)
On 11/04/16 00:19, Marvin Häuser wrote:
> Hey Laszlo,
>
> I'm quoting Jordan's mail because, for some reason, I didn't receive yours.
> Sure, I'm fine with you re-posting the patches, though I thought I considered everything you had said in v3 (not abstracting the size and adding the error message).
> Sorry if I didn't consider something, in that case I must have overread it.
>
> Thanks for 'fixing' the patches!
My email is overflowing and I've switched into batched / temporarily
write-only mode. I think I got out of sync with you guys. I'll look at
your v3 patches soon -- they take priority over my modifications, of course.
Thanks
Laszlo
>> -----Original Message-----
>> From: Jordan Justen [mailto:jordan.l.justen@intel.com]
>> Sent: Friday, November 4, 2016 12:10 AM
>> To: Laszlo Ersek <lersek@redhat.com>; Marvin Häuser
>> <Marvin.Haeuser@outlook.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH v2] OvmfPkg/ResetVector: Depend on PCD
>> values of the page tables.
>>
>> On 2016-11-03 15:55:10, Laszlo Ersek wrote:
>>> Hi Marvin,
>>>
>>> On 11/03/16 20:18, Marvin Häuser wrote:
>>>> Currently, the values of the page tables' address and size are
>>>> hard-coded in the ResetVector. This patch replaces this with a PCD
>>>> dependency for the NASM Reset Vector. The ASM Reset Vector remains
>>>> using a hard-coded value due to the lack of C preprocessing.
>>>>
>>>> Checks for the size have 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 | 7 ++++++
>>>> OvmfPkg/ResetVector/ResetVectorCode.asm | 16 ++++++++++++++
>>>> 4 files changed, 39 insertions(+), 12 deletions(-)
>>>
>>> I'll soon send a modified / simplified version of your patch (keeping
>>> your authorship and S-o-b in the first place of course). I find that
>>> simpler than struggling with an explanation :) I hope you're fine with this.
>>>
>>
>> Marvin's v3 seemed okay, but of course, it should be a series, together with
>> his separate patch that removes the unused ResetVectorCode.asm file.
>>
>> If your tweaks are minor, you can add my
>>
>> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>>
>> to both of his patches.
>>
>> Thanks,
>>
>> -Jordan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-03 23:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <AM5PR0601MB25794FB82411C0C6AC2B0FF280A30@AM5PR0601MB2579.eurprd06.prod.outlook.com>
2016-11-03 20:59 ` [PATCH v2] OvmfPkg/ResetVector: Depend on PCD values of the page tables Jordan Justen
2016-11-03 21:46 ` Marvin Häuser
2016-11-03 22:55 ` Laszlo Ersek
2016-11-03 23:10 ` Jordan Justen
2016-11-03 23:19 ` Marvin Häuser
2016-11-03 23:43 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox