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

[Laszlo Ersek <lersek@redhat.com>]

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