Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables

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

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 <michael.d.kinney@intel.com>
> Sent: Thursday, November 24, 2022 10:31 AM
> To: Michael Kubacki <mikuback@linux.microsoft.com>; devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
> 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 <mikuback@linux.microsoft.com>
> > Sent: Wednesday, November 23, 2022 6:14 PM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>;
> > Ni, Ray <ray.ni@intel.com>
> > 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: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> > >> Sent: Wednesday, November 9, 2022 9:33 AM
> > >> To: devel@edk2.groups.io
> > >> Cc: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D
> > >> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>;
> > >> Ni, Ray <ray.ni@intel.com>
> > >> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
> > >>
> > >> From: Michael Kubacki <michael.kubacki@microsoft.com>
> > >>
> > >> Fixes CodeQL alerts for CWE-457:
> > >> https://cwe.mitre.org/data/definitions/457.html
> > >>
> > >> Cc: Eric Dong <eric.dong@intel.com>
> > >> Cc: Erich McMillan <emcmillan@microsoft.com>
> > >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > >> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > >> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > >> Cc: Ray Ni <ray.ni@intel.com>
> > >> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> > >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> > >> ---
> > >>   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: devel+owner@edk2.groups.io
> > >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> > >> -=-=-=-=-=-=
> > >>
> > >
> > >
> > >
> > > 
> > >
> > >