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
> Cc: Laszlo Ersek
> Cc: Jian J Wang
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong
> ---
> 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 ();
>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 (
>,
>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:X64fails works
> +# ===
> +#
> +
> [Sources]
>S3Resume.c
>
>
With the whitespace changes above (no need to repost just because of
them):
Reviewed-by: Laszlo Ersek
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
Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel