From: "Laszlo Ersek" <lersek@redhat.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>,
"Dong, Eric" <eric.dong@intel.com>
Cc: "Ni, Ray" <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.
Date: Fri, 4 Sep 2020 09:08:50 +0200 [thread overview]
Message-ID: <bc1cab75-8220-ab60-f173-f9faa16b6d2c@redhat.com> (raw)
In-Reply-To: <CY4PR11MB12887AC720B6743871F4AEC98C2D0@CY4PR11MB1288.namprd11.prod.outlook.com>
Hello Jiewen,
On 09/04/20 05:48, Yao, Jiewen wrote:
> I provided the feedback in https://bugzilla.tianocore.org/show_bug.cgi?id=2954.
>
> I suggest we justify the requirement at first.
in comment <https://bugzilla.tianocore.org/show_bug.cgi?id=2954#c2>, you
write,
> I don't treat it as a valid test case, to move page table, GDT, IDT in
> UEFI environment.
>
> The CPU context should be managed by CPU driver, and only CPU driver.
> Other driver or application should only change the system state via
> UEFI/PI spec defined API. Otherwise, the system may run into an
> unknown state.
>
> IMHO, I dont think it is a valid case.
and I couldn't agree more with you -- I'm also convinced that what the
particular UEFI application does is invalid.
However, it seems like Eric really needs this change. I don't know his
business case (maybe he's not at liberty to share it), but when he
implied it was really important, I trusted him. So I figured we can
accommodate this use case (even if we don't exactly know all the
details, and even if we know for a fact that what the UEFI application
does is *invalid*, per spec) -- but then the impact, on both
performance, and on code complexity, should be minimal, for platforms
that do not care. (And those platforms have the *right* not to care.)
So I'm going to review this patch now *assuming* that the edk2 community
is OK to accept the use case in the first place. I'm not *encouraging*
the use case or whatever -- the use case is clearly wrong. But Eric
seems to need it: maybe some of the circumstances are out of his
control, and he can't just say "don't run this application", or "change
the application". Perhaps it's a temporary fix in edk2 (Eric mentioned
release time pressure, IIRC). I can fully appreciate that, and I'm OK to
tolerate temporary fixes -- it's just that temporary fixes sometimes
become permanent (increasing technical debt), and for that reason, I'd
prefer this hack to be as clean and non-intrusive as possible.
Thanks,
Laszlo
>> -----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 7:08 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 ` [edk2-devel] " Yao, Jiewen
2020-09-04 7:08 ` Laszlo Ersek [this message]
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=bc1cab75-8220-ab60-f173-f9faa16b6d2c@redhat.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