Re: [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

["Laszlo Ersek" <lersek@redhat.com>]

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