Re: [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask

["Duran, Leo" <leo.duran@amd.com>]

Lazlo, et al,
Please see reply below.
Lleo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 08, 2017 11:11 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Duran, Leo
> <leo.duran@amd.com>; Zeng, Star <star.zeng@intel.com>; edk2-
> devel@ml01.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Singh, Brijesh
> <brijesh.singh@amd.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
> PcdPteMemoryEncryptionAddressOrMask
> 
> On 02/08/17 18:05, Yao, Jiewen wrote:
> > HI Leo
> >
> > Thanks to clarify that.
> >
> >
> >
> > If that is the case, do you think it will be better to limit this PCD
> > to
> > X64 only in DEC file. Such as [PcdsDynamic.X64, PcdsDynamicEx.X64]
> 
> Not sure if this is the best place to raise the following observation, but it
> should do:
> 
> please everyone remember that PcdDxeIplSwitchToLongMode is only TRUE
> if PEI is 32-bit and DXE is 64-bit. It is FALSE in *two* cases:
> - both PEI and DXE are 32-bit, and
> - both PEI and DXE are 64-bit.
> 
> This doesn't necessarily invalidate anything said thus fair in the thread, but
> the following statement from Leo:
> 
>     The SEV feature requires 64-bit LongMode, so the
>     PcdDxeIplSwitchtoLongMode *must* set to TRUE at build-time
> 
> does not follow. The PCD is FALSE in OvmfPkgX64.dsc.
[Duran, Leo] 
Good points... I should have provided more context.
1) I had referred the "PEI is 32-bit and DXE is 64-bit."
2) If both PEI and DXE are 64-bit, then you would be executing the X64 of HandOffToDxe(), which does *not* call Create4GPageTables().

That is, Create4GPageTables() only gets called in the "PEI is 32-bit" case.

