Re: [edk2-devel] [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers

["Ni, Ray" <ray.ni@intel.com>]

> +  MpInitLibWhoAmI (&Index);
>    SwitchStackData = (EXCEPTION_STACK_SWITCH_CONTEXT *)Buffer;
> -  InitializeSeparateExceptionStacks (SwitchStackData->Buffer,
> SwitchStackData->BufferSize);
> +  if ((SwitchStackData[Index].Status == EFI_NOT_STARTED) ||
> (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL)) {
> +    SwitchStackData[Index].Status = InitializeSeparateExceptionStacks
> (SwitchStackData[Index].Buffer, &SwitchStackData[Index].BufferSize);
> +  }
>  }

1. Can you add comment to explain why BufferTooSmall is checked?
I guess you want to handle the case when calls in some APs succeed
while some calls in other APs get BufferTooSmall.
So this check makes sure that the 2nd call happens only on those failed APs.

> +  SwitchStackData = AllocateRuntimeZeroPool (mNumberOfProcessors *
> sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
2. Why runtime type memory?

> +      if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
3. Can you add assertion to make sure " SwitchStackData[Index].BufferSize != 0"?


> +        SwitchStackData[Index].Buffer = (VOID *)(BufferSize + (UINTN)Buffer);
4. Use "&Buffer[BufferSize]";



> 
> -  SwitchStackData.BufferSize = &BufferSize;
>    MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> -  MpInitLibWhoAmI (&Bsp);
> +  SwitchStackData = AllocateRuntimeZeroPool (NumberOfProcessors *
> sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));

5. No need for runtime memory.

> +  for (Index = 0; Index < NumberOfProcessors; ++Index) {
> +    if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
> +      BufferSize += SwitchStackData[Index].BufferSize;

6. add assertion to make sure ".BufferSize == 0" when ".Status == EFI_SUCCESS".
I think assertion here is better than my last comment which requests assertion in
the other place.

7. Please use AllocatePages in PEI because in a many-core platform the pool 64KB
limitation may cause the code fail to allocate.

Thanks,
Ray