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:[email protected]] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, October 10, 2018 9:19 PM
>> To: Yao, Jiewen <[email protected]>; Dong, Eric <[email protected]>;
>> [email protected]
>> Cc: Ni, Ruiyu <[email protected]>
>> 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:[email protected]]
>>>> Sent: Wednesday, October 10, 2018 9:04 PM
>>>> To: Yao, Jiewen <[email protected]>; Dong, Eric
>> <[email protected]>;
>>>> [email protected]
>>>> Cc: Ni, Ruiyu <[email protected]>
>>>> 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:[email protected]] On Behalf
>> Of
>>>>>> Eric Dong
>>>>>> Sent: Wednesday, October 10, 2018 3:44 PM
>>>>>> To: [email protected]
>>>>>> Cc: Ni, Ruiyu <[email protected]>; Laszlo Ersek <[email protected]>
>>>>>> 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 <[email protected]>
>>>>>> Cc: Laszlo Ersek <[email protected]>
>>>>>> Cc: Jian J Wang <[email protected]>
>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>>> Signed-off-by:Eric Dong <[email protected]>
>>>>>> Signed-off-by: Eric Dong <[email protected]>
>>>>>> ---
>>>>>> 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
>>>>>> [email protected]
>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> [email protected]
>> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
