[edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Ni, Ray]

Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.

We should not assume PEI runs at 32 bit mode.

________________________________
发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 Laszlo Ersek <lersek at redhat.com>
发送时间: Friday, September 4, 2020 3:55:47 AM
收件人: Dong, Eric <eric.dong at intel.com>; devel at edk2.groups.io <devel at edk2.groups.io>
抄送: Ni, Ray <ray.ni at intel.com>
主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/03/20 21:00, Laszlo Ersek wrote:

> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>
> Instead, the calls should be made in the DXE instance of the library
> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
> functions:
>
> - MpInitLibStartupAllAPs
> - MpInitLibStartupThisAP
> - MpInitLibSwitchBSP
> - MpInitLibEnableDisableAP
>
> Here's why:
>
> (a) The symptom does not affect the PEI phase -- by the time the UEFI
> application is executed, the PEI phase has ended; there's no need to
> modify the PEI instance of the library.
>
> (b) The currently proposed failure exits are too late. For example, in
> the SwitchBSPWorker() function, by the time we exit, we have called
> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
> DisableLvtInterrupts() -- and the error path does not restore the
> original environment.
>
> (c) According to the PI spec (v1.7), the StartupAllAPs(),
> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
> first action in the above-listed DxeMpLib functions.
>
> (Assuming the protocol members are called from an AP, and consequently
> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
> *caller's* fault, per spec!)

This means we can move the ValidCr3GdtIdtCheck() function to
"DxeMpLib.c", and it is not necessary to modify "MpLib.h".

Thanks
Laszlo





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65018): https://edk2.groups.io/g/devel/message/65018
Mute This Topic: https://groups.io/mt/76609049/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200904/1639222a/attachment.htm>