From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com)
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
 by groups.io with SMTP; Tue, 23 Jul 2019 02:22:38 -0700
Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id 553FD3083362;
	Tue, 23 Jul 2019 09:22:38 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-52.ams2.redhat.com [10.36.117.52])
	by smtp.corp.redhat.com (Postfix) with ESMTP id 7F6F91001B20;
	Tue, 23 Jul 2019 09:22:37 +0000 (UTC)
Subject: Re: [edk2-devel] [PATCH 2/4] UefiCpuPkg/CpuDxe: Support parsing 5-level page table
To: devel@edk2.groups.io, ray.ni@intel.com
Cc: Eric Dong <eric.dong@intel.com>
References: <20190722081547.56100-1-ray.ni@intel.com>
 <20190722081547.56100-3-ray.ni@intel.com>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <5d8ff62d-606e-ecee-ee72-6f7597388ebd@redhat.com>
Date: Tue, 23 Jul 2019 11:22:36 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20190722081547.56100-3-ray.ni@intel.com>
X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Tue, 23 Jul 2019 09:22:38 +0000 (UTC)
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

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?

(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?

(3) Should we consider the PCD (from patch #3) before accessing the
hardware (CR4 / CPUID alike)?

Thanks
Laszlo