Re: [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.

[Laszlo Ersek <lersek@redhat.com>]

On 08/10/18 06:19, Eric Dong wrote:
> Current implementation copies GDT/IDT at SmmReadyToLock point
> from ACPI NVS memory to Smram. Later at S3 resume phase, it restores
> memory saved in Smram to ACPI NVS. Driver can directly use GDT/IDT
> saved in Smram instead of restore the original ACPI NVS memory.
> 
> This patch do this change.
> 
> Test Done:
>   Do the OS boot and S3 resume test.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 0b8ef70359..764d8276d3 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -448,13 +448,6 @@ PrepareApStartupVector (
>    CopyMem ((VOID *) (UINTN) &mExchangeInfo->GdtrProfile, (VOID *) (UINTN) mAcpiCpuData.GdtrProfile, sizeof (IA32_DESCRIPTOR));
>    CopyMem ((VOID *) (UINTN) &mExchangeInfo->IdtrProfile, (VOID *) (UINTN) mAcpiCpuData.IdtrProfile, sizeof (IA32_DESCRIPTOR));
>  
> -  //
> -  // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI NVS memory
> -  //
> -  CopyMem ((VOID *) mExchangeInfo->GdtrProfile.Base, mGdtForAp, mExchangeInfo->GdtrProfile.Limit + 1);
> -  CopyMem ((VOID *) mExchangeInfo->IdtrProfile.Base, mIdtForAp, mExchangeInfo->IdtrProfile.Limit + 1);
> -  CopyMem ((VOID *)(UINTN) mAcpiCpuData.ApMachineCheckHandlerBase, mMachineCheckHandlerForAp, mAcpiCpuData.ApMachineCheckHandlerSize);
> -
>    mExchangeInfo->StackStart  = (VOID *) (UINTN) mAcpiCpuData.StackAddress;
>    mExchangeInfo->StackSize   = mAcpiCpuData.StackSize;
>    mExchangeInfo->BufferStart = (UINT32) StartupVector;
> @@ -901,6 +894,10 @@ GetAcpiCpuData (
>    CopyMem (mGdtForAp, (VOID *)Gdtr->Base, Gdtr->Limit + 1);
>    CopyMem (mIdtForAp, (VOID *)Idtr->Base, Idtr->Limit + 1);
>    CopyMem (mMachineCheckHandlerForAp, (VOID *)(UINTN)mAcpiCpuData.ApMachineCheckHandlerBase, mAcpiCpuData.ApMachineCheckHandlerSize);
> +
> +  Gdtr->Base = (UINTN)mGdtForAp;
> +  Idtr->Base = (UINTN)mIdtForAp;
> +  mAcpiCpuData.ApMachineCheckHandlerBase = (EFI_PHYSICAL_ADDRESS)(UINTN)mMachineCheckHandlerForAp;
>  }
>  
>  /**
> 

This patch looks good to me (I'm ready to give R-b), but I think we can
simplify the code further. Can we replace the mGdtForAp, mIdtForAp and
mMachineCheckHandlerForAp global variables, with GdtForAp, IdtForAp and
MachineCheckHandlerForAp local variables, used only in the
GetAcpiCpuData() function?

If you prefer to do that as an incremental patch, that's fine too.

Thanks,
Laszlo