subject:"Re\: \[edk2\] \[PATCH v1 1\/1\] OvmfPkg\/ResetVector\: Depend on PCD values of the page tables."

Re: [edk2] [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD values of the page tables.

2016-11-02 Thread Laszlo Ersek

On 11/03/16 00:26, Laszlo Ersek wrote:
> 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[0x80], 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 0x80
>>> +%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

Re: [edk2] [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD values of the page tables.

2016-11-02 Thread Laszlo Ersek

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[0x80], 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 0x80
>> +%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

Re: [edk2] [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD values of the page tables.

2016-11-02 Thread Marvin Häuser

Hey Jordan,

1) I have used git send-mail, but due to a corruption of my edk2 clone I did 
some 'funky' stuff to get it working. I expect stuff to be better with the next 
patch.
2) Will submit a V2 with line-breaks hopefully today, though might be tomorrow.
3) As said in the commit message, as far as I am aware, C preprocessing is not 
available for ASM (non-NASM), hence the values are hard-coded there still. Is 
there a way to use PCDs in ASM?
4) Can't the ASM file actually be removed? I don't see it referenced anywhere 
anymore. Is it kept for the case external packages expect it to be present? 
Should I clarify the commit message, such as using 'NASM ResetVector'?

Thanks for your input!

Regards,
Marvin.

> -Original Message-
> From: Jordan Justen [mailto:jordan.l.jus...@intel.com]
> Sent: Wednesday, November 2, 2016 11:31 PM
> To: Marvin Häuser ; edk2-
> de...@lists.01.org
> Cc: ler...@redhat.com
> Subject: Re: [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD values of
> the page tables.
> 
> 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.)
> 
> 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[0x80], 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 0x80
> > +%define OVMF_SEC_PAGE_TABLES_SIZE 6 * 0x1000
> 
> I thought we were using the PCDs?
> 
> -Jordan
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD values of the page tables.

2016-11-02 Thread Jordan Justen

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.)

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[0x80], 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 0x80
> +%define OVMF_SEC_PAGE_TABLES_SIZE 6 * 0x1000

I thought we were using the PCDs?

-Jordan
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD values of the page tables.

Re: [edk2] [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD values of the page tables.

Re: [edk2] [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD values of the page tables.

Re: [edk2] [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD values of the page tables.

4 matches

Site Navigation

Mail list logo

Footer information