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

["Wang, Jian J" <jian.j.wang@intel.com>]

Mike,

Thanks for the feedback. I'll update the patch as what you suggested. And if no more other comments, I'll check in the updated one today (or tonight).

Jian

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Saturday, September 30, 2017 12:27 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [PATCH] UefiCpuPkg/CpuDxe: Fix assert issue on IA32 platform
> 
> 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