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

[Laszlo Ersek <lersek@redhat.com>]

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