1. Patchew
  2. EDK2
  3. [edk2] [Patch v3 0/5] Change CpuS3Data memory type and address limitation
  4. View patch

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

Eric Dong posted 5 patches 6 years, 6 months ago
There is a newer version of this series
[edk2] [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.
Posted by Eric Dong 6 years, 6 months ago 
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;
 }
 
 /**
-- 
2.15.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.
Posted by Laszlo Ersek 6 years, 6 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.
Posted by Dong, Eric 6 years, 6 months ago 
Hi Laszlo,

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Friday, August 10, 2018 11:40 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use
> GDT/IDT saved in Smram.
> 
> 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?
>

Agree to do this change in my next version change. Will send new patches after finishing AllocateZeroPages related discussion.

> If you prefer to do that as an incremental patch, that's fine too.
> 
> Thanks,
> Laszlo
> _______________________________________________
> 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
Re: [edk2] [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.
Posted by Laszlo Ersek 6 years, 6 months ago 
On 08/13/18 03:54, Dong, Eric wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, August 10, 2018 11:40 PM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use
>> GDT/IDT saved in Smram.
>>
>> 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?
>>
> 
> Agree to do this change in my next version change. Will send new patches after finishing AllocateZeroPages related discussion.

OK, cool, thanks! I will test that version then.

Laszlo

> 
>> If you prefer to do that as an incremental patch, that's fine too.
>>
>> Thanks,
>> Laszlo
>> _______________________________________________
>> 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
Re: [edk2] [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.
Posted by Ni, Ruiyu 6 years, 6 months ago 

Thanks/Ray

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, August 10, 2018 11:40 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT
> saved in Smram.
> 
> 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?
Great code cleanup to remove 3 global variables.
> 
> If you prefer to do that as an incremental patch, that's fine too.
> 
> Thanks,
> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.