Mike, IMO, the failure is not expected at all. When such failure appears, no guaranteed system can run further because the hw is in a unstable state and I think sw should just give up. But ASSERT() is a NOP in release image.
I feel that we may need some macro that can still work in release image. E.g.: VERIFY_EFI_SUCCESS (Status)? so if-check is not needed. But it's a different topic. Michael, can CodeQL be happy with only ASSERT()? I just feel the "if" check looks like the failure should be handled by sw. Thanks, Ray > -----Original Message----- > From: Kinney, Michael D <[email protected]> > Sent: Thursday, November 24, 2022 10:31 AM > To: Michael Kubacki <[email protected]>; [email protected]; > Kinney, Michael D > <[email protected]>; Ni, Ray <[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 > > 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 (#96610): https://edk2.groups.io/g/devel/message/96610 Mute This Topic: https://groups.io/mt/94918104/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
