* [Patch v5] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
@ 2018-10-11 7:05 Eric Dong
2018-10-11 7:38 ` Ni, Ruiyu
2018-10-11 12:06 ` Laszlo Ersek
0 siblings, 2 replies; 3+ messages in thread
From: Eric Dong @ 2018-10-11 7:05 UTC (permalink / raw)
To: edk2-devel; +Cc: Ruiyu Ni, Laszlo Ersek, Jian J Wang
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
--
2.15.0.windows.1
^ permalink raw reply related [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: 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
end of thread, other threads:[~2018-10-11 12:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox