Re: [Patch v5] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 10/11/2018 3:05 PM, Eric Dong wrote:
> V5:
> 1. Add ASSERT to indicate this assumption that environment is 32 bit mode.
> 2. Add description in INF about this driver's expected result
>     in different environment.
> 
> V4:
> Only disable paging when it is enabled.
> 
> V3 changes:
> No need to change inf file.
> 
> V2 changes:
> Only disable paging in 32 bit mode, no matter it is enable or not.
> 
> V1 changes:
> PEI Stack Guard needs to enable paging. This might cause #GP if code
> trying to write CR3 register with PML4 page table while the processor
> is enabled with PAE paging.
> 
> Simply disabling paging before updating CR3 can solve this conflict.
> 
> It's an regression caused by change: 0a0d5296e448fc350de1594c49b9c0deff7fad60
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1232
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>   UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c       | 17 +++++++++++++++++
>   UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 10 ++++++++++
>   2 files changed, 27 insertions(+)
> 
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index f164c1713b..8415ab1583 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -964,6 +964,7 @@ S3RestoreConfig2 (
>     VOID                                          *GuidHob;
>     BOOLEAN                                       Build4GPageTableOnly;
>     BOOLEAN                                       InterruptStatus;
> +  IA32_CR0                                      CR0Reg;
>   
>     TempAcpiS3Context = 0;
>     TempEfiBootScriptExecutorVariable = 0;
> @@ -1045,6 +1046,13 @@ S3RestoreConfig2 (
>     //
>     GuidHob = GetFirstGuidHob (&gEfiAcpiVariableGuid);
>     if (GuidHob != NULL) {
> +    //
> +    // Below SwitchStack/AsmEnablePaging64 function has
> +    // assumption that it's in 32 bits mode now.
> +    // Add ASSERT code to indicate this assumption.
> +    //
> +    ASSERT(sizeof (UINTN) == sizeof (UINT32));
> +
>       Status = PeiServicesLocatePpi (
>                                 &gPeiSmmAccessPpiGuid,
>                                 0,
> @@ -1105,6 +1113,15 @@ S3RestoreConfig2 (
>         //
>         SetInterruptState (InterruptStatus);
>   
> +      CR0Reg.UintN = AsmReadCr0 ();
> +      if (CR0Reg.Bits.PG != 0) {
> +        //
> +        // We're in 32-bit mode, with paging enabled. We can't set CR3 to
> +        // the 64-bit page tables without first disabling paging.
> +        //
> +        CR0Reg.Bits.PG = 0;
> +        AsmWriteCr0 (CR0Reg.UintN);
> +      }
>         AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
>   
>         //
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> index 6ce1bf944c..1d0740526f 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> @@ -33,6 +33,16 @@
>   #  VALID_ARCHITECTURES           = IA32 X64
>   #
>   
> +#
> +# This module is not always workable in IA32 and X64 mode. It has below result:
> +# when it works with SMM mode:
> +# ===============================
> +#           SMM:used  SMM:unused
> +# PEI:IA32   works      works
> +# PEI:X64    fails      works
> +# ===============================
> +#
> +
>   [Sources]
>     S3Resume.c
>   
> 
Can you change CR0Reg to Cr0?
With this change, Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray