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

["Laszlo Ersek" <lersek@redhat.com>]

On 07/24/19 09:03, Ni, Ray wrote:
> 
> 
>> -----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.

Makes sense, thanks.
Laszlo