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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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]
> >> -=-=-=-=-=-=
> >>
> >
> >
> >
> > 
> >
> >