Re: [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Jian,

The names of the functions do not look right to me.

I think the checks being made check to see if paging
is enabled.  Also, the check being made is to see if both
paging is enabled and page address extensions is enabled.

If you use the IA32_CR0 and IA32_CR4 structures from 
BaseLib.h, the updates will also be easier to understand 
than using BITxx macros.

Maybe use the following:


BOOLEAN
IsPagingAndPageAddressExtensionsEnabled (
  VOID
  )
{
  IA32_CR0  Cr0;
  IA32_CR4  Cr4;

  Cr0.UintN = AsmReadCr0 ();
  Cr4.UintN = AsmReadCr4 ();
  return ((Cr0.Bits.PG != 0) && (Cr4.Bits.PAE != 0));
}

Best regards,

Mike

> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, September 28, 2017 10:40 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32
> platform
> 
> This patch is to fix an assert issue during booting IA32
> platforms
> such as OvmfIa32 or Quark. This issue is caused by trying to
> access
> page table on a platform without page table. A check is added
> to
> avoid the assert.
> 
> Bug tracker: https://bugzilla.tianocore.org/show_bug.cgi?id=724
> 
> c: Star Zeng <star.zeng@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.c | 48
> ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c
> b/UefiCpuPkg/CpuDxe/CpuDxe.c
> index 4e8fa100e0..85a520079f 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -683,7 +683,7 @@ SetGcdMemorySpaceAttributes (
> 
>  **/
>  VOID
> -RefreshGcdMemoryAttributes (
> +RefreshMemoryAttributesFromMtrr (
>    VOID
>    )
>  {
> @@ -704,14 +704,9 @@ RefreshGcdMemoryAttributes (
>    UINT32
> FirmwareVariableMtrrCount;
>    UINT8                               DefaultMemoryType;
> 
> -  if (!IsMtrrSupported ()) {
> -    return;
> -  }
> -
>    FirmwareVariableMtrrCount = GetFirmwareVariableMtrrCount ();
>    ASSERT (FirmwareVariableMtrrCount <=
> MTRR_NUMBER_OF_VARIABLE_MTRR);
> 
> -  mIsFlushingGCD = TRUE;
>    MemorySpaceMap = NULL;
> 
>    //
> @@ -862,11 +857,44 @@ RefreshGcdMemoryAttributes (
>    if (MemorySpaceMap != NULL) {
>      FreePool (MemorySpaceMap);
>    }
> +}
> 
> -  //
> -  // Update page attributes
> -  //
> -  RefreshGcdMemoryAttributesFromPaging();
> +/**
> + Check if paging is enabled or not.
> +**/
> +BOOLEAN
> +IsPagingSupported (
> +  VOID
> +  )
> +{
> +  return (
> +          (AsmReadCr0 () & BIT31) != 0
> +          &&
> +          (AsmReadCr4 () & BIT5) != 0
> +         );
> +}
> +
> +/**
> +  Refreshes the GCD Memory Space attributes according to MTRRs
> and Paging.
> +
> +  This function refreshes the GCD Memory Space attributes
> according to MTRRs
> +  and page tables.
> +
> +**/
> +VOID
> +RefreshGcdMemoryAttributes (
> +  VOID
> +  )
> +{
> +  mIsFlushingGCD = TRUE;
> +
> +  if (IsMtrrSupported ()) {
> +    RefreshMemoryAttributesFromMtrr ();
> +  }
> +
> +  if (IsPagingSupported ()) {
> +    RefreshGcdMemoryAttributesFromPaging ();
> +  }
> 
>    mIsFlushingGCD = FALSE;
>  }
> --
> 2.14.1.windows.1