Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table.

["Yao, Jiewen" <jiewen.yao@intel.com>]

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