Re: [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask

[Laszlo Ersek <lersek@redhat.com>]

On 02/09/17 06:56, Zeng, Star wrote:
> Stick to current comments and code, OvmfPkg X64 has bug? J
> 
>  
> 
> PCD comments:
> 
> #
> It is assumed that 64-bit DxeCore is built in firmware if it is true; otherwise 32-bit DxeCore
> 
>  
> 
> Code pieces in S3ResumePei, S3SaveStateDxe, SmmLockBoxPeiLib, etc:
> 
> // Both BIOS and OS wants 64bit vector
> 
> if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
> 
>   return TRUE;
> 
> }
> 
>  
> 
> //
> 
> // BIOS supports 64bit waking vector.
> //
> 
> if(FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
>   returnTRUE;
> 
> }
> 
>  
> 
> if ((sizeof(UINTN) == sizeof(UINT32)) && (FeaturePcdGet
> (PcdDxeIplSwitchToLongMode)) ) {
> 
>   //
> 
>  // 32 PEI + 64 DXE
> 
>  //
> 
>  
> 
>  
> 
> According to above, the patch I drafted is wrong.

That's too bad, because I agreed with it. :(

I can't start working on this right now, but if you guys think it's a
big problem, please file an OvmfPkg BZ, and we'll have to audit all uses
of PcdDxeIplSwitchToLongMode, to see what might break if we flip it to
TRUE for OVMF X64.

Thanks
Laszlo

> 
>  
> 
> Thanks,
> 
> Star
> 
> *From:*Yao, Jiewen
> *Sent:* Thursday, February 9, 2017 1:23 PM
> *To:* Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org
> *Cc:* Tian, Feng <feng.tian@intel.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>
> *Subject:* RE: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
> PcdPteMemoryEncryptionAddressOrMask
> 
>  
> 
> I suggest we evaluate **all** usages of PcdDxeIplSwitchToLongModebecause
> this is an incompatible change.
> 
>  
> 
> #
> It is assumed that 64-bit DxeCore is built in firmware if it is true; otherwise 32-bit DxeCore
> 
>  
> 
> Thank you
> 
> Yao Jiewen
> 
>  
> 
> *From:*Zeng, Star
> *Sent:* Wednesday, February 8, 2017 9:12 PM
> *To:* Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Yao,
> Jiewen <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>; Duran, Leo
> <leo.duran@amd.com <mailto:leo.duran@amd.com>>; edk2-devel@ml01.01.org
> <mailto:edk2-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>>;
> Justen, Jordan L <jordan.l.justen@intel.com
> <mailto:jordan.l.justen@intel.com>>; Zeng, Star <star.zeng@intel.com
> <mailto:star.zeng@intel.com>>
> *Subject:* RE: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
> PcdPteMemoryEncryptionAddressOrMask
> 
>  
> 
> In fact, X64 DxeIplPeim does not refer PcdDxeIplSwitchToLongMode at all.
> 
> DxeIpl.inf:
> [FeaturePcd.IA32]
>   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode      ## CONSUMES
> 
> As I remember, I did a draft patch below before for the discussion about how to determine *PEI* is 32BITs or 64BITs in title "[edk2] [PATCH v3 2/4] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB if needed", I can't find the archive link any more.
> 
> ---
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c | 5 +++++
>  MdeModulePkg/MdeModulePkg.dec                  | 4 +++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> index 6488880..348e084 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> @@ -43,6 +43,11 @@ HandOffToDxeCore (
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
>  
>    //
> +  // It should be FALSE for both PEI and DXE are 64-bit.
> +  //
> +  ASSERT (PcdGetBool (PcdDxeIplSwitchToLongMode) == FALSE);
> +
> +  //
>    // Get Vector Hand-off Info PPI and build Guided HOB
>    //
>    Status = PeiServicesLocatePpi (
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index af7bcab..4a73f7b 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -712,8 +712,10 @@
>    ## Indicates if DxeIpl should switch to long mode to enter DXE phase.
>    #  It is assumed that 64-bit DxeCore is built in firmware if it is true; otherwise 32-bit DxeCore
>    #  is built in firmware.<BR><BR>
> +  #  And it should be FALSE for both PEI and DXE are 64-bit.
>    #   TRUE  - DxeIpl will load a 64-bit DxeCore and switch to long mode to hand over to DxeCore.<BR>
> -  #   FALSE - DxeIpl will load a 32-bit DxeCore and perform stack switch to hand over to DxeCore.<BR>
> +  #   FALSE - DxeIpl will load a 32-bit DxeCore and perform stack switch to hand over to DxeCore,<BR>
> +  #           or both PEI and DXE are 64-bit.<BR>
>    # @Prompt DxeIpl switch to long mode.
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|TRUE|BOOLEAN|0x0001003b
>  
> --
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Thursday, February 9, 2017 3:48 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:edk2-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>>; Justen, Jordan L <jordan.l.justen@intel.com
> <mailto:jordan.l.justen@intel.com>>
> Subject: Re: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask
> 
> On 02/08/17 19:20, Yao, Jiewen wrote:
>> Got it.
>> 
>>  
>> 
>> If the means of PcdDxeIplSwitchtoLongMode  is unclear, we may add more 
>> description to make it clear.
>> 
>>  
>> 
>> If we believe “PcdDxeIplSwitchtoLongMode  == DXE is Long mode” as 
>> final conclusion, can we treat that as a bug and fix OVMF X64?
> 
> I don't know how to "fix" that. What is there to fix?
> 
> Flipping the PCD to TRUE in OVMF X64 would break OVMF X64, and likely OVMF Ia32X64 too. For two reasons:
> 
> 
> (1) Toggling the PCD would alter the behavior of the DXE IPL PEIM and/or the DXE Core, and that behavior has never ever been tested with OVMF X64.
> 
> The PcdDxeIplSwitchToLongMode=FALSE setting in the X64 DSC dates back to:
> 
>   commit 8fa729a8b1401f01c6fd8ddbcab45e4a4904fa9a
>   Author: lgao4 <lgao4@6f19259b-4bc3-4df7-8a09-765794883524>
>   Date:   Mon Mar 15 01:40:59 2010 +0000
> 
>       Merge the same type PCD section.
> 
>       git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@10243 6f19259b-4bc3-4df7-8a09-765794883524
> 
> and before that, to:
> 
>   commit 49ba9447c92d6fca214476381107a180d08e59d1
>   Author: jljusten <jljusten@6f19259b-4bc3-4df7-8a09-765794883524>
>   Date:   Wed May 27 21:10:18 2009 +0000
> 
>       Add initial version of Open Virtual Machine Firmware (OVMF) platform.
> 
>       git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@8398 6f19259b-4bc3-4df7-8a09-765794883524
> 
> In other words, OVMF X64 has always worked like this, since its inception.
> 
> 
> (2) PEI code under OvmfPkg itself uses this PCD to identify a 32-bit DXE phase.
> 
>     //
>     // If DXE is 32-bit, then we're done; PciBusDxe will degrade 64-bit MMIO
>     // resources to 32-bit anyway. See DegradeResource() in
>     // "PciResourceSupport.c".
>     //
>   #ifdef MDE_CPU_IA32
>     if (!FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
>       return FirstNonAddress;
>     }
>   #endif
> 
> and
> 
>     //
>     // If DXE is 32-bit, then just return the traditional 64 MB cap.
>     //
>   #ifdef MDE_CPU_IA32
>     if (!FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
>       return SIZE_64MB;
>     }
>   #endif
> 
> Thanks
> Laszlo
> 
>> 
>>  
>> 
>> Thank you
>> 
>> Yao Jiewen
>> 
>>  
>> 
>>  
>> 
>> *From:*Laszlo Ersek [mailto:lersek@redhat.com]
>> *Sent:* Wednesday, February 8, 2017 9:52 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:edk2-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>>; Justen, Jordan L <jordan.l.justen@intel.com
> <mailto:jordan.l.justen@intel.com>>
>> *Subject:* Re: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD 
>> PcdPteMemoryEncryptionAddressOrMask
>> 
>>  
>> 
>> On 02/08/17 18:27, Yao, Jiewen wrote:
>>> I believe PcdDxeIplSwitchtoLongMode  == DXE is Long mode.
>>> 
>>>  
>>> 
>>> See DEC description:
>>> 
>>>   #  It is assumed that 64-bit DxeCore is built in firmware if it is 
>>> true; otherwise 32-bit DxeCore
>>> 
>>>   #  is built in firmware.<BR><BR>
>> 
>> Unfortunately, I have no historical context or background for this 
>> PCD; all I can say is that the X64 OVMF platform does not set the PCD.
>> 
>> It enters long mode, and sets up page tables for the first 4GB of RAM, 
>> in SEC. Then SEC decompresses the flash contents to RAM, which covers 
>> both PEIFV and DXEFV. PEI runs from RAM.
>> 
>> This is possible because on QEMU/KVM, there's no need to initialize 
>> RAM, thus only SEC runs from flash, in-place.
>> 
>> Perhaps Jordan can provide more insight.
>> 
>> If Brijesh and Leo would like to run the X64 OVMF platform as a SEV 
>> guest too, then this should be considered, in my opinion.
>> 
>> One more comment below:
>> 
>>> 
>>>  
>>> 
>>> And the code MdeModulePkg\Universal\Acpi\S3SaveStateDxe\AcpiS3ContextSave.c:
>>> 
>>>  
>>> 
>>> BOOLEAN
>>> 
>>> IsLongModeWakingVectorSupport (
>>> 
>>>   IN EFI_ACPI_4_0_FIRMWARE_ACPI_CONTROL_STRUCTURE*Facs
>>> 
>>>   )
>>> 
>>> {
>>> 
>>>   if((Facs == NULL) ||
>>> 
>>>       (Facs->Signature !=
>>> EFI_ACPI_4_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) ) {
>>> 
>>>     //
>>> 
>>>     // Something wrong with FACS.
>>> 
>>>     //
>>> 
>>>     returnFALSE;
>>> 
>>>   }
>>> 
>>>   if((Facs->Version ==
>>> EFI_ACPI_4_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_VERSION) &&
>>> 
>>>       ((Facs->Flags & EFI_ACPI_4_0_64BIT_WAKE_SUPPORTED_F) != 0)) {
>>> 
>>>     //
>>> 
>>>     // BIOS supports 64bit waking vector.
>>> 
>>>     //
>>> 
>>>     if(FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
>>> 
>>>       returnTRUE;
>>> 
>>>     }
>>> 
>>>   }
>>> 
>>>   returnFALSE;
>>> 
>>> }
>> 
>> In practice, it's okay if the OVMF X64 platform is recognized as "not 
>> supporting a 64-bit waking vector for S3 resume". All the 64-bit guest 
>> OSes that I've tested with OVMF X64 (Linux, and the Windows 7 / 8 / 10 
>> families, both client and server) use a real mode (16-bit) waking 
>> vector, in practice.
>> 
>> ... Actually, upon reviewing the above code more carefully, we don't 
>> even reach the FeaturePcdGet() call: the FACS that QEMU generates (and 
>> OVMF downloads and installs) corresponds to ACPI 1.0, that is, 
>> EFI_ACPI_1_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_VERSION.
>> 
>> Thanks
>> Laszlo
>> 
>> 
>>> 
>>>  
>>> 
>>>  
>>> 
>>> Thank you
>>> 
>>> Yao Jiewen
>>> 
>>>  
>>> 
>>> *From:*edk2-devel [mailto:edk2-devel-bounces@lists.01.org] *On Behalf 
>>> Of *Yao, Jiewen
>>> *Sent:* Wednesday, February 8, 2017 9:18 AM
>>> *To:* Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com%0b>> <mailto:lersek@redhat.com>>; Duran, Leo <leo.duran@amd.com 
> <mailto:leo.duran@amd.com %0b>> <mailto:leo.duran@amd.com>>;
>>> Zeng, Star <star.zeng@intel.com
> <mailto:star.zeng@intel.com%0b>> <mailto:star.zeng@intel.com>>; edk2-devel@ml01.01.org
> <mailto:edk2-devel@ml01.01.org> 
>> <mailto:edk2-devel@ml01.01.org>
>>> *Cc:* Tian, Feng <feng.tian@intel.com <mailto:feng.tian@intel.com
> <mailto:feng.tian@intel.com %3cmailto:feng.tian@intel.com>>>; 
>>> Singh, Brijesh <brijesh.singh@amd.com <mailto:brijesh.singh@amd.com
> <mailto:brijesh.singh@amd.com %3cmailto:brijesh.singh@amd.com>>>
>>> *Subject:* Re: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD 
>>> PcdPteMemoryEncryptionAddressOrMask
>>> 
>>>  
>>> 
>>> Good reminder. I take back my word.
>>> 
>>> In this case, we need consume PcdPteMemoryEncryptionAddressOrMask  in IA32 mode to build X64 paging.
>>> 
>>> We need
>>> 
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Wednesday, February 8, 2017 9:11 AM
>>> To: Yao, Jiewen <jiewen.yao@intel.com
> <mailto:jiewen.yao@intel.com%0b>> <mailto:jiewen.yao@intel.com%0b>> <mailto:jiewen.yao@intel.com>>; 
>> Duran, Leo <leo.duran@amd.com <mailto:leo.duran@amd.com%0b
> <mailto:leo.duran@amd.com %3cmailto:leo.duran@amd.com%0b>>> 
>> <mailto:leo.duran@amd.com>>; Zeng, Star <star.zeng@intel.com 
> <mailto:star.zeng@intel.com %0b>> <mailto:star.zeng@intel.com%0b>> <mailto:star.zeng@intel.com>>; 
>> edk2-devel@ml01.01.org
> <mailto:edk2-devel@ml01.01.org> <mailto:edk2-devel@ml01.01.org>
>>> <mailto:edk2-devel@ml01.01.org>
>>> Cc: Tian, Feng <feng.tian@intel.com
> <mailto:feng.tian@intel.com%0b>> <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 %3cmailto:brijesh.singh@amd.com%0b>>> 
>> <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.
>>> 
>>> 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
> <mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com%0b>> <mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com%0b>> 
>> <mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com>>>; 
>> edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org
> <mailto:edk2-devel@ml01.01.org%3cmailto:edk2-devel@ml01.01.org>
>> <mailto:edk2-devel@ml01.01.org%3cmailto:edk2-devel@ml01.01.org>
>>> <mailto:edk2-devel@ml01.01.org%3cmailto:edk2-devel@ml01.01.org>>
>>>> *Cc:* Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com
> <mailto:lersek@redhat.com%3cmailto:lersek@redhat.com%0b>> <mailto:lersek@redhat.com%3cmailto:lersek@redhat.com%0b>> 
>> <mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>>; Tian, Feng
>>>> <feng.tian@intel.com<mailto:feng.tian@intel.com
> <mailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com%0b>> <mailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com%0b>> 
>> <mailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com>>>; Singh, 
>> Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com
> <mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com%0b>> <mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com%0b>> 
>> <mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>; Yao,
>>>> Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com
> <mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com%0b>> <mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com%0b>> 
>> <mailto:jiewen.yao@intel.com%3cmailto: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 %0b>>>>> <mailto:leo.duran@amd.com<mailto:leo.duran@amd.com 
> <mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com %0b>>>>> %3cmailto:leo.duran@amd.com
>> <mailto:leo.duran@amd.com 
> <mailto:leo.duran@amd.com %0b>> %3cmailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com 
>> %3cmailto:leo.duran@amd.com%0b>> <mailto:leo.duran@amd.com 
> <mailto:leo.duran@amd.com %0b>> %3cmailto:leo.duran@amd.com%3cmailto: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%3cmailto:edk2-devel@ml01.01.org>
>> <mailto:edk2-devel@ml01.01.org%3cmailto:edk2-devel@ml01.01.org>
>>> <mailto:edk2-devel@ml01.01.org%3cmailto: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%0b>> <mailto:lersek@redhat.com%0b 
> <mailto:lersek@redhat.com%0b %0b>> <mailto:lersek@redhat.com%0b%0b>> 
>> <mailto:lersek@redhat.com%0b%3cmailto: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%0b>> <mailto:feng.tian@intel.com%0b 
> <mailto:feng.tian@intel.com%0b %0b>> <mailto:feng.tian@intel.com%0b%0b>> 
>> <mailto:feng.tian@intel.com%0b%3cmailto: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%0b>> <mailto:brijesh.singh@amd.com%0b 
> <mailto:brijesh.singh@amd.com%0b %0b>> <mailto:brijesh.singh@amd.com%0b%0b>> 
>> <mailto:brijesh.singh@amd.com%0b%3cmailto: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%0b>> <mailto:star.zeng@intel.com%0b 
> <mailto:star.zeng@intel.com%0b %0b>> <mailto:star.zeng@intel.com%0b%0b>> 
>> <mailto:star.zeng@intel.com%0b%3cmailto:star.zeng@intel.com%0b>>> 
>> <mailto:star.zeng@intel.com>>;
>>>>> Yao, Jiewen <jiewen.yao@intel.com 
> <mailto:jiewen.yao@intel.com %0b>>>>> <mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com 
> <mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com %0b>>>>> %3cmailto:jiewen.yao@intel.com
>> <mailto:jiewen.yao@intel.com 
> <mailto:jiewen.yao@intel.com %0b>> %3cmailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com 
>> %3cmailto:jiewen.yao@intel.com%0b>> <mailto:jiewen.yao@intel.com 
> <mailto:jiewen.yao@intel.com %0b>> %3cmailto:jiewen.yao@intel.com%3cmailto: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%3cmailto:edk2-devel@ml01.01.org>
>> <mailto:edk2-devel@ml01.01.org%3cmailto:edk2-devel@ml01.01.org>
>>> <mailto:edk2-devel@ml01.01.org%3cmailto: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%0b>> <mailto:lersek@redhat.com%0b 
> <mailto:lersek@redhat.com%0b %0b>> <mailto:lersek@redhat.com%0b%0b>> 
>> <mailto:lersek@redhat.com%0b%3cmailto: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%0b>> <mailto:feng.tian@intel.com%0b 
> <mailto:feng.tian@intel.com%0b %0b>> <mailto:feng.tian@intel.com%0b%0b>> 
>> <mailto:feng.tian@intel.com%0b%3cmailto: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%0b>> <mailto:brijesh.singh@amd.com%0b 
> <mailto:brijesh.singh@amd.com%0b %0b>> <mailto:brijesh.singh@amd.com%0b%0b>> 
>> <mailto:brijesh.singh@amd.com%0b%3cmailto: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%0b>> <mailto:star.zeng@intel.com%0b 
> <mailto:star.zeng@intel.com%0b %0b>> <mailto:star.zeng@intel.com%0b%0b>> 
>> <mailto:star.zeng@intel.com%0b%3cmailto:star.zeng@intel.com%0b>>> 
>> <mailto:star.zeng@intel.com>>;
>>>>> Leo Duran <leo.duran@amd.com 
> <mailto:leo.duran@amd.com %0b>>>>> <mailto:leo.duran@amd.com<mailto:leo.duran@amd.com 
> <mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com %0b>>>>> %3cmailto:leo.duran@amd.com
>> <mailto:leo.duran@amd.com 
> <mailto:leo.duran@amd.com %0b>> %3cmailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com 
>> %3cmailto:leo.duran@amd.com%0b>> <mailto:leo.duran@amd.com 
> <mailto:leo.duran@amd.com %0b>> %3cmailto:leo.duran@amd.com%3cmailto: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<mailto:brijesh.singh@amd.com 
> <mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com %0b>>>>> %3cmailto:brijesh.singh@amd.com
>> <mailto:brijesh.singh@amd.com 
> <mailto:brijesh.singh@amd.com %0b>> %3cmailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com 
>> %3cmailto:brijesh.singh@amd.com%0b>> <mailto:brijesh.singh@amd.com 
> <mailto:brijesh.singh@amd.com %0b>> %3cmailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com 
>> %3cmailto: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 %0b>>>>> <mailto:feng.tian@intel.com<mailto:feng.tian@intel.com 
> <mailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com %0b>>>>> %3cmailto:feng.tian@intel.com
>> <mailto:feng.tian@intel.com 
> <mailto:feng.tian@intel.com %0b>> %3cmailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com 
>> %3cmailto:feng.tian@intel.com%0b>> <mailto:feng.tian@intel.com 
> <mailto:feng.tian@intel.com %0b>> %3cmailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com 
>> %3cmailto:feng.tian@intel.com>>>>
>>>>> Cc: Star Zeng <star.zeng@intel.com 
> <mailto:star.zeng@intel.com %0b>>>>> <mailto:star.zeng@intel.com<mailto:star.zeng@intel.com 
> <mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com %0b>>>>> %3cmailto:star.zeng@intel.com
>> <mailto:star.zeng@intel.com 
> <mailto:star.zeng@intel.com %0b>> %3cmailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com 
>> %3cmailto:star.zeng@intel.com%0b>> <mailto:star.zeng@intel.com 
> <mailto:star.zeng@intel.com %0b>> %3cmailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com 
>> %3cmailto:star.zeng@intel.com>>>>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com 
> <mailto:lersek@redhat.com %0b>>>>> <mailto:lersek@redhat.com<mailto:lersek@redhat.com 
> <mailto:lersek@redhat.com%3cmailto:lersek@redhat.com %0b>>>>> %3cmailto:lersek@redhat.com
>> <mailto:lersek@redhat.com 
> <mailto:lersek@redhat.com %0b>> %3cmailto:lersek@redhat.com%3cmailto:lersek@redhat.com 
>> %3cmailto:lersek@redhat.com%0b>> <mailto:lersek@redhat.com 
> <mailto:lersek@redhat.com %0b>> %3cmailto:lersek@redhat.com%3cmailto: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<mailto:leo.duran@amd.com 
> <mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com %0b>>>>> %3cmailto:leo.duran@amd.com
>> <mailto:leo.duran@amd.com 
> <mailto:leo.duran@amd.com %0b>> %3cmailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com 
>> %3cmailto:leo.duran@amd.com%0b>> <mailto:leo.duran@amd.com 
> <mailto:leo.duran@amd.com %0b>> %3cmailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com 
>> %3cmailto: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%3cmailto:edk2-devel@lists.01.org>
>> <mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>
>>> <mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>> 
>>> <mailto:edk2-devel@lists.01.org>
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org <mailto: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
>>> 
>> 
>