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

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

[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[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 <AutoGen.h>
> +    %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?

-Jordan

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

[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.justen@intel.com]
> Sent: Wednesday, November 2, 2016 11:31 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org
> Cc: lersek@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[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 <AutoGen.h>
> > +    %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?
> 
> -Jordan

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

[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[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 <AutoGen.h>
>> +    %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 <AutoGen.h>
in the assembly source -- despite the fact that commit 9b9fdbfa7059
#included <Base.h>!

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 <AutoGen.h>
%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 <AutoGen.h>
#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

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

[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[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 <AutoGen.h>
>>> +    %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 <AutoGen.h>
> in the assembly source -- despite the fact that commit 9b9fdbfa7059
> #included <Base.h>!
> 
> 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 <AutoGen.h>
> %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 <AutoGen.h>
> #define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))

Hold on: this won't work exactly like this. The reason is that CPP-level
preprocessing happens *first*, and NASM-level %include directives are
acted upon only *second*.

So, the above PT_ADDR() macro must be a %define (for NASM), not a
#define (for the C preprocessor).

Thanks
Laszlo

> 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

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

[Marvin Häuser]

Hey Laszlo,
Hey Jordan,

I sent a V2 of the patch. Changes:
* fixed the title to get rid of the numbering
* Introduced PT_ADDR() as proposed by Jordan
* introduced the size check proposed by Laszlo
* removed the 'ifndef' for the NASM ResetVector as only the PCD value should be allowed
* separated the 'ifndef' checks for the ASM ResetVector, so developers can change the base without needed to set a size

Thanks for your input!

Regards,
Marvin.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 3, 2016 12:40 AM
> To: Jordan Justen <jordan.l.justen@intel.com>; Marvin Häuser
> <Marvin.Haeuser@outlook.com>
> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>
> Subject: Re: [edk2] [PATCH v1 1/1] OvmfPkg/ResetVector: Depend on PCD
> values of the page tables.
> 
> 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[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 <AutoGen.h>
> >>> +    %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
> > <AutoGen.h> in the assembly source -- despite the fact that commit
> > 9b9fdbfa7059 #included <Base.h>!
> >
> > 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 <AutoGen.h>
> > %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 <AutoGen.h>
> > #define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) +
> > (Offset))
> 
> Hold on: this won't work exactly like this. The reason is that CPP-level
> preprocessing happens *first*, and NASM-level %include directives are
> acted upon only *second*.
> 
> So, the above PT_ADDR() macro must be a %define (for NASM), not a
> #define (for the C preprocessor).
> 
> Thanks
> Laszlo
> 
> > 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
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >