Re: [PATCH v3 0/4] Add PCD PcdPteMemoryEncryptionAddressOrMask

[Laszlo Ersek <lersek@redhat.com>]

On 02/16/17 22:02, Leo Duran wrote:
> This new 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 is be applied when creating 1:1 virtual to physical mapping tables.
> For example, the OvmfPkg sets the PCD when launching SEV-enabled guests.
> 
> Changes since v2:
> - Apply mask to PCD to keep value within Address field
> - Use variable instead of PCD in Page-Fault Handler
> 
> To-Do:
> - Add PCD support for page tables in UefiCpuPg/PiSmmCpuDxeSmm

>From a very superficial skim -- none of the patches are for OvmfPkg
after all! :) --, this approach looks good to me.

If I recall correctly, we're going to set the PCD in OVMF dynamically.
Is that right?

That usually requires investigating the possible orderings between "PCD
producer" and "PCD consumers". For DXE_DRIVER and DXE_SMM_DRIVER
consumers, it is good enough to set the PCD in OvmfPkg/PlatformPei
(which is a PEIM, so it's bound to run earlier -- PEI comes before DXE).

For PEIM consumers though, more thought might be necessary. OvmfPkg does
not pull in CapsulePei, so no need to worry about that.

Wrt. S3Resume2Pei: the dispatch order of S3Resume2Pei vs. PlatformPei is
unspecified (both have plain TRUE depexes). However, S3Resume2Pei does
not consume the PCD in its entry point function, only on the following
call path:

DxeLoadCore() [MdeModulePkg/Core/DxeIplPeim/DxeLoad.c]
  S3RestoreConfig2() [UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c]
    RestoreS3PageTables()
      PcdGet64(...)

IOW, the PCD is not consumed when S3Resume2Pei starts, but when the DXE
Initial Program Load PEIM realizes, at the end of the PEI phase, that
we're resuming, so it branches to EFI_PEI_S3_RESUME2_PPI, rather than
loading the DXE core (and starting the DXE phase).

So, I think setting the PCD in OvmfPkg/PlatformPei will be safe.

(BTW, if we had no such ordering guarantees between PlatformPei and
another PEIM, we could still make it work: we could hook a NULL class
library instance into all such PEIMs, and the lib constructor function
would (re-)set the PCD afresh in each, from whatever hw characteristics
SEV is detectable. Edk2 is awesome like that.

For an instance of this pattern (albeit in the DXE phase), please search
the OVMF DSC files for
"OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf".)

... Perhaps this has been obvious, but I thought it wouldn't be useless
to mention.

Thanks
Laszlo

> Leo Duran (4):
>   MdeModulePkg: Add PCD PcdPteMemoryEncryptionAddressOrMask
>   MdeModulePkg/Universal/CapsulePei:     Add support for PCD
>     PcdPteMemoryEncryptionAddressOrMask
>   UefiCpuPkg/Universal/Acpi/S3Resume2Pei:     Add support for PCD
>     PcdPteMemoryEncryptionAddressOrMask
>   MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe:     Add support for
>     PCD PcdPteMemoryEncryptionAddressOrMask
> 
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf            |  5 +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c   | 29 ++++++++++++++++------
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h   |  6 +++++
>  MdeModulePkg/MdeModulePkg.dec                      |  8 ++++++
>  .../BootScriptExecutorDxe.inf                      |  2 ++
>  .../Acpi/BootScriptExecutorDxe/ScriptExecute.c     |  7 ++++++
>  .../Acpi/BootScriptExecutorDxe/ScriptExecute.h     |  2 ++
>  .../Acpi/BootScriptExecutorDxe/X64/SetIdtEntry.c   | 16 ++++++++----
>  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf   |  2 ++
>  .../Universal/CapsulePei/Common/CommonHeader.h     |  7 ++++++
>  MdeModulePkg/Universal/CapsulePei/UefiCapsule.c    | 13 +++++++---
>  MdeModulePkg/Universal/CapsulePei/X64/X64Entry.c   | 23 +++++++++++------
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c  | 16 +++++++++---
>  .../Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf   |  2 ++
>  14 files changed, 109 insertions(+), 29 deletions(-)
>