Re: [PATCH v3 2/4] MdeModulePkg/Universal/CapsulePei: Add support for PCD PcdPteMemoryEncryptionAddressOrMask

["Zeng, Star" <star.zeng@intel.com>]

Leo,

Comments added inline.

On 2017/2/17 5:02, Leo Duran wrote:
> 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 4GB tables are created (UefiCapsule.c), and when
> the tables are expanded on-demand by page-faults above 4GB's (X64Entry.c).
>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> Reviewed-by: Star Zeng <star.zeng@intel.com>
> ---
>  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 +++++++++++++++-------
>  4 files changed, 34 insertions(+), 11 deletions(-)
>

[snipped]

> diff --git a/MdeModulePkg/Universal/CapsulePei/X64/X64Entry.c b/MdeModulePkg/Universal/CapsulePei/X64/X64Entry.c
> index 5ad95d2..2197502 100644
> --- a/MdeModulePkg/Universal/CapsulePei/X64/X64Entry.c
> +++ b/MdeModulePkg/Universal/CapsulePei/X64/X64Entry.c
> @@ -2,6 +2,8 @@
>    The X64 entrypoint is used to process capsule in long mode.
>
>  Copyright (c) 2011 - 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
> @@ -29,6 +31,7 @@ typedef struct _PAGE_FAULT_CONTEXT {
>    UINT64                        PhyMask;
>    UINTN                         PageFaultBuffer;
>    UINTN                         PageFaultIndex;
> +  UINT64                        PteMemoryEncryptionAddressOrMask;
>    //
>    // Store the uplink information for each page being used.
>    //
> @@ -114,6 +117,7 @@ AcquirePage (
>    )
>  {
>    UINTN             Address;
> +  UINT64            AddressSetMask;
>
>    Address = PageFaultContext->PageFaultBuffer + EFI_PAGES_TO_SIZE (PageFaultContext->PageFaultIndex);
>    ZeroMem ((VOID *) Address, EFI_PAGES_TO_SIZE (1));
> @@ -121,14 +125,16 @@ AcquirePage (
>    //
>    // Cut the previous uplink if it exists and wasn't overwritten.
>    //
> -  if ((PageFaultContext->PageFaultUplink[PageFaultContext->PageFaultIndex] != NULL) && ((*PageFaultContext->PageFaultUplink[PageFaultContext->PageFaultIndex] & PageFaultContext->PhyMask) == Address)) {
> +  if ((PageFaultContext->PageFaultUplink[PageFaultContext->PageFaultIndex] != NULL) &&
> +     ((*PageFaultContext->PageFaultUplink[PageFaultContext->PageFaultIndex] & PageFaultContext->PhyMask) == Address)) {

No real change at here except the line feed added.
You were going to update code at here, but forgot to do the real change?

Will you do similar change for [PATCH v3 4/4]

Thanks,
Star

>      *PageFaultContext->PageFaultUplink[PageFaultContext->PageFaultIndex] = 0;
>    }
>
>    //
>    // Link & Record the current uplink.
>    //
> -  *Uplink = Address | IA32_PG_P | IA32_PG_RW;
> +  AddressSetMask = PageFaultContext->PteMemoryEncryptionAddressOrMask;
> +  *Uplink = Address | (AddressSetMask & PAGING_4K_ADDRESS_MASK_64) | IA32_PG_P | IA32_PG_RW;
>    PageFaultContext->PageFaultUplink[PageFaultContext->PageFaultIndex] = Uplink;
>
>    PageFaultContext->PageFaultIndex = (PageFaultContext->PageFaultIndex + 1) % EXTRA_PAGE_TABLE_PAGES;
> @@ -153,6 +159,7 @@ PageFaultHandler (
>    UINT64                    *PageTable;
>    UINT64                    PFAddress;
>    UINTN                     PTIndex;
> +  UINT64                    AddressSetMask;
>
>    //
>    // Get the IDT Descriptor.
> @@ -163,6 +170,7 @@ PageFaultHandler (
>    //
>    PageFaultContext = (PAGE_FAULT_CONTEXT *) (UINTN) (Idtr.Base - sizeof (PAGE_FAULT_CONTEXT));
>    PhyMask = PageFaultContext->PhyMask;
> +  AddressSetMask = PageFaultContext->PteMemoryEncryptionAddressOrMask;
>
>    PFAddress = AsmReadCr2 ();
>    DEBUG ((EFI_D_ERROR, "CapsuleX64 - PageFaultHandler: Cr2 - %lx\n", PFAddress));
> @@ -179,19 +187,19 @@ PageFaultHandler (
>    if ((PageTable[PTIndex] & IA32_PG_P) == 0) {
>      AcquirePage (PageFaultContext, &PageTable[PTIndex]);
>    }
> -  PageTable = (UINT64*)(UINTN)(PageTable[PTIndex] & PhyMask);
> +  PageTable = (UINT64*)(UINTN)((PageTable[PTIndex] & ~(AddressSetMask & PAGING_4K_ADDRESS_MASK_64)) & PhyMask);
>    PTIndex = BitFieldRead64 (PFAddress, 30, 38);
>    // PDPTE
>    if (PageFaultContext->Page1GSupport) {
> -    PageTable[PTIndex] = (PFAddress & ~((1ull << 30) - 1)) | IA32_PG_P | IA32_PG_RW | IA32_PG_PS;
> +    PageTable[PTIndex] = ((PFAddress | (AddressSetMask & PAGING_1G_ADDRESS_MASK_64)) & ~((1ull << 30) - 1)) | IA32_PG_P | IA32_PG_RW | IA32_PG_PS;
>    } else {
>      if ((PageTable[PTIndex] & IA32_PG_P) == 0) {
>        AcquirePage (PageFaultContext, &PageTable[PTIndex]);
>      }
> -    PageTable = (UINT64*)(UINTN)(PageTable[PTIndex] & PhyMask);
> +    PageTable = (UINT64*)(UINTN)((PageTable[PTIndex] & ~(AddressSetMask & PAGING_4K_ADDRESS_MASK_64)) & PhyMask);
>      PTIndex = BitFieldRead64 (PFAddress, 21, 29);
>      // PD
> -    PageTable[PTIndex] = (PFAddress & ~((1ull << 21) - 1)) | IA32_PG_P | IA32_PG_RW | IA32_PG_PS;
> +    PageTable[PTIndex] = ((PFAddress | (AddressSetMask & PAGING_2M_ADDRESS_MASK_64)) & ~((1ull << 21) - 1)) | IA32_PG_P | IA32_PG_RW | IA32_PG_PS;
>    }
>
>    return NULL;
> @@ -244,6 +252,7 @@ _ModuleEntryPoint (
>    // Hook page fault handler to handle >4G request.
>    //
>    PageFaultIdtTable.PageFaultContext.Page1GSupport = EntrypointContext->Page1GSupport;
> +  PageFaultIdtTable.PageFaultContext.PteMemoryEncryptionAddressOrMask = EntrypointContext->PteMemoryEncryptionAddressOrMask;
>    IdtEntry = (IA32_IDT_GATE_DESCRIPTOR *) (X64Idtr.Base + (14 * sizeof (IA32_IDT_GATE_DESCRIPTOR)));
>    HookPageFaultHandler (IdtEntry, &(PageFaultIdtTable.PageFaultContext));
>
> @@ -298,4 +307,4 @@ _ModuleEntryPoint (
>    //
>    ASSERT (FALSE);
>    return EFI_SUCCESS;
> -}
> \ No newline at end of file
> +}
>