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

[Laszlo Ersek <lersek@redhat.com>]

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
>