Re: [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask

[Laszlo Ersek <lersek@redhat.com>]

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>; 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.

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>; 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
>>>
>