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

[Laszlo Ersek <lersek@redhat.com>]

On 10/10/18 15:30, Yao, Jiewen wrote:
> 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));
> ==================

Looks good!

> 
> I also think we need add your table to INF.
> ==================
>          SMM:used  SMM:unused
> PEI:IA32  works      works
> PEI:X64   fails       works
> ==================

Ditto. :)

Thanks!
Laszlo

>> -----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