* [Patch v3] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table. @ 2018-10-09 6:01 Eric Dong 2018-10-09 7:48 ` Ni, Ruiyu 2018-10-09 8:25 ` Laszlo Ersek 0 siblings, 2 replies; 5+ messages in thread From: Eric Dong @ 2018-10-09 6:01 UTC (permalink / raw) To: edk2-devel; +Cc: Ruiyu Ni, Laszlo Ersek, Jian J Wang V3 changes: No need to change inf file. Also update commit message to include regression info. 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 | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c index f164c1713b..53ed76c6e6 100644 --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c @@ -1105,6 +1105,14 @@ S3RestoreConfig2 ( // SetInterruptState (InterruptStatus); + if (sizeof(UINTN) == sizeof(UINT32)) { + // + // Paging maybe enabled. If current mode is 32 bit mode and code try to + // enable 64 bit mode page table, it will cause GP fault. + // To avoid conflict configuration, disable paging first anyway. + // + AsmWriteCr0 (AsmReadCr0 () & (~BIT31)); + } AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3); // -- 2.15.0.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch v3] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table. 2018-10-09 6:01 [Patch v3] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table Eric Dong @ 2018-10-09 7:48 ` Ni, Ruiyu 2018-10-09 8:25 ` Laszlo Ersek 1 sibling, 0 replies; 5+ messages in thread From: Ni, Ruiyu @ 2018-10-09 7:48 UTC (permalink / raw) To: Eric Dong, edk2-devel; +Cc: Laszlo Ersek, Jian J Wang On 10/9/2018 2:01 PM, Eric Dong wrote: > V3 changes: > No need to change inf file. Also update commit message to include regression info. > > 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 | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > index f164c1713b..53ed76c6e6 100644 > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > @@ -1105,6 +1105,14 @@ S3RestoreConfig2 ( > // > SetInterruptState (InterruptStatus); > > + if (sizeof(UINTN) == sizeof(UINT32)) { > + // > + // Paging maybe enabled. If current mode is 32 bit mode and code try to > + // enable 64 bit mode page table, it will cause GP fault. > + // To avoid conflict configuration, disable paging first anyway. > + // > + AsmWriteCr0 (AsmReadCr0 () & (~BIT31)); > + } > AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3); > > // > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com> -- Thanks, Ray ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch v3] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table. 2018-10-09 6:01 [Patch v3] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table Eric Dong 2018-10-09 7:48 ` Ni, Ruiyu @ 2018-10-09 8:25 ` Laszlo Ersek 2018-10-09 8:59 ` Ni, Ruiyu 1 sibling, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2018-10-09 8:25 UTC (permalink / raw) To: Eric Dong, edk2-devel; +Cc: Ruiyu Ni, Jian J Wang On 10/09/18 08:01, Eric Dong wrote: > V3 changes: > No need to change inf file. Also update commit message to include regression info. > > 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 | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > index f164c1713b..53ed76c6e6 100644 > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > @@ -1105,6 +1105,14 @@ S3RestoreConfig2 ( > // > SetInterruptState (InterruptStatus); > > + if (sizeof(UINTN) == sizeof(UINT32)) { I think we usually insert a space character after the "sizeof" operator in such cases. > + // > + // Paging maybe enabled. If current mode is 32 bit mode and code try to > + // enable 64 bit mode page table, it will cause GP fault. > + // To avoid conflict configuration, disable paging first anyway. > + // > + AsmWriteCr0 (AsmReadCr0 () & (~BIT31)); The bit-manipulation is valid, but only because this code is restricted to 32-bit mode, where UINTN is UINT32. For clarity, I think ~(UINTN)BIT31 would be better. (The AsmWriteCr0() and AsmReadCr0() BaseLib functions work with UINTN, but the type of BIT31 is UINT32.) > + } > AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3); > > // > So, my main point: Would it make sense to avoid a write to CR0 if paging is disabled? Such as: UINTN Cr0; if (sizeof (UINTN) == sizeof (UINT32)) { Cr0 = AsmReadCr0 (); if ((Cr0 & BIT31) != 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. // Cr0 &= ~(UINTN)BIT31; AsmWriteCr0 (Cr0); } } I haven't tested this patch yet, it's just that I'm generally concerned about CR *writes* under KVM that aren't absolutely necessary. OVMF does not enable the PEI Stack Guard, so in practice, disabling paging is not necessary (because it is never enabled anyway, for now). Therefore I would like to save the CR0 write, in the IA32X64 build of OVMF. Of course, I don't insist on the exact code example that I wrote above; it's just illustration. In summary, I suggest: - please consider making the CR0 write conditional on *both* being in 32-bit mode *and* BIT31 being set in CR0, - for clarity, please use ~(UINTN)BIT31 as mask (even though it makes no practical difference). What do you think? Thanks! Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch v3] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table. 2018-10-09 8:25 ` Laszlo Ersek @ 2018-10-09 8:59 ` Ni, Ruiyu 2018-10-09 9:14 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Ni, Ruiyu @ 2018-10-09 8:59 UTC (permalink / raw) To: Laszlo Ersek, Eric Dong, edk2-devel On 10/9/2018 4:25 PM, Laszlo Ersek wrote: > On 10/09/18 08:01, Eric Dong wrote: >> V3 changes: >> No need to change inf file. Also update commit message to include regression info. >> >> 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 | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> index f164c1713b..53ed76c6e6 100644 >> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> @@ -1105,6 +1105,14 @@ S3RestoreConfig2 ( >> // >> SetInterruptState (InterruptStatus); >> >> + if (sizeof(UINTN) == sizeof(UINT32)) { > > I think we usually insert a space character after the "sizeof" operator > in such cases. > >> + // >> + // Paging maybe enabled. If current mode is 32 bit mode and code try to >> + // enable 64 bit mode page table, it will cause GP fault. >> + // To avoid conflict configuration, disable paging first anyway. >> + // >> + AsmWriteCr0 (AsmReadCr0 () & (~BIT31)); > > The bit-manipulation is valid, but only because this code is restricted > to 32-bit mode, where UINTN is UINT32. For clarity, I think > ~(UINTN)BIT31 would be better. (The AsmWriteCr0() and AsmReadCr0() > BaseLib functions work with UINTN, but the type of BIT31 is UINT32.) > >> + } >> AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3); >> >> // >> > > So, my main point: > > Would it make sense to avoid a write to CR0 if paging is disabled? Such as: > > UINTN Cr0; > > if (sizeof (UINTN) == sizeof (UINT32)) { > Cr0 = AsmReadCr0 (); > if ((Cr0 & BIT31) != 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. > // > Cr0 &= ~(UINTN)BIT31; > AsmWriteCr0 (Cr0); > } > } > > I haven't tested this patch yet, it's just that I'm generally concerned > about CR *writes* under KVM that aren't absolutely necessary. OVMF does > not enable the PEI Stack Guard, so in practice, disabling paging is not > necessary (because it is never enabled anyway, for now). Therefore I > would like to save the CR0 write, in the IA32X64 build of OVMF. > > Of course, I don't insist on the exact code example that I wrote above; > it's just illustration. > > In summary, I suggest: > > - please consider making the CR0 write conditional on *both* being in > 32-bit mode *and* BIT31 being set in CR0, > > - for clarity, please use ~(UINTN)BIT31 as mask (even though it makes no > practical difference). Actually we could use IA32_CR0 structure to avoid BIT31 usage. > > What do you think? > > Thanks! > Laszlo > -- Thanks, Ray ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch v3] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table. 2018-10-09 8:59 ` Ni, Ruiyu @ 2018-10-09 9:14 ` Laszlo Ersek 0 siblings, 0 replies; 5+ messages in thread From: Laszlo Ersek @ 2018-10-09 9:14 UTC (permalink / raw) To: Ni, Ruiyu, Eric Dong, edk2-devel On 10/09/18 10:59, Ni, Ruiyu wrote: > On 10/9/2018 4:25 PM, Laszlo Ersek wrote: >> - for clarity, please use ~(UINTN)BIT31 as mask (even though it makes no >> practical difference). > Actually we could use IA32_CR0 structure to avoid BIT31 usage. That's a great idea. I wish I had known about IA32_CR0 (and IA32_CR4) earlier! Now we have a bunch of ad-hoc bitmask macros around the tree, e.g. CR0_WP, CR0_PG, CR4_PSE, CR4_PAE in "UefiCpuPkg/CpuDxe/CpuPageTable.c". Thanks! Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-09 9:14 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-09 6:01 [Patch v3] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table Eric Dong 2018-10-09 7:48 ` Ni, Ruiyu 2018-10-09 8:25 ` Laszlo Ersek 2018-10-09 8:59 ` Ni, Ruiyu 2018-10-09 9:14 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox