Hi Michael, It does not look like it is required. If the MP init fails for any reason. Could be HW failure, but the BSP is still working because it is obviously executing code, then the system should continue to boot with the BSP.
I will defer to Ray on this topic. Mike > -----Original Message----- > From: Michael Kubacki <[email protected]> > Sent: Wednesday, November 23, 2022 6:14 PM > To: [email protected]; Kinney, Michael D <[email protected]> > Cc: Dong, Eric <[email protected]>; Erich McMillan > <[email protected]>; Kumar, Rahul R <[email protected]>; > Ni, Ray <[email protected]> > Subject: Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally > uninitialized variables > > The ASSERT() was added to aid debugging by bringing attention to the > point where the fallback assignment occurs in the error status check block. > > Are you suggesting the ASSERT() be removed because of a known case where > it might trigger but the case is expected to return an error? Or, that > is is not necessary in general? > > Thanks, > Michael > > On 11/23/2022 9:04 PM, Michael D Kinney wrote: > > Hi Michael, > > > > comments below. > > > > Mike > > > >> -----Original Message----- > >> From: [email protected] <[email protected]> On Behalf Of Michael > >> Kubacki > >> Sent: Wednesday, November 9, 2022 9:33 AM > >> To: [email protected] > >> Cc: Dong, Eric <[email protected]>; Erich McMillan > >> <[email protected]>; Kinney, Michael D > >> <[email protected]>; Michael Kubacki > >> <[email protected]>; Kumar, Rahul R <[email protected]>; > >> Ni, Ray <[email protected]> > >> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally > >> uninitialized variables > >> > >> From: Michael Kubacki <[email protected]> > >> > >> Fixes CodeQL alerts for CWE-457: > >> https://cwe.mitre.org/data/definitions/457.html > >> > >> Cc: Eric Dong <[email protected]> > >> Cc: Erich McMillan <[email protected]> > >> Cc: Michael D Kinney <[email protected]> > >> Cc: Michael Kubacki <[email protected]> > >> Cc: Rahul Kumar <[email protected]> > >> Cc: Ray Ni <[email protected]> > >> Co-authored-by: Erich McMillan <[email protected]> > >> Signed-off-by: Michael Kubacki <[email protected]> > >> --- > >> UefiCpuPkg/CpuMpPei/CpuBist.c | 8 +++++++- > >> UefiCpuPkg/CpuMpPei/CpuMpPei.c | 8 +++++++- > >> UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++- > >> 3 files changed, 22 insertions(+), 3 deletions(-) > >> > >> diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c b/UefiCpuPkg/CpuMpPei/CpuBist.c > >> index 7dc93cd784d4..122808139b87 100644 > >> --- a/UefiCpuPkg/CpuMpPei/CpuBist.c > >> +++ b/UefiCpuPkg/CpuMpPei/CpuBist.c > >> @@ -175,7 +175,13 @@ CollectBistDataFromPpi ( > >> EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2; > >> EFI_SEC_PLATFORM_INFORMATION_CPU *CpuInstanceInHob; > >> > >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, > >> &NumberOfEnabledProcessors); > >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, > >> &NumberOfEnabledProcessors); > >> + ASSERT_EFI_ERROR (Status); > > > > I think this ASSERT() should be removed. The added error status check > > looks correct. > > > >> + > >> + if (EFI_ERROR (Status)) { > >> + NumberOfProcessors = 1u; > >> + NumberOfEnabledProcessors = 1u; > >> + } > >> > >> BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) + > >> sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * > >> NumberOfProcessors; > >> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c > >> b/UefiCpuPkg/CpuMpPei/CpuMpPei.c > >> index e7f1fe9f426c..a84304273168 100644 > >> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c > >> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c > >> @@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers ( > >> return; > >> } > >> > >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); > >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); > >> + ASSERT_EFI_ERROR (Status); > > > > I think this ASSERT() should be removed. The added error status check > > looks correct. > > > >> + > >> + if (EFI_ERROR (Status)) { > >> + NumberOfProcessors = 1u; > >> + } > >> + > >> SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES (NumberOfProcessors > >> * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT))); > >> ASSERT (SwitchStackData != NULL); > >> ZeroMem (SwitchStackData, NumberOfProcessors * sizeof > >> (EXCEPTION_STACK_SWITCH_CONTEXT)); > >> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c > >> b/UefiCpuPkg/CpuMpPei/CpuPaging.c > >> index 135422225340..1322fcb77f28 100644 > >> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c > >> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c > >> @@ -538,6 +538,7 @@ SetupStackGuardPage ( > >> UINTN NumberOfProcessors; > >> UINTN Bsp; > >> UINTN Index; > >> + EFI_STATUS Status; > >> > >> // > >> // One extra page at the bottom of the stack is needed for Guard page. > >> @@ -547,7 +548,13 @@ SetupStackGuardPage ( > >> ASSERT (FALSE); > >> } > >> > >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); > >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); > >> + ASSERT_EFI_ERROR (Status); > > > > I think this ASSERT() should be removed. The added error status check > > looks correct. > > > >> + > >> + if (EFI_ERROR (Status)) { > >> + NumberOfProcessors = 1u; > >> + } > >> + > >> MpInitLibWhoAmI (&Bsp); > >> for (Index = 0; Index < NumberOfProcessors; ++Index) { > >> StackBase = 0; > >> -- > >> 2.28.0.windows.1 > >> > >> > >> > >> -=-=-=-=-=-= > >> Groups.io Links: You receive all messages sent to this group. > >> View/Reply Online (#96156): https://edk2.groups.io/g/devel/message/96156 > >> Mute This Topic: https://groups.io/mt/94918104/1643496 > >> Group Owner: [email protected] > >> Unsubscribe: https://edk2.groups.io/g/devel/unsub > >> [[email protected]] > >> -=-=-=-=-=-= > >> > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#96607): https://edk2.groups.io/g/devel/message/96607 Mute This Topic: https://groups.io/mt/94918104/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