> 
> Thanks,
> Laszlo
> 
> >
> >
> >
> > Thank you
> >
> > Yao Jiewen
> >
> >
> >
> > *From:*Duran, Leo [mailto:leo.duran@amd.com]
> > *Sent:* Wednesday, February 8, 2017 9:00 AM
> > *To:* Zeng, Star <star.zeng@intel.com>; edk2-devel@ml01.01.org
> > *Cc:* Laszlo Ersek <lersek@redhat.com>; Tian, Feng
> > <feng.tian@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>
> > *Subject:* RE: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
> > PcdPteMemoryEncryptionAddressOrMask
> >
> >
> >
> > Pease see reply below.
> > Leo
> >
> >> -----Original Message-----
> >> From: Zeng, Star [mailto:star.zeng@intel.com]
> >> Sent: Tuesday, February 07, 2017 8:27 PM
> >> To: Duran, Leo <leo.duran@amd.com <mailto:leo.duran@amd.com>>;
> >> edk2-devel@ml01.01.org
> > <mailto:edk2-devel@ml01.01.org>
> >> Cc: Laszlo Ersek <lersek@redhat.com
> > <mailto:lersek@redhat.com>>; Tian, Feng <feng.tian@intel.com
> > <mailto:feng.tian@intel.com>>;
> >> Singh, Brijesh <brijesh.singh@amd.com
> > <mailto:brijesh.singh@amd.com>>; Zeng, Star <star.zeng@intel.com
> > <mailto:star.zeng@intel.com>>;
> >> Yao, Jiewen <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>
> >> Subject: RE: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
> >> PcdPteMemoryEncryptionAddressOrMask
> >>
> >> Does Create4GPageTablesIa32Pae() also need to be updated?
> >>
> >> Thanks,
> >> Star
> > [Duran, Leo]
> > Hi Star,
> > No, I do not think Create4GPageTablesIa32Pae() is in the execution path.
> >
> > The SEV feature requires 64-bit LongMode, so the
> > PcdDxeIplSwitchtoLongMode *must* set to TRUE at build-time, in which
> case Create4GPageTablesIa32Pae() would *not* be called by
> HandOffToDxeCore().
> >
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >> Of Leo Duran
> >> Sent: Wednesday, February 8, 2017 3:54 AM
> >> To: edk2-devel@ml01.01.org <mailto:edk2-devel@ml01.01.org>
> >> Cc: Laszlo Ersek <lersek@redhat.com
> > <mailto:lersek@redhat.com>>; Tian, Feng <feng.tian@intel.com
> > <mailto:feng.tian@intel.com>>;
> >> Brijesh Singh <brijesh.singh@amd.com
> > <mailto:brijesh.singh@amd.com>>; Zeng, Star <star.zeng@intel.com
> > <mailto:star.zeng@intel.com>>;
> >> Leo Duran <leo.duran@amd.com <mailto:leo.duran@amd.com>>
> >> Subject: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
> >> PcdPteMemoryEncryptionAddressOrMask
> >>
> >> From: Brijesh Singh <brijesh.singh@amd.com
> >> <mailto:brijesh.singh@amd.com>>
> >>
> >> This dynamic PCD holds the address mask for page table entries when
> >> memory encryption is enabled on AMD processors supporting the Secure
> >> Encrypted Virtualization (SEV) feature.
> >>
> >> Cc: Feng Tian <feng.tian@intel.com <mailto:feng.tian@intel.com>>
> >> Cc: Star Zeng <star.zeng@intel.com <mailto:star.zeng@intel.com>>
> >> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Leo Duran <leo.duran@amd.com
> >> <mailto:leo.duran@amd.com>>
> >> ---
> >>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  5 ++++-
> >>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 18
> ++++++++++--
> >> ------
> >>  MdeModulePkg/MdeModulePkg.dec                    |  8 ++++++++
> >>  3 files changed, 22 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >> index 2bc41be..d62bd9b 100644
> >> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >> @@ -6,6 +6,8 @@
> >>  #  needed to run the DXE Foundation.
> >>  #
> >>  #  Copyright (c) 2006 - 2016, Intel Corporation. All rights
> >> reserved.<BR>
> >> +#  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> #
> >>  #  This program and the accompanying materials  #  are licensed and
> >> made available under the terms and conditions of the BSD License  #
> >> which accompanies this distribution.  The full text of the license
> >> may be found at @@ -111,7 +113,8 @@ [FeaturePcd]
> >>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress
> ##
> >> CONSUMES
> >>
> >>  [Pcd.IA32,Pcd.X64]
> >> -  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable              ##
> >> SOMETIMES_CONSUMES
> >> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ##
> >> SOMETIMES_CONSUMES
> >> +
> >>
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> >> ask    ## CONSUMES
> >>
> >>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
> >>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> >> SOMETIMES_CONSUMES
> >> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >> index 790f6ab..2c52389 100644
> >> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >> @@ -16,6 +16,8 @@
> >>      3) IA-32 Intel(R) Architecture Software Developer's Manual
> >> Volume 3:System Programmer's Guide, Intel
> >>
> >>  Copyright (c) 2006 - 2016, Intel Corporation. All rights
> >> reserved.<BR>
> >> +Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> >> +
> >>  This program and the accompanying materials  are licensed and made
> >> available under the terms and conditions of the BSD License  which
> >> accompanies this distribution.  The full text of the license may be
> >> found at @@ -71,14 +73,14 @@ Split2MPageTo4K (
> >>    //
> >>    // Fill in 2M page entry.
> >>    //
> >> -  *PageEntry2M = (UINT64) (UINTN) PageTableEntry | IA32_PG_P |
> >> IA32_PG_RW;
> >> +  *PageEntry2M = (UINT64) (UINTN) PageTableEntry | PcdGet64
> >> + (PcdPteMemoryEncryptionAddressOrMask) | IA32_PG_P |
> IA32_PG_RW;
> >>
> >>    PhysicalAddress4K = PhysicalAddress;
> >>    for (IndexOfPageTableEntries = 0; IndexOfPageTableEntries < 512;
> >> IndexOfPageTableEntries++, PageTableEntry++, PhysicalAddress4K +=
> >> SIZE_4KB) {
> >>      //
> >>      // Fill in the Page Table entries
> >>      //
> >> -    PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K;
> >> +    PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | PcdGet64
> >> + (PcdPteMemoryEncryptionAddressOrMask);
> >>      PageTableEntry->Bits.ReadWrite = 1;
> >>      PageTableEntry->Bits.Present = 1;
> >>      if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K <
> >> StackBase +
> >> StackSize)) { @@ -116,7 +118,7 @@ Split1GPageTo2M (
> >>    //
> >>    // Fill in 1G page entry.
> >>    //
> >> -  *PageEntry1G = (UINT64) (UINTN) PageDirectoryEntry | IA32_PG_P |
> >> IA32_PG_RW;
> >> +  *PageEntry1G = (UINT64) (UINTN) PageDirectoryEntry | PcdGet64
> >> + (PcdPteMemoryEncryptionAddressOrMask) | IA32_PG_P |
> IA32_PG_RW;
> >>
> >>    PhysicalAddress2M = PhysicalAddress;
> >>    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries
> >> < 512;
> >> IndexOfPageDirectoryEntries++, PageDirectoryEntry++,
> >> IndexOfPageDirectoryEntries++PhysicalAddress2M
> >> += SIZE_2MB) { @@ -129,7 +131,7 @@ Split1GPageTo2M (
> >>        //
> >>        // Fill in the Page Directory entries
> >>        //
> >> -      PageDirectoryEntry->Uint64 = (UINT64) PhysicalAddress2M;
> >> +      PageDirectoryEntry->Uint64 = (UINT64) PhysicalAddress2M |
> >> + PcdGet64 (PcdPteMemoryEncryptionAddressOrMask);
> >>        PageDirectoryEntry->Bits.ReadWrite = 1;
> >>        PageDirectoryEntry->Bits.Present = 1;
> >>        PageDirectoryEntry->Bits.MustBe1 = 1; @@ -248,7 +250,7 @@
> >> CreateIdentityMappingPageTables (
> >>      //
> >>      // Make a PML4 Entry
> >>      //
> >> -    PageMapLevel4Entry->Uint64 =
> >> (UINT64)(UINTN)PageDirectoryPointerEntry;
> >> +    PageMapLevel4Entry->Uint64 =
> >> + (UINT64)(UINTN)PageDirectoryPointerEntry | PcdGet64
> >> + (PcdPteMemoryEncryptionAddressOrMask);
> >>      PageMapLevel4Entry->Bits.ReadWrite = 1;
> >>      PageMapLevel4Entry->Bits.Present = 1;
> >>
> >> @@ -262,7 +264,7 @@ CreateIdentityMappingPageTables (
> >>            //
> >>            // Fill in the Page Directory entries
> >>            //
> >> -          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress;
> >> +          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress |
> >> + PcdGet64 (PcdPteMemoryEncryptionAddressOrMask);
> >>            PageDirectory1GEntry->Bits.ReadWrite = 1;
> >>            PageDirectory1GEntry->Bits.Present = 1;
> >>            PageDirectory1GEntry->Bits.MustBe1 = 1; @@ -280,7 +282,7
> >> @@ CreateIdentityMappingPageTables (
> >>          //
> >>          // Fill in a Page Directory Pointer Entries
> >>          //
> >> -        PageDirectoryPointerEntry->Uint64 =
> >> (UINT64)(UINTN)PageDirectoryEntry;
> >> +        PageDirectoryPointerEntry->Uint64 =
> >> + (UINT64)(UINTN)PageDirectoryEntry | PcdGet64
> >> + (PcdPteMemoryEncryptionAddressOrMask);
> >>          PageDirectoryPointerEntry->Bits.ReadWrite = 1;
> >>          PageDirectoryPointerEntry->Bits.Present = 1;
> >>
> >> @@ -294,7 +296,7 @@ CreateIdentityMappingPageTables (
> >>              //
> >>              // Fill in the Page Directory entries
> >>              //
> >> -            PageDirectoryEntry->Uint64 = (UINT64)PageAddress;
> >> +            PageDirectoryEntry->Uint64 = (UINT64)PageAddress |
> >> + PcdGet64 (PcdPteMemoryEncryptionAddressOrMask);
> >>              PageDirectoryEntry->Bits.ReadWrite = 1;
> >>              PageDirectoryEntry->Bits.Present = 1;
> >>              PageDirectoryEntry->Bits.MustBe1 = 1; diff --git
> >> a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index
> >> 273cd7e..207384f 100644
> >> --- a/MdeModulePkg/MdeModulePkg.dec
> >> +++ b/MdeModulePkg/MdeModulePkg.dec
> >> @@ -6,6 +6,8 @@
> >>  # Copyright (c) 2007 - 2017, Intel Corporation. All rights
> >> reserved.<BR>  # Copyright (c) 2016, Linaro Ltd. All rights
> >> reserved.<BR>  # (C) Copyright 2016 Hewlett Packard Enterprise
> >> Development LP<BR>
> >> +# Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> #
> >>  # This program and the accompanying materials are licensed and made
> >> available under  # the terms and conditions of the BSD License that
> >> accompanies this distribution.
> >>  # The full text of the license may be found at @@ -1738,5 +1740,11
> >> @@ [PcdsDynamic, PcdsDynamicEx]
> >>    # @Prompt If there is any test key used by the platform.
> >>
> >>
> gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed|FALSE|BOOLEAN|0x0
> >> 0030003
> >>
> >> +  ## This dynamic PCD holds the address mask for page table entries
> >> + when memory encryption is  #  enabled on AMD processors supporting
> >> + the
> >> Secure Encrypted Virtualization (SEV) feature.
> >> +  #  This mask should be applied when creating 1:1 virtual to
> >> + physical
> >> mapping tables.
> >> +  #
> >> +
> >> +
> >>
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> >> ask|0x0
> >> + |UINT64|0x00030004
> >> +
> >>  [UserExtensions.TianoCore."ExtraFiles"]
> >>    MdeModulePkgExtra.uni
> >> --
> >> 1.9.1
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> >