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

["Duran, Leo" <leo.duran@amd.com>]

Hi Star,

Please see my reply below.

Thanks,
Leo.

> -----Original Message-----
> From: Zeng, Star [mailto:star.zeng@intel.com]
> Sent: Tuesday, February 21, 2017 7:20 PM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org
> Cc: Singh, Brijesh <brijesh.singh@amd.com>; Tian, Feng
> <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v3 2/4] MdeModulePkg/Universal/CapsulePei:
> Add support for PCD PcdPteMemoryEncryptionAddressOrMask
> 
> Shouldn't
> 
> ((*PageFaultContext->PageFaultUplink[PageFaultContext->PageFaultIndex]
> & PageFaultContext->PhyMask) == Address)
> 
> be
> 
> (((*PageFaultContext->PageFaultUplink[PageFaultContext-
> >PageFaultIndex] & ~(AddressSetMask &
> PAGING_4K_ADDRESS_MASK_64)) & PageFaultContext->PhyMask) ==
> Address)
> 
> like you did at other place?
[Duran, Leo] 
Yes, I agree... I will take care of that in v4 of the set.

> 
> Thanks,
> Star
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Duran, Leo
> Sent: Wednesday, February 22, 2017 12:43 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@ml01.01.org
> Cc: Singh, Brijesh <brijesh.singh@amd.com>; Tian, Feng
> <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2] [PATCH v3 2/4] MdeModulePkg/Universal/CapsulePei:
> Add support for PCD PcdPteMemoryEncryptionAddressOrMask
> 
> Hi Star,
> 
> Please double-check the complete [PATCH v3 2/4].
> 
> Yes, there is a non-functional change where I did break a 'very long' line into
> 2 lines as you noted (I can put that back as it was before if so required).
> However the intended functional changes are applied in the rest of the patch
> in lines where I reference 'AddressSetMask'.
> 
> As for [PATCH v3 4/4]
> The intended functional changes are applied... please confirm, or please let
> me know what seems to be missing.
> 
> Thanks,
> Leo.
> 
> > -----Original Message-----
> > From: Zeng, Star [mailto:star.zeng@intel.com]
> > Sent: Monday, February 20, 2017 12:05 AM
> > To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org
> > Cc: Laszlo Ersek <lersek@redhat.com>; Feng Tian <feng.tian@intel.com>;
> > Singh, Brijesh <brijesh.singh@amd.com>; star.zeng@intel.com
> > Subject: Re: [edk2] [PATCH v3 2/4] MdeModulePkg/Universal/CapsulePei:
> > Add support for PCD PcdPteMemoryEncryptionAddressOrMask
> >
> > 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-
> > >PageFaultInde
> > > + x] & 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
> > > +}
> > >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel