From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"Dong, Eric" <eric.dong@intel.com>
Cc: "Ni, Ray" <ray.ni@intel.com>, Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.
Date: Fri, 4 Sep 2020 03:48:13 +0000 [thread overview]
Message-ID: <CY4PR11MB12887AC720B6743871F4AEC98C2D0@CY4PR11MB1288.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200904015156.1273-1-eric.dong@intel.com>
I provided the feedback in https://bugzilla.tianocore.org/show_bug.cgi?id=2954.
I suggest we justify the requirement at first.
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong, Eric
> Sent: Friday, September 4, 2020 9:52 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for
> Cr3/GDT/IDT.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2954
>
> AP needs to run from real mode to 32bit mode to LONG mode. Page table
> (pointed by Cr3) and GDT are necessary to set up to correct value when
> CPU execution mode is switched to LONG mode.
> AP uses the same location page table (Cr3) and GDT as what BSP uses.
> But when the page table or GDT is above 4GB, it's impossible for CPU
> to use because GDTR.base and Cr3 are 32bits before switching to LONG
> mode.
>
> This patch adds check for the Cr3, GDT and IDT to not above 4G limitation.
>
> The check is avoided -- assumed successful -- if the new
> PcdEnableCpuApCr3GdtIdtCheck is FALSE (which is the default). The reason
> is that the 32-bit requirement is always ensured by edk2 itself; the
> requirement is only possibly invalidated by a particular UEFI shell
> application that manually moves the GDT/IDT/CR3 above 4GB. Platforms
> that don't intend to be compatible with such UEFI applications need not
> set the PCD to TRUE.
>
> If the PCD is TRUE and the check fails, then the StartupAllAPs(),
> StartupThisAP(), SwitchBSP() and EnableDisableAP() MP service APIs are
> rejected at once. Reporting an error immediately is more graceful than
> hanging when the APs attempt to switch to long mode.
>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>
> V3:
>
> only add check for the DxeMplib, no need for PeiMpLib.
>
>
>
> V2:
>
> Change the check point. Just in the different caller to make the logic
>
> clear. V1 patch add check just before the use of the code. It make the
>
> logic complicated.
>
>
> UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 +
> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 97 +++++++++++++++++++
> UefiCpuPkg/UefiCpuPkg.dec | 4 +
> UefiCpuPkg/UefiCpuPkg.uni | 2 +
> 4 files changed, 104 insertions(+)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 1771575c69..7792df516e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -74,5 +74,6 @@
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds
> ## CONSUMES
>
> gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled ## CONSUMES
>
> gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase ##
> SOMETIMES_CONSUMES
>
> + gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck ##
> CONSUMES
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
> CONSUMES
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase ##
> CONSUMES
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 2c00d72dde..332b4447bb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -29,6 +29,66 @@ VOID *mReservedApLoopFunc = NULL;
> UINTN mReservedTopOfApStack;
>
> volatile UINT32 mNumberToFinish = 0;
>
>
>
> +/**
>
> + Check whether Cr3/GDT/IDT value valid for the APs.
>
> +
>
> + @retval TRUE Pass the check.
>
> + @retval FALSE Fail the check.
>
> +
>
> +**/
>
> +BOOLEAN
>
> +ValidCr3GdtIdtCheck (
>
> + VOID
>
> + )
>
> +{
>
> + IA32_DESCRIPTOR Gdtr;
>
> + IA32_DESCRIPTOR Idtr;
>
> +
>
> + if (!PcdGetBool (PcdEnableCpuApCr3GdtIdtCheck)) {
>
> + return TRUE;
>
> + }
>
> +
>
> + //
>
> + // AP needs to run from real mode to 32bit mode to LONG mode. Page table
>
> + // (pointed by Cr3) and GDT are necessary to set up to correct value when
>
> + // CPU execution mode is switched to LONG mode. IDT also necessary if the
>
> + // exception happened.
>
> + // AP uses the same location page table (Cr3) and GDT/IDT as what BSP uses.
>
> + // But when the page table or GDT is above 4GB, it's impossible for CPU
>
> + // to use because GDTR.base and Cr3 are 32bits before switching to LONG
>
> + // mode.
>
> + // Here add check for the Cr3, GDT.Base and range, IDT.Base and range are
>
> + // not above 32 bits limitation.
>
> + //
>
> + if (AsmReadCr3 () >= BASE_4GB) {
>
> + return FALSE;
>
> + }
>
> +
>
> + AsmReadGdtr (&Gdtr);
>
> + //
>
> + // Here code needs to check both Gdtr.Base and Gdtr.Base + Gdtr.Limit
>
> + // below BASE_4GB, but Gdtr.Base + Gdtr.Limit below BASE_4GB also means
>
> + // Gdtr.Base below BASE_4GB. so here just add Gdtr.Base + Gdtr.Limit
>
> + // check.
>
> + //
>
> + if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) {
>
> + return FALSE;
>
> + }
>
> +
>
> + AsmReadIdtr (&Idtr);
>
> + //
>
> + // Here code needs to check both Idtr.Base and Idtr.Base + Idtr.Limit
>
> + // below BASE_4GB, but Idtr.Base + Idtr.Limit below BASE_4GB also means
>
> + // Idtr.Base below BASE_4GB. so here just add Idtr.Base + Idtr.Limit
>
> + // check.
>
> + //
>
> + if (Idtr.Base + Idtr.Limit >= BASE_4GB) {
>
> + return FALSE;
>
> + }
>
> +
>
> + return TRUE;
>
> +}
>
> +
>
> /**
>
> Enable Debug Agent to support source debugging on AP function.
>
>
>
> @@ -394,6 +454,15 @@ MpInitChangeApLoopCallback (
> {
>
> CPU_MP_DATA *CpuMpData;
>
>
>
> + //
>
> + // Check the Cr3/GDT/IDT before waking up AP.
>
> + // If the check return fail, it will block later
>
> + // OS boot, so halt the system here.
>
> + //
>
> + if (!ValidCr3GdtIdtCheck ()) {
>
> + CpuDeadLoop ();
>
> + }
>
> +
>
> CpuMpData = GetCpuMpData ();
>
> CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>
> CpuMpData->Pm16CodeSegment = GetProtectedMode16CS ();
>
> @@ -676,6 +745,13 @@ MpInitLibStartupAllAPs (
> {
>
> EFI_STATUS Status;
>
>
>
> + //
>
> + // Check whether Cr3/GDT/IDT valid for AP.
>
> + //
>
> + if (!ValidCr3GdtIdtCheck()) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> +
>
> //
>
> // Temporarily stop checkAllApsStatus for avoid resource dead-lock.
>
> //
>
> @@ -783,6 +859,13 @@ MpInitLibStartupThisAP (
> {
>
> EFI_STATUS Status;
>
>
>
> + //
>
> + // Check whether Cr3/GDT/IDT valid for AP.
>
> + //
>
> + if (!ValidCr3GdtIdtCheck()) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> +
>
> //
>
> // temporarily stop checkAllApsStatus for avoid resource dead-lock.
>
> //
>
> @@ -839,6 +922,13 @@ MpInitLibSwitchBSP (
> EFI_TIMER_ARCH_PROTOCOL *Timer;
>
> UINT64 TimerPeriod;
>
>
>
> + //
>
> + // Check whether Cr3/GDT/IDT valid for AP.
>
> + //
>
> + if (!ValidCr3GdtIdtCheck()) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> +
>
> TimerPeriod = 0;
>
> //
>
> // Locate Timer Arch Protocol
>
> @@ -912,6 +1002,13 @@ MpInitLibEnableDisableAP (
> EFI_STATUS Status;
>
> BOOLEAN TempStopCheckState;
>
>
>
> + //
>
> + // Check whether Cr3/GDT/IDT valid for AP.
>
> + //
>
> + if (!ValidCr3GdtIdtCheck()) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> +
>
> TempStopCheckState = FALSE;
>
> //
>
> // temporarily stop checkAllAPsStatus for initialize parameters.
>
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index d83c084467..08ec36e76c 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -148,6 +148,10 @@
> # @Prompt Lock SMM Feature Control MSR.
>
>
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOL
> EAN|0x3213210B
>
>
>
> + ## This value specifies whether need to check the Cr3/GDT/IDT value for the
> APs.
>
> + # @Prompt Enable Cr3/GDT/IDT value check for the APs.
>
> +
> gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEA
> N|0x30000044
>
> +
>
> [PcdsFixedAtBuild]
>
> ## List of exception vectors which need switching stack.
>
> # This PCD will only take into effect if PcdCpuStackGuard is enabled.
>
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index 219c1963bf..3461a83003 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -96,6 +96,8 @@
>
>
> #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_PROMP
> T #language en-US "Lock SMM Feature Control MSR"
>
>
>
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdEnableCpuApCr3GdtIdtCheck_PROMPT
> #language en-US "Enable Cr3/GDT/IDT value check for the APs"
>
> +
>
> #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_HELP
> #language en-US "Lock SMM Feature Control MSR?<BR><BR>\n"
>
> "TRUE - locked.<BR>\n"
>
> "FALSE - unlocked.<BR>"
>
> --
> 2.23.0.windows.1
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#65019): https://edk2.groups.io/g/devel/message/65019
> Mute This Topic: https://groups.io/mt/76621468/1772286
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiewen.yao@intel.com]
> -=-=-=-=-=-=
next prev parent reply other threads:[~2020-09-04 3:48 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-04 1:51 [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT Dong, Eric
2020-09-04 3:48 ` Yao, Jiewen [this message]
2020-09-04 7:08 ` [edk2-devel] " Laszlo Ersek
2020-09-04 7:30 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CY4PR11MB12887AC720B6743871F4AEC98C2D0@CY4PR11MB1288.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox