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

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

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