From: "Ni, Ray" <ray.ni@intel.com>
To: "Liu, Zhiguang" <zhiguang.liu@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Dong, Eric" <eric.dong@intel.com>,
"Kumar, Rahul R" <rahul.r.kumar@intel.com>
Subject: Re: [PATCH v4] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers
Date: Fri, 26 Aug 2022 08:51:17 +0000 [thread overview]
Message-ID: <MWHPR11MB1631AE987FF548F10AF871248C759@MWHPR11MB1631.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20220826073901.2634-1-zhiguang.liu@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Friday, August 26, 2022 3:39 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: [PATCH v4] UefiCpuPkg: Enhance logic in
> InitializeMpExceptionStackSwitchHandlers
>
> Parallelly run the function to SeparateExceptionStacks for all CPUs and
> allocate buffers together for better performance.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuMp.c | 104 +++++++++++++++++++------------
> UefiCpuPkg/CpuMpPei/CpuMpPei.c | 108 ++++++++++++++++++++---------
> ----
> 2 files changed, 132 insertions(+), 80 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> index f3ca813d2a..e7575d9b80 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -600,8 +600,9 @@ CollectBistDataFromHob (
> // Structure for InitializeSeparateExceptionStacks
> //
> typedef struct {
> - VOID *Buffer;
> - UINTN *BufferSize;
> + VOID *Buffer;
> + UINTN BufferSize;
> + EFI_STATUS Status;
> } EXCEPTION_STACK_SWITCH_CONTEXT;
>
> /**
> @@ -620,9 +621,18 @@ InitializeExceptionStackSwitchHandlers (
> )
> {
> EXCEPTION_STACK_SWITCH_CONTEXT *SwitchStackData;
> + UINTN Index;
>
> + MpInitLibWhoAmI (&Index);
> SwitchStackData = (EXCEPTION_STACK_SWITCH_CONTEXT *)Buffer;
> - InitializeSeparateExceptionStacks (SwitchStackData->Buffer,
> SwitchStackData->BufferSize);
> +
> + //
> + // This may be called twice for each Cpu. Only run
> InitializeSeparateExceptionStacks
> + // if this is the first call or the first call failed because of size too small.
> + //
> + if ((SwitchStackData[Index].Status == EFI_NOT_STARTED) ||
> (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL)) {
> + SwitchStackData[Index].Status = InitializeSeparateExceptionStacks
> (SwitchStackData[Index].Buffer, &SwitchStackData[Index].BufferSize);
> + }
> }
>
> /**
> @@ -638,53 +648,69 @@ InitializeMpExceptionStackSwitchHandlers (
> )
> {
> UINTN Index;
> - UINTN Bsp;
> - EXCEPTION_STACK_SWITCH_CONTEXT SwitchStackData;
> + EXCEPTION_STACK_SWITCH_CONTEXT *SwitchStackData;
> UINTN BufferSize;
> + EFI_STATUS Status;
> + UINT8 *Buffer;
>
> - SwitchStackData.BufferSize = &BufferSize;
> - MpInitLibWhoAmI (&Bsp);
> -
> + SwitchStackData = AllocateZeroPool (mNumberOfProcessors * sizeof
> (EXCEPTION_STACK_SWITCH_CONTEXT));
> + ASSERT (SwitchStackData != NULL);
> for (Index = 0; Index < mNumberOfProcessors; ++Index) {
> - SwitchStackData.Buffer = NULL;
> - BufferSize = 0;
> + //
> + // Because the procedure may runs multiple times, use the status
> EFI_NOT_STARTED
> + // to indicate the procedure haven't been run yet.
> + //
> + SwitchStackData[Index].Status = EFI_NOT_STARTED;
> + }
>
> - if (Index == Bsp) {
> - InitializeExceptionStackSwitchHandlers (&SwitchStackData);
> + Status = MpInitLibStartupAllCPUs (
> + InitializeExceptionStackSwitchHandlers,
> + 0,
> + SwitchStackData
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> + BufferSize = 0;
> + for (Index = 0; Index < mNumberOfProcessors; ++Index) {
> + if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
> + ASSERT (SwitchStackData[Index].BufferSize != 0);
> + BufferSize += SwitchStackData[Index].BufferSize;
> } else {
> - //
> - // AP might need different buffer size from BSP.
> - //
> - MpInitLibStartupThisAP (InitializeExceptionStackSwitchHandlers, Index,
> NULL, 0, (VOID *)&SwitchStackData, NULL);
> + ASSERT (SwitchStackData[Index].Status == EFI_SUCCESS);
> + ASSERT (SwitchStackData[Index].BufferSize == 0);
> }
> + }
>
> - if (BufferSize == 0) {
> - continue;
> + if (BufferSize != 0) {
> + Buffer = AllocateRuntimeZeroPool (BufferSize);
> + ASSERT (Buffer != NULL);
> + BufferSize = 0;
> + for (Index = 0; Index < mNumberOfProcessors; ++Index) {
> + if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
> + SwitchStackData[Index].Buffer = (VOID *)(&Buffer[BufferSize]);
> + BufferSize += SwitchStackData[Index].BufferSize;
> + DEBUG ((
> + DEBUG_INFO,
> + "Buffer[cpu%lu] for InitializeExceptionStackSwitchHandlers: 0x%lX with
> size 0x%lX\n",
> + (UINT64)(UINTN)Index,
> + (UINT64)(UINTN)SwitchStackData[Index].Buffer,
> + (UINT64)(UINTN)SwitchStackData[Index].BufferSize
> + ));
> + }
> }
>
> - SwitchStackData.Buffer = AllocateRuntimeZeroPool (BufferSize);
> - ASSERT (SwitchStackData.Buffer != NULL);
> - DEBUG ((
> - DEBUG_INFO,
> - "Buffer[cpu%lu] for InitializeExceptionStackSwitchHandlers: 0x%lX with
> size 0x%x\n",
> - (UINT64)(UINTN)Index,
> - (UINT64)(UINTN)SwitchStackData.Buffer,
> - (UINT32)BufferSize
> - ));
> -
> - if (Index == Bsp) {
> - InitializeExceptionStackSwitchHandlers (&SwitchStackData);
> - } else {
> - MpInitLibStartupThisAP (
> - InitializeExceptionStackSwitchHandlers,
> - Index,
> - NULL,
> - 0,
> - (VOID *)&SwitchStackData,
> - NULL
> - );
> + Status = MpInitLibStartupAllCPUs (
> + InitializeExceptionStackSwitchHandlers,
> + 0,
> + SwitchStackData
> + );
> + ASSERT_EFI_ERROR (Status);
> + for (Index = 0; Index < mNumberOfProcessors; ++Index) {
> + ASSERT (SwitchStackData[Index].Status == EFI_SUCCESS);
> }
> }
> +
> + FreePool (SwitchStackData);
> }
>
> /**
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> index c0be11d3ad..e7f1fe9f42 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> @@ -415,8 +415,9 @@ PeiWhoAmI (
> // Structure for InitializeSeparateExceptionStacks
> //
> typedef struct {
> - VOID *Buffer;
> - UINTN *BufferSize;
> + VOID *Buffer;
> + UINTN BufferSize;
> + EFI_STATUS Status;
> } EXCEPTION_STACK_SWITCH_CONTEXT;
>
> /**
> @@ -435,9 +436,18 @@ InitializeExceptionStackSwitchHandlers (
> )
> {
> EXCEPTION_STACK_SWITCH_CONTEXT *SwitchStackData;
> + UINTN Index;
>
> + MpInitLibWhoAmI (&Index);
> SwitchStackData = (EXCEPTION_STACK_SWITCH_CONTEXT *)Buffer;
> - InitializeSeparateExceptionStacks (SwitchStackData->Buffer,
> SwitchStackData->BufferSize);
> +
> + //
> + // This function may be called twice for each Cpu. Only run
> InitializeSeparateExceptionStacks
> + // if this is the first call or the first call failed because of size too small.
> + //
> + if ((SwitchStackData[Index].Status == EFI_NOT_STARTED) ||
> (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL)) {
> + SwitchStackData[Index].Status = InitializeSeparateExceptionStacks
> (SwitchStackData[Index].Buffer, &SwitchStackData[Index].BufferSize);
> + }
> }
>
> /**
> @@ -453,60 +463,76 @@ InitializeMpExceptionStackSwitchHandlers (
> )
> {
> UINTN Index;
> - UINTN Bsp;
> - EXCEPTION_STACK_SWITCH_CONTEXT SwitchStackData;
> - UINTN BufferSize;
> UINTN NumberOfProcessors;
> + EXCEPTION_STACK_SWITCH_CONTEXT *SwitchStackData;
> + UINTN BufferSize;
> + EFI_STATUS Status;
> + UINT8 *Buffer;
>
> if (!PcdGetBool (PcdCpuStackGuard)) {
> return;
> }
>
> - SwitchStackData.BufferSize = &BufferSize;
> MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> - MpInitLibWhoAmI (&Bsp);
> -
> + SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES
> (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)));
> + ASSERT (SwitchStackData != NULL);
> + ZeroMem (SwitchStackData, NumberOfProcessors * sizeof
> (EXCEPTION_STACK_SWITCH_CONTEXT));
> for (Index = 0; Index < NumberOfProcessors; ++Index) {
> - SwitchStackData.Buffer = NULL;
> - BufferSize = 0;
> + //
> + // Because the procedure may runs multiple times, use the status
> EFI_NOT_STARTED
> + // to indicate the procedure haven't been run yet.
> + //
> + SwitchStackData[Index].Status = EFI_NOT_STARTED;
> + }
> +
> + Status = MpInitLibStartupAllCPUs (
> + InitializeExceptionStackSwitchHandlers,
> + 0,
> + SwitchStackData
> + );
> + ASSERT_EFI_ERROR (Status);
>
> - if (Index == Bsp) {
> - InitializeExceptionStackSwitchHandlers (&SwitchStackData);
> + BufferSize = 0;
> + for (Index = 0; Index < NumberOfProcessors; ++Index) {
> + if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
> + ASSERT (SwitchStackData[Index].BufferSize != 0);
> + BufferSize += SwitchStackData[Index].BufferSize;
> } else {
> - //
> - // AP might need different buffer size from BSP.
> - //
> - MpInitLibStartupThisAP (InitializeExceptionStackSwitchHandlers, Index,
> NULL, 0, (VOID *)&SwitchStackData, NULL);
> + ASSERT (SwitchStackData[Index].Status == EFI_SUCCESS);
> + ASSERT (SwitchStackData[Index].BufferSize == 0);
> }
> + }
>
> - if (BufferSize == 0) {
> - continue;
> + if (BufferSize != 0) {
> + Buffer = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize));
> + ASSERT (Buffer != NULL);
> + BufferSize = 0;
> + for (Index = 0; Index < NumberOfProcessors; ++Index) {
> + if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
> + SwitchStackData[Index].Buffer = (VOID *)(&Buffer[BufferSize]);
> + BufferSize += SwitchStackData[Index].BufferSize;
> + DEBUG ((
> + DEBUG_INFO,
> + "Buffer[cpu%lu] for InitializeExceptionStackSwitchHandlers: 0x%lX with
> size 0x%lX\n",
> + (UINT64)(UINTN)Index,
> + (UINT64)(UINTN)SwitchStackData[Index].Buffer,
> + (UINT64)(UINTN)SwitchStackData[Index].BufferSize
> + ));
> + }
> }
>
> - SwitchStackData.Buffer = AllocatePages (EFI_SIZE_TO_PAGES
> (BufferSize));
> - ASSERT (SwitchStackData.Buffer != NULL);
> - ZeroMem (SwitchStackData.Buffer, EFI_PAGES_TO_SIZE
> (EFI_SIZE_TO_PAGES (BufferSize)));
> - DEBUG ((
> - DEBUG_INFO,
> - "Buffer[cpu%lu] for InitializeExceptionStackSwitchHandlers: 0x%lX with
> size 0x%x\n",
> - (UINT64)(UINTN)Index,
> - (UINT64)(UINTN)SwitchStackData.Buffer,
> - (UINT32)BufferSize
> - ));
> -
> - if (Index == Bsp) {
> - InitializeExceptionStackSwitchHandlers (&SwitchStackData);
> - } else {
> - MpInitLibStartupThisAP (
> - InitializeExceptionStackSwitchHandlers,
> - Index,
> - NULL,
> - 0,
> - (VOID *)&SwitchStackData,
> - NULL
> - );
> + Status = MpInitLibStartupAllCPUs (
> + InitializeExceptionStackSwitchHandlers,
> + 0,
> + SwitchStackData
> + );
> + ASSERT_EFI_ERROR (Status);
> + for (Index = 0; Index < NumberOfProcessors; ++Index) {
> + ASSERT (SwitchStackData[Index].Status == EFI_SUCCESS);
> }
> }
> +
> + FreePages (SwitchStackData, EFI_SIZE_TO_PAGES (NumberOfProcessors *
> sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)));
> }
>
> /**
> --
> 2.31.1.windows.1
prev parent reply other threads:[~2022-08-26 8:51 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-26 7:39 [PATCH v4] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers Zhiguang Liu
2022-08-26 8:51 ` Ni, Ray [this message]
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=MWHPR11MB1631AE987FF548F10AF871248C759@MWHPR11MB1631.namprd11.prod.outlook.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