Re: [edk2-devel] [PATCH 2/4] UefiCpuPkg/CpuDxe: Support parsing 5-level page table

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, July 23, 2019 5:23 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 2/4] UefiCpuPkg/CpuDxe: Support parsing
> 5-level page table
> 
> On 07/22/19 10:15, Ni, Ray wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> >
> > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index c369b44f12..8e959eb2b7 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    Page table management support.
> >
> > -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +  Copyright (c) 2017 - 2019, Intel Corporation. All rights
> > + reserved.<BR>
> >    Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent @@ -276,25 +276,43 @@
> > GetPageTableEntry (
> >    UINTN                 Index2;
> >    UINTN                 Index3;
> >    UINTN                 Index4;
> > +  UINTN                 Index5;
> >    UINT64                *L1PageTable;
> >    UINT64                *L2PageTable;
> >    UINT64                *L3PageTable;
> >    UINT64                *L4PageTable;
> > +  UINT64                *L5PageTable;
> >    UINT64                AddressEncMask;
> > +  IA32_CR4              Cr4;
> > +  BOOLEAN               Enable5LevelPaging;
> >
> >    ASSERT (PagingContext != NULL);
> >
> > +  Index5 = ((UINTN)RShiftU64 (Address, 48)) &
> PAGING_PAE_INDEX_MASK;
> >    Index4 = ((UINTN)RShiftU64 (Address, 39)) &
> PAGING_PAE_INDEX_MASK;
> >    Index3 = ((UINTN)Address >> 30) & PAGING_PAE_INDEX_MASK;
> >    Index2 = ((UINTN)Address >> 21) & PAGING_PAE_INDEX_MASK;
> >    Index1 = ((UINTN)Address >> 12) & PAGING_PAE_INDEX_MASK;
> >
> > +  Cr4.UintN = AsmReadCr4 ();
> > +  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> > +
> >    // Make sure AddressEncMask is contained to smallest supported address
> field.
> >    //
> >    AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask)
> &
> > PAGING_1G_ADDRESS_MASK_64;
> >
> >    if (PagingContext->MachineType == IMAGE_FILE_MACHINE_X64) {
> > -    L4PageTable = (UINT64 *)(UINTN)PagingContext-
> >ContextData.X64.PageTableBase;
> > +    if (Enable5LevelPaging) {
> > +      L5PageTable = (UINT64 *)(UINTN)PagingContext-
> >ContextData.X64.PageTableBase;
> > +      if (L5PageTable[Index5] == 0) {
> > +        *PageAttribute = PageNone;
> > +        return NULL;
> > +      }
> > +
> > +      L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] &
> ~AddressEncMask & PAGING_4K_ADDRESS_MASK_64);
> > +    } else {
> > +      L4PageTable = (UINT64 *)(UINTN)PagingContext-
> >ContextData.X64.PageTableBase;
> > +    }
> >      if (L4PageTable[Index4] == 0) {
> >        *PageAttribute = PageNone;
> >        return NULL;
> >
> 
> The patch seems to be a no-op if Enable5LevelPaging is FALSE, so that's good.
> 
> Questions:
> 
> (1) Same question as under patch #1: is the CR4 check reliable on AMD
> processors too?

Replied in 1/4 thread.

> 
> (2) Should we read CR4 (or call CPUID) every time this function is invoked?
> Can we perform the check at CpuDxe startup, and cache the result?
It's a good suggestion. Will address it in V2.


> 
> (3) Should we consider the PCD (from patch #3) before accessing the
> hardware (CR4 / CPUID alike)?
I want to avoid touching PCD in this CPU driver.
All the information is taken in DxeIpl and pass to CPU driver through Cr4.LA57.

> 
> Thanks
> Laszlo