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

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

[Eric Dong]

V3 changes:
No need to change inf file. Also update commit message to include regression info.

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

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>
---
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
index f164c1713b..53ed76c6e6 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
@@ -1105,6 +1105,14 @@ S3RestoreConfig2 (
       //
       SetInterruptState (InterruptStatus);
 
+      if (sizeof(UINTN) == sizeof(UINT32)) {
+        //
+        // Paging maybe enabled. If current mode is 32 bit mode and code try to
+        // enable 64 bit mode page table, it will cause GP fault.
+        // To avoid conflict configuration, disable paging first anyway.
+        //
+        AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
+      }
       AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
 
       //
-- 
2.15.0.windows.1

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

[Ni, Ruiyu]

On 10/9/2018 2:01 PM, Eric Dong wrote:
> V3 changes:
> No need to change inf file. Also update commit message to include regression info.
> 
> 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
> 
> 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>
> ---
>   UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index f164c1713b..53ed76c6e6 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -1105,6 +1105,14 @@ S3RestoreConfig2 (
>         //
>         SetInterruptState (InterruptStatus);
>   
> +      if (sizeof(UINTN) == sizeof(UINT32)) {
> +        //
> +        // Paging maybe enabled. If current mode is 32 bit mode and code try to
> +        // enable 64 bit mode page table, it will cause GP fault.
> +        // To avoid conflict configuration, disable paging first anyway.
> +        //
> +        AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> +      }
>         AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
>   
>         //
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray

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

[Laszlo Ersek]

On 10/09/18 08:01, Eric Dong wrote:
> V3 changes:
> No need to change inf file. Also update commit message to include regression info.
> 
> 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
> 
> 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>
> ---
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index f164c1713b..53ed76c6e6 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -1105,6 +1105,14 @@ S3RestoreConfig2 (
>        //
>        SetInterruptState (InterruptStatus);
>  
> +      if (sizeof(UINTN) == sizeof(UINT32)) {

I think we usually insert a space character after the "sizeof" operator
in such cases.

> +        //
> +        // Paging maybe enabled. If current mode is 32 bit mode and code try to
> +        // enable 64 bit mode page table, it will cause GP fault.
> +        // To avoid conflict configuration, disable paging first anyway.
> +        //
> +        AsmWriteCr0 (AsmReadCr0 () & (~BIT31));

The bit-manipulation is valid, but only because this code is restricted
to 32-bit mode, where UINTN is UINT32. For clarity, I think
~(UINTN)BIT31 would be better. (The AsmWriteCr0() and AsmReadCr0()
BaseLib functions work with UINTN, but the type of BIT31 is UINT32.)

> +      }
>        AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
>  
>        //
> 

So, my main point:

Would it make sense to avoid a write to CR0 if paging is disabled? Such as:

  UINTN Cr0;

  if (sizeof (UINTN) == sizeof (UINT32)) {
    Cr0 = AsmReadCr0 ();
    if ((Cr0 & BIT31) != 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.
      //
      Cr0 &= ~(UINTN)BIT31;
      AsmWriteCr0 (Cr0);
    }
  }

I haven't tested this patch yet, it's just that I'm generally concerned
about CR *writes* under KVM that aren't absolutely necessary. OVMF does
not enable the PEI Stack Guard, so in practice, disabling paging is not
necessary (because it is never enabled anyway, for now). Therefore I
would like to save the CR0 write, in the IA32X64 build of OVMF.

Of course, I don't insist on the exact code example that I wrote above;
it's just illustration.

In summary, I suggest:

- please consider making the CR0 write conditional on *both* being in
  32-bit mode *and* BIT31 being set in CR0,

- for clarity, please use ~(UINTN)BIT31 as mask (even though it makes no
  practical difference).

What do you think?

Thanks!
Laszlo

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

[Ni, Ruiyu]

On 10/9/2018 4:25 PM, Laszlo Ersek wrote:
> On 10/09/18 08:01, Eric Dong wrote:
>> V3 changes:
>> No need to change inf file. Also update commit message to include regression info.
>>
>> 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
>>
>> 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>
>> ---
>>   UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> index f164c1713b..53ed76c6e6 100644
>> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> @@ -1105,6 +1105,14 @@ S3RestoreConfig2 (
>>         //
>>         SetInterruptState (InterruptStatus);
>>   
>> +      if (sizeof(UINTN) == sizeof(UINT32)) {
> 
> I think we usually insert a space character after the "sizeof" operator
> in such cases.
> 
>> +        //
>> +        // Paging maybe enabled. If current mode is 32 bit mode and code try to
>> +        // enable 64 bit mode page table, it will cause GP fault.
>> +        // To avoid conflict configuration, disable paging first anyway.
>> +        //
>> +        AsmWriteCr0 (AsmReadCr0 () & (~BIT31));
> 
> The bit-manipulation is valid, but only because this code is restricted
> to 32-bit mode, where UINTN is UINT32. For clarity, I think
> ~(UINTN)BIT31 would be better. (The AsmWriteCr0() and AsmReadCr0()
> BaseLib functions work with UINTN, but the type of BIT31 is UINT32.)
> 
>> +      }
>>         AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3);
>>   
>>         //
>>
> 
> So, my main point:
> 
> Would it make sense to avoid a write to CR0 if paging is disabled? Such as:
> 
>    UINTN Cr0;
> 
>    if (sizeof (UINTN) == sizeof (UINT32)) {
>      Cr0 = AsmReadCr0 ();
>      if ((Cr0 & BIT31) != 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.
>        //
>        Cr0 &= ~(UINTN)BIT31;
>        AsmWriteCr0 (Cr0);
>      }
>    }
> 
> I haven't tested this patch yet, it's just that I'm generally concerned
> about CR *writes* under KVM that aren't absolutely necessary. OVMF does
> not enable the PEI Stack Guard, so in practice, disabling paging is not
> necessary (because it is never enabled anyway, for now). Therefore I
> would like to save the CR0 write, in the IA32X64 build of OVMF.
> 
> Of course, I don't insist on the exact code example that I wrote above;
> it's just illustration.
> 
> In summary, I suggest:
> 
> - please consider making the CR0 write conditional on *both* being in
>    32-bit mode *and* BIT31 being set in CR0,
> 
> - for clarity, please use ~(UINTN)BIT31 as mask (even though it makes no
>    practical difference).
Actually we could use IA32_CR0 structure to avoid BIT31 usage.

> 
> What do you think?
> 
> Thanks!
> Laszlo
> 


-- 
Thanks,
Ray

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

[Laszlo Ersek]

On 10/09/18 10:59, Ni, Ruiyu wrote:
> On 10/9/2018 4:25 PM, Laszlo Ersek wrote:

>> - for clarity, please use ~(UINTN)BIT31 as mask (even though it makes no
>>    practical difference).
> Actually we could use IA32_CR0 structure to avoid BIT31 usage.

That's a great idea.

I wish I had known about IA32_CR0 (and IA32_CR4) earlier! Now we have a
bunch of ad-hoc bitmask macros around the tree, e.g. CR0_WP, CR0_PG,
CR4_PSE, CR4_PAE in "UefiCpuPkg/CpuDxe/CpuPageTable.c".

Thanks!
Laszlo