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

[Laszlo Ersek <lersek@redhat.com>]

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