Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

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

Hi Eric,

(1) on the email address list of this patch set, there is a strange item:

00000000  16 64 65 76 65 6c 40 65  64 6b 32 2e 67 72 6f 75  |.devel@edk2.grou|
00000010  70 73 2e 69 6f                                    |ps.io|

The first character is U+0016, which seems to stand for SYNCHRONOUS IDLE <https://en.wikipedia.org/wiki/Synchronous_Idle>. I think it must have been a typo.

(The set was otherwise posted correctly to the list -- the correct list address is present, too.)

Pointing this out just for future postings.

On 04/24/20 10:47, Dong, Eric wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> 
> This patch fixes an assertion because AP can't find the CpuMpData.
> When AP is waken up through Init-Sipi-Sipi, AP's IDT should
> be restored to pre-allocated buffer so AP can get the CpuMpData
> through the IDT base address.
> Current code already has logic to handle this when CpuMpData->
> InitFlag is ApInitConfig but misses the logic
> when CpuMpData->InitFlag is ApInitReconfig.
> This patch fixes this gap.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> V3:
>   Remove invalid save volatile registers process. Refine restore
>   volatile registers process.
> 
> V2: 
>   Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check. 
> 
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 34 +++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 64a4c3546e..7fd757b428 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -686,18 +686,31 @@ ApWakeupFunction (
>          WAKEUP_AP_SIGNAL,
>          0
>          );
> -      if (CpuMpData->ApLoopMode == ApInHltLoop) {
> -        //
> -        // Restore AP's volatile registers saved
> -        //
> -        RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> -      } else {
> +
> +      if (CpuMpData->InitFlag == ApInitReconfig) {
>          //
> -        // The CPU driver might not flush TLB for APs on spot after updating
> -        // page attributes. AP in mwait loop mode needs to take care of it when
> -        // woken up.
> +        // ApInitReconfig happens when:
> +        // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
> +        // 2. AP is initialized in DXE phase. 

(2) git-am complains that the line above has a trailing space character. Please remove it before you push the set.

> +        // In either case, use the volatile registers value derived from BSP.
> +        // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a
> +        //   different IDT shared by all APs.
>          //
> -        CpuFlushTlb ();
> +        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> +      }  else {
> +        if (CpuMpData->ApLoopMode == ApInHltLoop) {
> +          //
> +          // Restore AP's volatile registers saved before AP is halted
> +          //
> +          RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> +        } else {
> +          //
> +          // The CPU driver might not flush TLB for APs on spot after updating
> +          // page attributes. AP in mwait loop mode needs to take care of it when
> +          // woken up.
> +          //
> +          CpuFlushTlb ();
> +        }
>        }
>  
>        if (GetApState (&CpuMpData->CpuData[ProcessorNumber]) == CpuStateReady) {
> @@ -1780,7 +1793,6 @@ MpInitLibInitialize (
>        InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock);
>        CpuMpData->CpuData[Index].CpuHealthy = (CpuInfoInHob[Index].Health == 0)? TRUE:FALSE;
>        CpuMpData->CpuData[Index].ApFunction = 0;
> -      CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));
>      }
>    }
>  
> 

For the full series:

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

(For the testing, I used OVMF, with/without SMM, and normal boot and S3. With SMM, I also tested CPU hotplug. OVMF doesn't support "microcode patching", so maybe it didn't make sense to perform these regression-tests. But, "these patches should be harmless regarding <whatever>", are famous last words... So I rather wanted to regression-test them, under my use case.)

Thanks,
Laszlo