* [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
@ 2018-10-09 1:51 Eric Dong
2018-10-09 1:59 ` Wang, Jian J
2018-10-09 2:05 ` Dong, Eric
0 siblings, 2 replies; 16+ messages in thread
From: Eric Dong @ 2018-10-09 1:51 UTC (permalink / raw)
To: edk2-devel; +Cc: Ruiyu Ni, Laszlo Ersek, Jian J Wang
PEI Stack Guard needs to enable paging. This might cause #GP in the
transition from 32-bit PEI to 64-bit SMM due to the 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.
Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
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>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 7 +++++++
UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
2 files changed, 8 insertions(+)
diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
index f164c1713b..b3bf56e13d 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
@@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
//
SetInterruptState (InterruptStatus);
+ if (PcdGetBool (PcdCpuStackGuard)) {
+ //
+ // Paging already been enabled, to avoid conflict configuration,
+ // disable paging first anyway.
+ //
+ AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
+ }
AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
//
diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
index 6ce1bf944c..0f131d19df 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
@@ -90,6 +90,7 @@
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES
[Depex]
TRUE
--
2.15.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-09 1:51 [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table Eric Dong
@ 2018-10-09 1:59 ` Wang, Jian J
2018-10-09 2:03 ` Wang, Jian J
2018-10-09 2:05 ` Dong, Eric
1 sibling, 1 reply; 16+ messages in thread
From: Wang, Jian J @ 2018-10-09 1:59 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Laszlo Ersek
Thanks for fixing this issue.
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
> -----Original Message-----
> From: Dong, Eric
> Sent: Tuesday, October 09, 2018 9:51 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Wang,
> Jian J <jian.j.wang@intel.com>
> Subject: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new
> page table.
>
> PEI Stack Guard needs to enable paging. This might cause #GP in the
> transition from 32-bit PEI to 64-bit SMM due to the 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.
>
> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> 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>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 7 +++++++
> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
> 2 files changed, 8 insertions(+)
>
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index f164c1713b..b3bf56e13d 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
> //
> SetInterruptState (InterruptStatus);
>
> + if (PcdGetBool (PcdCpuStackGuard)) {
> + //
> + // Paging already been enabled, to avoid conflict configuration,
> + // disable paging first anyway.
> + //
> + AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> + }
> AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
>
> //
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> index 6ce1bf944c..0f131d19df 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> @@ -90,6 +90,7 @@
> [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ##
> SOMETIMES_CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
> CONSUMES
>
> [Depex]
> TRUE
> --
> 2.15.0.windows.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-09 1:59 ` Wang, Jian J
@ 2018-10-09 2:03 ` Wang, Jian J
2018-10-09 2:27 ` Dong, Eric
0 siblings, 1 reply; 16+ messages in thread
From: Wang, Jian J @ 2018-10-09 2:03 UTC (permalink / raw)
To: edk2-devel, Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Laszlo Ersek
Forgot one thing: this issue is caused by introducing of PEI stack guard so please
add the revision id of related check in for references in message.
Regards,
Jian
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> Sent: Tuesday, October 09, 2018 9:59 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> creating new page table.
>
> Thanks for fixing this issue.
>
> Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
>
> > -----Original Message-----
> > From: Dong, Eric
> > Sent: Tuesday, October 09, 2018 9:51 AM
> > To: edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Wang,
> > Jian J <jian.j.wang@intel.com>
> > Subject: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating
> new
> > page table.
> >
> > PEI Stack Guard needs to enable paging. This might cause #GP in the
> > transition from 32-bit PEI to 64-bit SMM due to the 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.
> >
> > Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> > 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>
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 7 +++++++
> > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
> > 2 files changed, 8 insertions(+)
> >
> > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > index f164c1713b..b3bf56e13d 100644
> > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
> > //
> > SetInterruptState (InterruptStatus);
> >
> > + if (PcdGetBool (PcdCpuStackGuard)) {
> > + //
> > + // Paging already been enabled, to avoid conflict configuration,
> > + // disable paging first anyway.
> > + //
> > + AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> > + }
> > AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
> >
> > //
> > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > index 6ce1bf944c..0f131d19df 100644
> > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > @@ -90,6 +90,7 @@
> > [Pcd]
> > gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ##
> > SOMETIMES_CONSUMES
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> > ## CONSUMES
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
> > CONSUMES
> >
> > [Depex]
> > TRUE
> > --
> > 2.15.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-09 1:51 [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table Eric Dong
2018-10-09 1:59 ` Wang, Jian J
@ 2018-10-09 2:05 ` Dong, Eric
2018-10-09 2:15 ` Ni, Ruiyu
1 sibling, 1 reply; 16+ messages in thread
From: Dong, Eric @ 2018-10-09 2:05 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Laszlo Ersek, Wang, Jian J
Add BZ link for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=1232
Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Eric Dong
> Sent: Tuesday, October 9, 2018 9:51 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> creating new page table.
>
> PEI Stack Guard needs to enable paging. This might cause #GP in the
> transition from 32-bit PEI to 64-bit SMM due to the 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.
>
> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> 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>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 7 +++++++
> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
> 2 files changed, 8 insertions(+)
>
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index f164c1713b..b3bf56e13d 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
> //
> SetInterruptState (InterruptStatus);
>
> + if (PcdGetBool (PcdCpuStackGuard)) {
> + //
> + // Paging already been enabled, to avoid conflict configuration,
> + // disable paging first anyway.
> + //
> + AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> + }
> AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
>
> //
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> index 6ce1bf944c..0f131d19df 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> @@ -90,6 +90,7 @@
> [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ##
> SOMETIMES_CONSUMES
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
> CONSUMES
>
> [Depex]
> TRUE
> --
> 2.15.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-09 2:05 ` Dong, Eric
@ 2018-10-09 2:15 ` Ni, Ruiyu
2018-10-09 8:09 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Ni, Ruiyu @ 2018-10-09 2:15 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek, Wang, Jian J
On 10/9/2018 10:05 AM, Dong, Eric wrote:
> Add BZ link for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=1232
>
> Thanks,
> Eric
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Eric Dong
>> Sent: Tuesday, October 9, 2018 9:51 AM
>> To: edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
>> creating new page table.
>>
>> PEI Stack Guard needs to enable paging. This might cause #GP in the
>> transition from 32-bit PEI to 64-bit SMM due to the 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.
>>
>> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
>> 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>
>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>> ---
>> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 7 +++++++
>> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> index f164c1713b..b3bf56e13d 100644
>> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
>> //
>> SetInterruptState (InterruptStatus);
>>
>> + if (PcdGetBool (PcdCpuStackGuard)) {
>> + //
>> + // Paging already been enabled, to avoid conflict configuration,
>> + // disable paging first anyway.
>> + //
>> + AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
>> + }
Two comments:
1. We'd better not map the PcdCpuStackGuard to paging-enable. Maybe some
other feature also enables the paging in PEI phase but the
PcdCpuStackGuard is FALSE.
2. When PEI is in 64bit mode, disabling paging may not work because
paging-enable is a must in 64bit mode.
>> AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
>>
>> //
>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>> index 6ce1bf944c..0f131d19df 100644
>> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>> @@ -90,6 +90,7 @@
>> [Pcd]
>> gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ##
>> SOMETIMES_CONSUMES
>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
>> ask ## CONSUMES
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
>> CONSUMES
>>
>> [Depex]
>> TRUE
>> --
>> 2.15.0.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-09 2:03 ` Wang, Jian J
@ 2018-10-09 2:27 ` Dong, Eric
0 siblings, 0 replies; 16+ messages in thread
From: Dong, Eric @ 2018-10-09 2:27 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Laszlo Ersek
Hi Jian,
Not aware this comments before I send out V2 patch, will include this info in commit message when I check in the code.
Thanks,
Eric
> -----Original Message-----
> From: Wang, Jian J
> Sent: Tuesday, October 9, 2018 10:03 AM
> To: edk2-devel <edk2-devel-bounces@lists.01.org>; Dong, Eric
> <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> creating new page table.
>
> Forgot one thing: this issue is caused by introducing of PEI stack guard so
> please add the revision id of related check in for references in message.
>
> Regards,
> Jian
>
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> > Sent: Tuesday, October 09, 2018 9:59 AM
> > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
> > before creating new page table.
> >
> > Thanks for fixing this issue.
> >
> > Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
> >
> > > -----Original Message-----
> > > From: Dong, Eric
> > > Sent: Tuesday, October 09, 2018 9:51 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>
> > > Subject: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> > > creating
> > new
> > > page table.
> > >
> > > PEI Stack Guard needs to enable paging. This might cause #GP in the
> > > transition from 32-bit PEI to 64-bit SMM due to the 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.
> > >
> > > Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> > > 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>
> > > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > > ---
> > > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 7 +++++++
> > > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
> > > 2 files changed, 8 insertions(+)
> > >
> > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > > b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > > index f164c1713b..b3bf56e13d 100644
> > > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> > > @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
> > > //
> > > SetInterruptState (InterruptStatus);
> > >
> > > + if (PcdGetBool (PcdCpuStackGuard)) {
> > > + //
> > > + // Paging already been enabled, to avoid conflict configuration,
> > > + // disable paging first anyway.
> > > + //
> > > + AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> > > + }
> > > AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
> > >
> > > //
> > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > index 6ce1bf944c..0f131d19df 100644
> > > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > @@ -90,6 +90,7 @@
> > > [Pcd]
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ##
> > > SOMETIMES_CONSUMES
> > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask
> > > ## CONSUMES
> > > + gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
> > > CONSUMES
> > >
> > > [Depex]
> > > TRUE
> > > --
> > > 2.15.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-09 2:15 ` Ni, Ruiyu
@ 2018-10-09 8:09 ` Laszlo Ersek
2018-10-09 8:26 ` Ni, Ruiyu
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-10-09 8:09 UTC (permalink / raw)
To: Ni, Ruiyu, Dong, Eric, edk2-devel@lists.01.org
On 10/09/18 04:15, Ni, Ruiyu wrote:
> On 10/9/2018 10:05 AM, Dong, Eric wrote:
>> Add BZ link for this issue:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=1232
>>
>> Thanks,
>> Eric
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Eric Dong
>>> Sent: Tuesday, October 9, 2018 9:51 AM
>>> To: edk2-devel@lists.01.org
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>>> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
>>> creating new page table.
>>>
>>> PEI Stack Guard needs to enable paging. This might cause #GP in the
>>> transition from 32-bit PEI to 64-bit SMM due to the 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.
>>>
>>> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
>>> 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>
>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>>> ---
>>> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 7 +++++++
>>> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 1 +
>>> 2 files changed, 8 insertions(+)
>>>
>>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>>> index f164c1713b..b3bf56e13d 100644
>>> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>>> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>>> @@ -1105,6 +1105,13 @@ S3RestoreConfig2 (
>>> //
>>> SetInterruptState (InterruptStatus);
>>>
>>> + if (PcdGetBool (PcdCpuStackGuard)) {
>>> + //
>>> + // Paging already been enabled, to avoid conflict
>>> configuration,
>>> + // disable paging first anyway.
>>> + //
>>> + AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
>>> + }
>
> Two comments:
> 1. We'd better not map the PcdCpuStackGuard to paging-enable. Maybe some
> other feature also enables the paging in PEI phase but the
> PcdCpuStackGuard is FALSE.
I think I agree.
> 2. When PEI is in 64bit mode, disabling paging may not work because
> paging-enable is a must in 64bit mode.
I think this case is academic. S3Resume2Pei does not support 64-bit PEI
with SMM enabled. This is why we have commit 5133d1f1d297 ("OvmfPkg:
replace README fine print about X64 SMM S3 with PlatformPei check",
2015-11-30) in OVMF.
Anyway, I'm making this comment in the general sense only. I'm not
suggesting that we disable paging unconditionally. Actually, I believe,
I will suggest (under the v3 posting) restricting the write to CR0 even
more.
Thanks!
Laszlo
>
>>> AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
>>>
>>> //
>>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>>> index 6ce1bf944c..0f131d19df 100644
>>> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>>> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>>> @@ -90,6 +90,7 @@
>>> [Pcd]
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ##
>>> SOMETIMES_CONSUMES
>>>
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
>>> ask ## CONSUMES
>>> +
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
>>> CONSUMES
>>>
>>> [Depex]
>>> TRUE
>>> --
>>> 2.15.0.windows.1
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-09 8:09 ` Laszlo Ersek
@ 2018-10-09 8:26 ` Ni, Ruiyu
2018-10-09 8:54 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Ni, Ruiyu @ 2018-10-09 8:26 UTC (permalink / raw)
To: Laszlo Ersek, Dong, Eric, edk2-devel@lists.01.org
On 10/9/2018 4:09 PM, Laszlo Ersek wrote:
>>
>> Two comments:
>> 1. We'd better not map the PcdCpuStackGuard to paging-enable. Maybe some
>> other feature also enables the paging in PEI phase but the
>> PcdCpuStackGuard is FALSE.
>
> I think I agree.
>
>> 2. When PEI is in 64bit mode, disabling paging may not work because
>> paging-enable is a must in 64bit mode.
>
> I think this case is academic. S3Resume2Pei does not support 64-bit PEI
> with SMM enabled. This is why we have commit 5133d1f1d297 ("OvmfPkg:
> replace README fine print about X64 SMM S3 with PlatformPei check",
> 2015-11-30) in OVMF.
I found that commit. I just checked S3Resume2Pei module. It does assumes
it's running in 32bit mode. And CpuS3.c also assumes S3Resume2Pei is
running in 32bit mode.
>
> Anyway, I'm making this comment in the general sense only. I'm not
> suggesting that we disable paging unconditionally. Actually, I believe,
> I will suggest (under the v3 posting) restricting the write to CR0 even
> more.
Why?
>
> Thanks!
> Laszlo
>
>>
>>>> AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
>>>>
>>>> //
>>>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>>>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>>>> index 6ce1bf944c..0f131d19df 100644
>>>> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>>>> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>>>> @@ -90,6 +90,7 @@
>>>> [Pcd]
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ##
>>>> SOMETIMES_CONSUMES
>>>>
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
>>>> ask ## CONSUMES
>>>> +
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
>>>> CONSUMES
>>>>
>>>> [Depex]
>>>> TRUE
>>>> --
>>>> 2.15.0.windows.1
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-09 8:26 ` Ni, Ruiyu
@ 2018-10-09 8:54 ` Laszlo Ersek
0 siblings, 0 replies; 16+ messages in thread
From: Laszlo Ersek @ 2018-10-09 8:54 UTC (permalink / raw)
To: Ni, Ruiyu, Dong, Eric, edk2-devel@lists.01.org
On 10/09/18 10:26, Ni, Ruiyu wrote:
> On 10/9/2018 4:09 PM, Laszlo Ersek wrote:
>>>
>>> Two comments:
>>> 1. We'd better not map the PcdCpuStackGuard to paging-enable. Maybe some
>>> other feature also enables the paging in PEI phase but the
>>> PcdCpuStackGuard is FALSE.
>>
>> I think I agree.
>>
>>> 2. When PEI is in 64bit mode, disabling paging may not work because
>>> paging-enable is a must in 64bit mode.
>>
>> I think this case is academic. S3Resume2Pei does not support 64-bit PEI
>> with SMM enabled. This is why we have commit 5133d1f1d297 ("OvmfPkg:
>> replace README fine print about X64 SMM S3 with PlatformPei check",
>> 2015-11-30) in OVMF.
>
> I found that commit. I just checked S3Resume2Pei module. It does assumes
> it's running in 32bit mode. And CpuS3.c also assumes S3Resume2Pei is
> running in 32bit mode.
>
>>
>> Anyway, I'm making this comment in the general sense only. I'm not
>> suggesting that we disable paging unconditionally. Actually, I believe,
>> I will suggest (under the v3 posting) restricting the write to CR0 even
>> more.
> Why?
I explained under v3.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
@ 2018-10-10 7:43 Eric Dong
2018-10-10 7:58 ` Yao, Jiewen
0 siblings, 1 reply; 16+ messages in thread
From: Eric Dong @ 2018-10-10 7:43 UTC (permalink / raw)
To: edk2-devel; +Cc: Ruiyu Ni, Laszlo Ersek, Jian J Wang
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
Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
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>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
index f164c1713b..c059c42db5 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;
@@ -1105,6 +1106,17 @@ S3RestoreConfig2 (
//
SetInterruptState (InterruptStatus);
+ if (sizeof (UINTN) == sizeof (UINT32)) {
+ 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);
//
--
2.15.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-10 7:43 Eric Dong
@ 2018-10-10 7:58 ` Yao, Jiewen
2018-10-10 13:03 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Yao, Jiewen @ 2018-10-10 7:58 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Laszlo Ersek
Hey
I do not think we need add if (sizeof (UINTN) == sizeof (UINT32))
This piece of code assume PEI is 32 bit.
The following code AsmEnablePaging64() does not work for X64.
Thank you
Yao Jiewen
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Eric Dong
> Sent: Wednesday, October 10, 2018 3:44 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> creating new page table.
>
> 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
>
> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> 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>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index f164c1713b..c059c42db5 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;
> @@ -1105,6 +1106,17 @@ S3RestoreConfig2 (
> //
> SetInterruptState (InterruptStatus);
>
> + if (sizeof (UINTN) == sizeof (UINT32)) {
> + 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);
>
> //
> --
> 2.15.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-10 7:58 ` Yao, Jiewen
@ 2018-10-10 13:03 ` Laszlo Ersek
2018-10-10 13:14 ` Yao, Jiewen
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-10-10 13:03 UTC (permalink / raw)
To: Yao, Jiewen, Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu
On 10/10/18 09:58, Yao, Jiewen wrote:
> Hey
> I do not think we need add if (sizeof (UINTN) == sizeof (UINT32))
>
> This piece of code assume PEI is 32 bit.
> The following code AsmEnablePaging64() does not work for X64.
I don't feel strongly about this particular question. Any decent
compiler will optimize away the UINTN size check, and IIRC Ray suggested
under v1 to explicitly restrict the new code to 32-bit. (Although, we
both confirmed that this PEIM only supported 32-bit PEI with SMM.)
Now, if the code is *clearly* restricted to 32-bit already -- do we
*declare* that fact somewhere? in the code or in the INF file? --, then
I agree we might not need the UINTN size check.
... Maybe we should split this driver into [Sources.IA32] and
[Sources.X64] more extensively that we currently do, and make the
X64+SMM case fail more *obviously* than it currently does.
I'll leave it up to you guys to decide if the UINTN size check should be
preserved in this patch. Once you have an agreement, I'd like to
regression-test the resultant version of the patch.
FWIW, this version (v4) does look good to me. Should Jiewen think the
UINTN size check is acceptable after all, I'd be ready to give my R-b
(and to regression-test the patch as well).
Thanks!
Laszlo
>
> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Eric Dong
>> Sent: Wednesday, October 10, 2018 3:44 PM
>> To: edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
>> creating new page table.
>>
>> 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
>>
>> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
>> 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>
>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>> ---
>> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> index f164c1713b..c059c42db5 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;
>> @@ -1105,6 +1106,17 @@ S3RestoreConfig2 (
>> //
>> SetInterruptState (InterruptStatus);
>>
>> + if (sizeof (UINTN) == sizeof (UINT32)) {
>> + 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);
>>
>> //
>> --
>> 2.15.0.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-10 13:03 ` Laszlo Ersek
@ 2018-10-10 13:14 ` Yao, Jiewen
2018-10-10 13:19 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Yao, Jiewen @ 2018-10-10 13:14 UTC (permalink / raw)
To: Laszlo Ersek, Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu
Good idea, Laszlo.
If we know something will fail, we had better make it fail as early as possible, and as obvious as possible.
I think we can do sth in INF:
1) Change # VALID_ARCHITECTURES = IA32
2) Change:
[Sources.IA32]
S3Resume.c
and remove [Sources.X64]
If we decide to enable X64 S3Resume, we can go back and add such support.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 10, 2018 9:04 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
> before creating new page table.
>
> On 10/10/18 09:58, Yao, Jiewen wrote:
> > Hey
> > I do not think we need add if (sizeof (UINTN) == sizeof (UINT32))
> >
> > This piece of code assume PEI is 32 bit.
> > The following code AsmEnablePaging64() does not work for X64.
>
> I don't feel strongly about this particular question. Any decent
> compiler will optimize away the UINTN size check, and IIRC Ray suggested
> under v1 to explicitly restrict the new code to 32-bit. (Although, we
> both confirmed that this PEIM only supported 32-bit PEI with SMM.)
>
> Now, if the code is *clearly* restricted to 32-bit already -- do we
> *declare* that fact somewhere? in the code or in the INF file? --, then
> I agree we might not need the UINTN size check.
>
> ... Maybe we should split this driver into [Sources.IA32] and
> [Sources.X64] more extensively that we currently do, and make the
> X64+SMM case fail more *obviously* than it currently does.
>
> I'll leave it up to you guys to decide if the UINTN size check should be
> preserved in this patch. Once you have an agreement, I'd like to
> regression-test the resultant version of the patch.
>
> FWIW, this version (v4) does look good to me. Should Jiewen think the
> UINTN size check is acceptable after all, I'd be ready to give my R-b
> (and to regression-test the patch as well).
>
> Thanks!
> Laszlo
>
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >> Eric Dong
> >> Sent: Wednesday, October 10, 2018 3:44 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> >> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
> >> creating new page table.
> >>
> >> 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
> >>
> >> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> >> 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>
> >> Signed-off-by: Eric Dong <eric.dong@intel.com>
> >> ---
> >> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 12
> ++++++++++++
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> >> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> >> index f164c1713b..c059c42db5 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;
> >> @@ -1105,6 +1106,17 @@ S3RestoreConfig2 (
> >> //
> >> SetInterruptState (InterruptStatus);
> >>
> >> + if (sizeof (UINTN) == sizeof (UINT32)) {
> >> + 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);
> >>
> >> //
> >> --
> >> 2.15.0.windows.1
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-10 13:14 ` Yao, Jiewen
@ 2018-10-10 13:19 ` Laszlo Ersek
2018-10-10 13:30 ` Yao, Jiewen
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-10-10 13:19 UTC (permalink / raw)
To: Yao, Jiewen, Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu
On 10/10/18 15:14, Yao, Jiewen wrote:
> Good idea, Laszlo.
>
> If we know something will fail, we had better make it fail as early as possible, and as obvious as possible.
>
> I think we can do sth in INF:
> 1) Change # VALID_ARCHITECTURES = IA32
> 2) Change:
> [Sources.IA32]
> S3Resume.c
> and remove [Sources.X64]
>
>
> If we decide to enable X64 S3Resume, we can go back and add such support.
This would be a bit heavy-handed however, as S3Resume2Pei works fine
with X64 PEI, but only without SMM.
There are four cases:
SMM:used SMM:unused
PEI:IA32 works works
PEI:X64 fails works
Only the X64+SMM case fails.
Thanks
Laszlo
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, October 10, 2018 9:04 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
>> before creating new page table.
>>
>> On 10/10/18 09:58, Yao, Jiewen wrote:
>>> Hey
>>> I do not think we need add if (sizeof (UINTN) == sizeof (UINT32))
>>>
>>> This piece of code assume PEI is 32 bit.
>>> The following code AsmEnablePaging64() does not work for X64.
>>
>> I don't feel strongly about this particular question. Any decent
>> compiler will optimize away the UINTN size check, and IIRC Ray suggested
>> under v1 to explicitly restrict the new code to 32-bit. (Although, we
>> both confirmed that this PEIM only supported 32-bit PEI with SMM.)
>>
>> Now, if the code is *clearly* restricted to 32-bit already -- do we
>> *declare* that fact somewhere? in the code or in the INF file? --, then
>> I agree we might not need the UINTN size check.
>>
>> ... Maybe we should split this driver into [Sources.IA32] and
>> [Sources.X64] more extensively that we currently do, and make the
>> X64+SMM case fail more *obviously* than it currently does.
>>
>> I'll leave it up to you guys to decide if the UINTN size check should be
>> preserved in this patch. Once you have an agreement, I'd like to
>> regression-test the resultant version of the patch.
>>
>> FWIW, this version (v4) does look good to me. Should Jiewen think the
>> UINTN size check is acceptable after all, I'd be ready to give my R-b
>> (and to regression-test the patch as well).
>>
>> Thanks!
>> Laszlo
>>
>>>
>>> Thank you
>>> Yao Jiewen
>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>>> Eric Dong
>>>> Sent: Wednesday, October 10, 2018 3:44 PM
>>>> To: edk2-devel@lists.01.org
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>>>> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before
>>>> creating new page table.
>>>>
>>>> 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
>>>>
>>>> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
>>>> 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>
>>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>>>> ---
>>>> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 12
>> ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>>>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>>>> index f164c1713b..c059c42db5 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;
>>>> @@ -1105,6 +1106,17 @@ S3RestoreConfig2 (
>>>> //
>>>> SetInterruptState (InterruptStatus);
>>>>
>>>> + if (sizeof (UINTN) == sizeof (UINT32)) {
>>>> + 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);
>>>>
>>>> //
>>>> --
>>>> 2.15.0.windows.1
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-10 13:19 ` Laszlo Ersek
@ 2018-10-10 13:30 ` Yao, Jiewen
2018-10-10 14:00 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Yao, Jiewen @ 2018-10-10 13:30 UTC (permalink / raw)
To: Laszlo Ersek, Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu
OK. I forget that OVMF uses a different lock box implementation.
If so, how about below:
==================
GuidHob = GetFirstGuidHob (&gEfiAcpiVariableGuid);
if (GuidHob != NULL) {
+ ASSERT(sizeof(UINTN) == sizeof(UINT32));
==================
I also think we need add your table to INF.
==================
SMM:used SMM:unused
PEI:IA32 works works
PEI:X64 fails works
==================
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, October 10, 2018 9:19 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
> before creating new page table.
>
> On 10/10/18 15:14, Yao, Jiewen wrote:
> > Good idea, Laszlo.
> >
> > If we know something will fail, we had better make it fail as early as
> possible, and as obvious as possible.
> >
> > I think we can do sth in INF:
> > 1) Change # VALID_ARCHITECTURES = IA32
> > 2) Change:
> > [Sources.IA32]
> > S3Resume.c
> > and remove [Sources.X64]
> >
> >
> > If we decide to enable X64 S3Resume, we can go back and add such
> support.
>
> This would be a bit heavy-handed however, as S3Resume2Pei works fine
> with X64 PEI, but only without SMM.
>
> There are four cases:
>
> SMM:used SMM:unused
> PEI:IA32 works works
> PEI:X64 fails works
>
> Only the X64+SMM case fails.
>
> Thanks
> Laszlo
>
>
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Wednesday, October 10, 2018 9:04 PM
> >> To: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric
> <eric.dong@intel.com>;
> >> edk2-devel@lists.01.org
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> >> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
> >> before creating new page table.
> >>
> >> On 10/10/18 09:58, Yao, Jiewen wrote:
> >>> Hey
> >>> I do not think we need add if (sizeof (UINTN) == sizeof (UINT32))
> >>>
> >>> This piece of code assume PEI is 32 bit.
> >>> The following code AsmEnablePaging64() does not work for X64.
> >>
> >> I don't feel strongly about this particular question. Any decent
> >> compiler will optimize away the UINTN size check, and IIRC Ray
> suggested
> >> under v1 to explicitly restrict the new code to 32-bit. (Although, we
> >> both confirmed that this PEIM only supported 32-bit PEI with SMM.)
> >>
> >> Now, if the code is *clearly* restricted to 32-bit already -- do we
> >> *declare* that fact somewhere? in the code or in the INF file? --, then
> >> I agree we might not need the UINTN size check.
> >>
> >> ... Maybe we should split this driver into [Sources.IA32] and
> >> [Sources.X64] more extensively that we currently do, and make the
> >> X64+SMM case fail more *obviously* than it currently does.
> >>
> >> I'll leave it up to you guys to decide if the UINTN size check should be
> >> preserved in this patch. Once you have an agreement, I'd like to
> >> regression-test the resultant version of the patch.
> >>
> >> FWIW, this version (v4) does look good to me. Should Jiewen think the
> >> UINTN size check is acceptable after all, I'd be ready to give my R-b
> >> (and to regression-test the patch as well).
> >>
> >> Thanks!
> >> Laszlo
> >>
> >>>
> >>> Thank you
> >>> Yao Jiewen
> >>>
> >>>> -----Original Message-----
> >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> Of
> >>>> Eric Dong
> >>>> Sent: Wednesday, October 10, 2018 3:44 PM
> >>>> To: edk2-devel@lists.01.org
> >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> >>>> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
> before
> >>>> creating new page table.
> >>>>
> >>>> 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
> >>>>
> >>>> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
> >>>> 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>
> >>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
> >>>> ---
> >>>> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 12
> >> ++++++++++++
> >>>> 1 file changed, 12 insertions(+)
> >>>>
> >>>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> >>>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> >>>> index f164c1713b..c059c42db5 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;
> >>>> @@ -1105,6 +1106,17 @@ S3RestoreConfig2 (
> >>>> //
> >>>> SetInterruptState (InterruptStatus);
> >>>>
> >>>> + if (sizeof (UINTN) == sizeof (UINT32)) {
> >>>> + 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);
> >>>>
> >>>> //
> >>>> --
> >>>> 2.15.0.windows.1
> >>>>
> >>>> _______________________________________________
> >>>> edk2-devel mailing list
> >>>> edk2-devel@lists.01.org
> >>>> https://lists.01.org/mailman/listinfo/edk2-devel
> >
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.
2018-10-10 13:30 ` Yao, Jiewen
@ 2018-10-10 14:00 ` Laszlo Ersek
0 siblings, 0 replies; 16+ messages in thread
From: Laszlo Ersek @ 2018-10-10 14:00 UTC (permalink / raw)
To: Yao, Jiewen, Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu
On 10/10/18 15:30, Yao, Jiewen wrote:
> OK. I forget that OVMF uses a different lock box implementation.
>
> If so, how about below:
> ==================
> GuidHob = GetFirstGuidHob (&gEfiAcpiVariableGuid);
> if (GuidHob != NULL) {
> + ASSERT(sizeof(UINTN) == sizeof(UINT32));
> ==================
Looks good!
>
> I also think we need add your table to INF.
> ==================
> SMM:used SMM:unused
> PEI:IA32 works works
> PEI:X64 fails works
> ==================
Ditto. :)
Thanks!
Laszlo
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, October 10, 2018 9:19 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
>> before creating new page table.
>>
>> On 10/10/18 15:14, Yao, Jiewen wrote:
>>> Good idea, Laszlo.
>>>
>>> If we know something will fail, we had better make it fail as early as
>> possible, and as obvious as possible.
>>>
>>> I think we can do sth in INF:
>>> 1) Change # VALID_ARCHITECTURES = IA32
>>> 2) Change:
>>> [Sources.IA32]
>>> S3Resume.c
>>> and remove [Sources.X64]
>>>
>>>
>>> If we decide to enable X64 S3Resume, we can go back and add such
>> support.
>>
>> This would be a bit heavy-handed however, as S3Resume2Pei works fine
>> with X64 PEI, but only without SMM.
>>
>> There are four cases:
>>
>> SMM:used SMM:unused
>> PEI:IA32 works works
>> PEI:X64 fails works
>>
>> Only the X64+SMM case fails.
>>
>> Thanks
>> Laszlo
>>
>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Wednesday, October 10, 2018 9:04 PM
>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric
>> <eric.dong@intel.com>;
>>>> edk2-devel@lists.01.org
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>>>> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
>>>> before creating new page table.
>>>>
>>>> On 10/10/18 09:58, Yao, Jiewen wrote:
>>>>> Hey
>>>>> I do not think we need add if (sizeof (UINTN) == sizeof (UINT32))
>>>>>
>>>>> This piece of code assume PEI is 32 bit.
>>>>> The following code AsmEnablePaging64() does not work for X64.
>>>>
>>>> I don't feel strongly about this particular question. Any decent
>>>> compiler will optimize away the UINTN size check, and IIRC Ray
>> suggested
>>>> under v1 to explicitly restrict the new code to 32-bit. (Although, we
>>>> both confirmed that this PEIM only supported 32-bit PEI with SMM.)
>>>>
>>>> Now, if the code is *clearly* restricted to 32-bit already -- do we
>>>> *declare* that fact somewhere? in the code or in the INF file? --, then
>>>> I agree we might not need the UINTN size check.
>>>>
>>>> ... Maybe we should split this driver into [Sources.IA32] and
>>>> [Sources.X64] more extensively that we currently do, and make the
>>>> X64+SMM case fail more *obviously* than it currently does.
>>>>
>>>> I'll leave it up to you guys to decide if the UINTN size check should be
>>>> preserved in this patch. Once you have an agreement, I'd like to
>>>> regression-test the resultant version of the patch.
>>>>
>>>> FWIW, this version (v4) does look good to me. Should Jiewen think the
>>>> UINTN size check is acceptable after all, I'd be ready to give my R-b
>>>> (and to regression-test the patch as well).
>>>>
>>>> Thanks!
>>>> Laszlo
>>>>
>>>>>
>>>>> Thank you
>>>>> Yao Jiewen
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>> Of
>>>>>> Eric Dong
>>>>>> Sent: Wednesday, October 10, 2018 3:44 PM
>>>>>> To: edk2-devel@lists.01.org
>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>>>>>> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging
>> before
>>>>>> creating new page table.
>>>>>>
>>>>>> 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
>>>>>>
>>>>>> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957
>>>>>> 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>
>>>>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>>>>>> ---
>>>>>> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 12
>>>> ++++++++++++
>>>>>> 1 file changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>>>>>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>>>>>> index f164c1713b..c059c42db5 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;
>>>>>> @@ -1105,6 +1106,17 @@ S3RestoreConfig2 (
>>>>>> //
>>>>>> SetInterruptState (InterruptStatus);
>>>>>>
>>>>>> + if (sizeof (UINTN) == sizeof (UINT32)) {
>>>>>> + 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);
>>>>>>
>>>>>> //
>>>>>> --
>>>>>> 2.15.0.windows.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> edk2-devel mailing list
>>>>>> edk2-devel@lists.01.org
>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-10-10 14:00 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-09 1:51 [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table Eric Dong
2018-10-09 1:59 ` Wang, Jian J
2018-10-09 2:03 ` Wang, Jian J
2018-10-09 2:27 ` Dong, Eric
2018-10-09 2:05 ` Dong, Eric
2018-10-09 2:15 ` Ni, Ruiyu
2018-10-09 8:09 ` Laszlo Ersek
2018-10-09 8:26 ` Ni, Ruiyu
2018-10-09 8:54 ` Laszlo Ersek
-- strict thread matches above, loose matches on Subject: below --
2018-10-10 7:43 Eric Dong
2018-10-10 7:58 ` Yao, Jiewen
2018-10-10 13:03 ` Laszlo Ersek
2018-10-10 13:14 ` Yao, Jiewen
2018-10-10 13:19 ` Laszlo Ersek
2018-10-10 13:30 ` Yao, Jiewen
2018-10-10 14:00 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox