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