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

[Laszlo Ersek <lersek@redhat.com>]

On 10/11/18 09:05, 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

(1) please strip the trailing space character here

> +    // 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:

(2) please strip the trailing space character here


> +# when it works with SMM mode:
> +# ===============================
> +#           SMM:used  SMM:unused
> +# PEI:IA32   works      works
> +# PEI:X64    fails      works
> +# ===============================
> +#
> +
>  [Sources]
>    S3Resume.c
>
>

With the whitespace changes above (no need to repost just because of
them):

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I also regression-tested this patch on top of commit 8122c6bc8b6f, with
OVMF, in the following configurations:

- IA32,    SMM,    S3 enabled:  boot & suspend/resume PASS
- IA32X64, SMM,    S3 enabled:  boot & suspend/resume PASS
- X64,     SMM,    S3 enabled:  boot failure, due to S3Verification().
                                This is by design, see commit
                                5133d1f1d297. Therefore this test case
                                is also PASS.
- X64,     SMM,    S3 disabled: boot PASS
- IA32,    no SMM, S3 enabled:  boot & suspend/resume PASS
- IA32X64, no SMM, S3 enabled:  boot & suspend/resume PASS
- X64,     no SMM, S3 enabled:  boot & suspend/resume PASS

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo