Re: [PATCH 3/3] UefiCpuPkg/Universal/Acpi/S3Resume2Pei: Add support for PCD PcdPteMemoryEncryptionAddressOrMask

["Fan, Jeff" <jeff.fan@intel.com>]

Leo,

If this PCD is only to patch address, could you make sure the other fields not to be updated in case the platform set the other fields in PCD.

PageTable setup in DxeIpl is in boot service data range. If this feature is only supported in POST phase, that's enough.

Thanks!
Jeff

-----Original Message-----
From: Duran, Leo [mailto:leo.duran@amd.com] 
Sent: Friday, February 10, 2017 12:28 PM
To: Fan, Jeff; edk2-devel@ml01.01.org
Cc: Tian, Feng; Zeng, Star; Laszlo Ersek; Singh, Brijesh
Subject: RE: [PATCH 3/3] UefiCpuPkg/Universal/Acpi/S3Resume2Pei: Add support for PCD PcdPteMemoryEncryptionAddressOrMask

Hi Jeff,
The new PCD is intended to be OR'ed with the address (upper bits).
Leo.

> -----Original Message-----
> From: Fan, Jeff [mailto:jeff.fan@intel.com]
> Sent: Thursday, February 09, 2017 8:23 PM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star 
> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Singh, 
> Brijesh <brijesh.singh@amd.com>
> Subject: RE: [PATCH 3/3] UefiCpuPkg/Universal/Acpi/S3Resume2Pei: Add 
> support for PCD PcdPteMemoryEncryptionAddressOrMask
> 
> Hi Leo,
> 
> I want to understand your usage model. What fields are you going to 
> update in below Page Table Entry by the new PCD?
> 
> typedef union {
>   struct {
>     UINT64  Present:1;                // 0 = Not present in memory, 1 = Present in
> memory
>     UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
>     UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
>     UINT64  WriteThrough:1;           // 0 = Write-Back caching, 1=Write-Through
> caching
>     UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
>     UINT64  Accessed:1;               // 0 = Not accessed, 1 = Accessed (set by CPU)
>     UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by processor on access
> to page
>     UINT64  MustBe1:1;                // Must be 1
>     UINT64  Global:1;                 // 0 = Not global page, 1 = global page TLB not
> cleared on CR3 write
>     UINT64  Available:3;              // Available for use by system software
>     UINT64  PAT:1;                    //
>     UINT64  MustBeZero:8;             // Must be zero;
>     UINT64  PageTableBaseAddress:31;  // Page Table Base Address
>     UINT64  AvabilableHigh:11;        // Available for use by system software
>     UINT64  Nx:1;                     // 0 = Execute Code, 1 = No Code Execution
>   } Bits;
>   UINT64    Uint64;
> } PAGE_TABLE_ENTRY;
> 
> I did not see any updating in SMM. Is it un-necessary? Is this feature 
> working on POST phase or OS runtime phase?
> 
> Thanks!
> Jeff
> 
> -----Original Message-----
> From: Leo Duran [mailto:leo.duran@amd.com]
> Sent: Thursday, February 09, 2017 5:13 AM
> To: edk2-devel@ml01.01.org
> Cc: Leo Duran; Fan, Jeff; Tian, Feng; Zeng, Star; Laszlo Ersek; 
> Brijesh Singh
> Subject: [PATCH 3/3] UefiCpuPkg/Universal/Acpi/S3Resume2Pei: Add 
> support for PCD PcdPteMemoryEncryptionAddressOrMask
> 
> This PCD holds the address mask for page table entries when memory 
> encryption is enabled on AMD processors supporting the Secure 
> Encrypted Virtualization (SEV) feature.
> 
> The mask is applied when page tables are created (S3Resume.c).
> 
> CC: Jeff Fan <jeff.fan@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> ---
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c       | 9 +++++----
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 2 ++
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index d306fba..ee1e2cd 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -5,6 +5,7 @@
>    control is passed to OS waking up handler.
> 
>    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 @@ 
> -696,7
> +697,7 @@ RestoreS3PageTables (
>        //
>        // Make a PML4 Entry
>        //
> -      PageMapLevel4Entry->Uint64 =
> (UINT64)(UINTN)PageDirectoryPointerEntry;
> +      PageMapLevel4Entry->Uint64 =
> + (UINT64)(UINTN)PageDirectoryPointerEntry | PcdGet64 
> + (PcdPteMemoryEncryptionAddressOrMask);
>        PageMapLevel4Entry->Bits.ReadWrite = 1;
>        PageMapLevel4Entry->Bits.Present = 1;
> 
> @@ -707,7 +708,7 @@ RestoreS3PageTables (
>            //
>            // 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; @@ -724,7 +725,7 @@ 
> RestoreS3PageTables (
>            //
>            // 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;
> 
> @@ -732,7 +733,7 @@ RestoreS3PageTables (
>              //
>              // 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/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> index 73aeca3..d514523 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> @@ -6,6 +6,7 @@
>  # control is passed to OS waking up handler.
>  #
>  # Copyright (c) 2010 - 2014, 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 @@ 
> -91,6 +92,7 @@ [FeaturePcd]
> 
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ## 
> SOMETIMES_CONSUMES
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask    ## CONSUMES
> 
>  [Depex]
>    TRUE
> --
> 1.9.1