* Re: [Patch v5] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-11 7:05 [Patch v5] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table Eric Dong
@ 2018-10-11 7:38 ` Ni, Ruiyu
2018-10-11 12:06 ` Laszlo Ersek
1 sibling, 0 replies; 3+ messages in thread
From: Ni, Ruiyu @ 2018-10-11 7:38 UTC (permalink / raw)
To: Eric Dong, edk2-devel; +Cc: Laszlo Ersek, Jian J Wang
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Patch v5] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-11 7:05 [Patch v5] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table Eric Dong
2018-10-11 7:38 ` Ni, Ruiyu
@ 2018-10-11 12:06 ` Laszlo Ersek
1 sibling, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2018-10-11 12:06 UTC (permalink / raw)
To: Eric Dong; +Cc: edk2-devel, Ruiyu Ni, Jian J Wang
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
^ permalink raw reply [flat|nested] 3+ messages in thread