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

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

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