Re: [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask

[Laszlo Ersek <lersek@redhat.com>]

On 02/08/17 19:13, Yao, Jiewen wrote:
> I think X64 DXEIPL **may** create page table for X64 DXE. It is
> controlled by PcdDxeIplBuildPageTables.
> 
>  
> 
>   if(FeaturePcdGet (PcdDxeIplBuildPageTables)) {
> 
>     //
> 
>     // Create page table and save PageMapLevel4 to CR3
> 
>     //
> 
>     PageTables = CreateIdentityMappingPageTables ((EFI_PHYSICAL_ADDRESS)
> (UINTN) BaseOfStack, STACK_SIZE);

That's a good find:

  ## Indicates if DxeIpl should rebuild page tables. This flag only
  #  makes sense in the case where the DxeIpl and the DxeCore are both X64.<BR><BR>
  #   TRUE  - DxeIpl will rebuild page tables.<BR>
  #   FALSE - DxeIpl will not rebuild page tables.<BR>
  # @Prompt DxeIpl rebuild page tables.
  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplBuildPageTables|TRUE|BOOLEAN|0x0001003c

OVMF (all three platforms) inherit the DEC default without change.

For more confirmation, there's also:

  ## Indicates if to set NX for stack.<BR><BR>
  #  For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.<BR>
  #  For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require
  #  IA32 PAE is supported and Execute Disable Bit is available.<BR>
  #   TRUE  - to set NX for stack.<BR>
  #   FALSE - Not to set NX for stack.<BR>
  # @Prompt Set NX for stack.
  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE|BOOLEAN|0x0001006f

And in OVMF, all three platforms support NX-for-stack. It can be controlled dynamically, from the QEMU command line. We used to enable it by default, but it caused some old UEFI boot loaders to crash (because they were executing code from the stack), so we flipped it off, and exposed it on the QEMU command line.

  901c58c59412 MdeModulePkg: PcdSetNxForStack: enable dynamism
  d26753f8358c OvmfPkg: make PcdSetNxForStack dynamic
  c075d250f6f2 OvmfPkg: make PcdPropertiesTableEnable dynamic
  ab081a50e565 OvmfPkg: PlatformPei: take no-exec DXE settings from the QEMU command line
  d20b06a3afdf OvmfPkg: disable no-exec DXE stack by default

Either way, the point is that OVMF X64 keeps PcdDxeIplBuildPageTables=TRUE.

Thank you!
Laszlo


> 
>  
> 
> Thank you
> 
> Yao Jiewen
> 
>  
> 
>  
> 
> *From:*Laszlo Ersek [mailto:lersek@redhat.com]
> *Sent:* Wednesday, February 8, 2017 9:56 AM
> *To:* Duran, Leo <leo.duran@amd.com>; Yao, Jiewen
> <jiewen.yao@intel.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:28, Duran, Leo wrote:
>> 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 <mailto:jiewen.yao@intel.com>>; Duran, Leo
>>> <leo.duran@amd.com
> <mailto:leo.duran@amd.com>>; Zeng, Star <star.zeng@intel.com
> <mailto:star.zeng@intel.com>>; edk2-
>>> devel@ml01.01.org <mailto:devel@ml01.01.org>
>>> Cc: Tian, Feng <feng.tian@intel.com <mailto:feng.tian@intel.com>>; Singh, Brijesh
>>> <brijesh.singh@amd.com <mailto: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.
> 
> Right. Now that you mention the function name HandOffToDxe(), I'm pretty
> sure that I've looked at this several times in the past, I just couldn't
> / can't recall it now, without looking.
> 
> My goal was to ensure that all cases would be considered. Looks like
> they have been. :) It would be nice if both the Ia32X64 and the X64 OVMF
> platforms could work under SEV.
> 
> Thanks!
> Laszlo
> 
> 
> 
> 
>> 
>>>
>>> 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
> <mailto:star.zeng@intel.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>>; Yao,
>>>> Jiewen <jiewen.yao@intel.com <mailto: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
> <mailto:leo.duran@amd.com %3cmailto:leo.duran@amd.com>>>;
>>>>> edk2-devel@ml01.01.org <mailto:edk2-devel@ml01.01.org>
>>>> <mailto:edk2-devel@ml01.01.org>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com%0b>>>> <mailto:lersek@redhat.com>>; Tian, Feng <feng.tian@intel.com
> <mailto:feng.tian@intel.com%0b>>>> <mailto:feng.tian@intel.com>>;
>>>>> Singh, Brijesh <brijesh.singh@amd.com
> <mailto:brijesh.singh@amd.com%0b>>>> <mailto:brijesh.singh@amd.com>>; Zeng, Star <star.zeng@intel.com
> <mailto:star.zeng@intel.com%0b>>>> <mailto:star.zeng@intel.com>>;
>>>>> Yao, Jiewen <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com
> <mailto:jiewen.yao@intel.com %3cmailto: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> <mailto:edk2-devel@ml01.01.org>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com%0b>>>> <mailto:lersek@redhat.com>>; Tian, Feng <feng.tian@intel.com
> <mailto:feng.tian@intel.com%0b>>>> <mailto:feng.tian@intel.com>>;
>>>>> Brijesh Singh <brijesh.singh@amd.com
> <mailto:brijesh.singh@amd.com%0b>>>> <mailto:brijesh.singh@amd.com>>; Zeng, Star <star.zeng@intel.com
> <mailto:star.zeng@intel.com%0b>>>> <mailto:star.zeng@intel.com>>;
>>>>> Leo Duran <leo.duran@amd.com <mailto:leo.duran@amd.com
> <mailto:leo.duran@amd.com %3cmailto:leo.duran@amd.com>>>
>>>>> Subject: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
>>>>> PcdPteMemoryEncryptionAddressOrMask
>>>>>
>>>>> From: Brijesh Singh <brijesh.singh@amd.com
> <mailto:brijesh.singh@amd.com%0b>>>>> <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
> <mailto:feng.tian@intel.com %3cmailto:feng.tian@intel.com>>>
>>>>> Cc: Star Zeng <star.zeng@intel.com <mailto:star.zeng@intel.com
> <mailto:star.zeng@intel.com %3cmailto:star.zeng@intel.com>>>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com
> <mailto:lersek@redhat.com %3cmailto:lersek@redhat.com>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Leo Duran <leo.duran@amd.com
> <mailto:leo.duran@amd.com%0b>>>>> <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> <mailto:edk2-devel@lists.01.org>
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>
>> 
>