Re: [Patch V2] UefiCpuPkg/MpInitLib: Remove redundant parameter.

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

I finally succeed to setup the OVMF platform which can verify the boot failure issue.  But on my platform, if I use image build with below command (I assume it is used to enable SMM), the system can't boot to OS (host OS is fedora 25 and guest OS is Ubuntu 18.04). It hang at OS boot phase after ExitBootService point (I can see the console log which should been printed at ExitBootService point, so I think hang should after this point). 
	build -a IA32 -a X64 -p OvmfPkg/OvmfPkgIa32X64.dsc -t VS2015x86 -b NOOPT -D SMM_REQUIRE -D SECURE_BOOT_ENABLE -D TLS_ENABLE 

If I use below command to build the image, the system can boot to OS.  
	build -a IA32 -a X64 -p OvmfPkg\OvmfPkgIa32X64.dsc -t VS2015x86 -b NOOPT

Does my OVMF environment still has problem?


When do the above test, I don't include my two patches. Then I include my patches and build the image with SMM enabled, I found I can't reproduce the issue you met. I can find the "MpInitChangeApLoopCallback done!" message in the console log.  Attached the console log.

Can you help to verify the OVMF image build from my side?  Also can you attach the image from you side to let me try? 

Thanks,
Eric

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Friday, June 29, 2018 8:15 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch V2] UefiCpuPkg/MpInitLib: Remove redundant
> parameter.
> 
> Hi Eric,
> 
> On 06/29/18 05:20, Eric Dong wrote:
> > Parameter StartCount duplicates with RunningCount. After this change,
> > RunningCount means the running AP count.
> >
> > V2 changes: Remove volatile for RunningCount.
> >
> > Done Test:
> > 1.PI SCT Test
> > 2.Boot OS / S3
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Jeff Fan <vanjeff_919@hotmail.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +++++------
> > UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +--
> >  2 files changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 3945771764..52c9679099 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -1400,7 +1400,7 @@ CheckAllAPs (
> >      // value of state after setting the it to CpuStateFinished, so BSP can safely
> make use of its value.
> >      //
> >      if (GetApState(CpuData) != CpuStateBusy) {
> > -      CpuMpData->RunningCount ++;
> > +      CpuMpData->RunningCount --;
> >        CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> >
> >        //
> > @@ -1425,7 +1425,7 @@ CheckAllAPs (
> >    //
> >    // If all APs finish, return EFI_SUCCESS.
> >    //
> > -  if (CpuMpData->RunningCount == CpuMpData->StartCount) {
> > +  if (CpuMpData->RunningCount == 0) {
> >      return EFI_SUCCESS;
> >    }
> >
> > @@ -1442,7 +1442,7 @@ CheckAllAPs (
> >      //
> >      if (CpuMpData->FailedCpuList != NULL) {
> >        *CpuMpData->FailedCpuList =
> > -         AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount
> + 1) * sizeof (UINTN));
> > +         AllocatePool ((CpuMpData->RunningCount + 1) * sizeof
> > + (UINTN));
> >        ASSERT (*CpuMpData->FailedCpuList != NULL);
> >      }
> >      ListIndex = 0;
> > @@ -2121,7 +2121,7 @@ StartupAllAPsWorker (
> >      return EFI_NOT_STARTED;
> >    }
> >
> > -  CpuMpData->StartCount = 0;
> > +  CpuMpData->RunningCount = 0;
> >    for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount;
> ProcessorNumber++) {
> >      CpuData = &CpuMpData->CpuData[ProcessorNumber];
> >      CpuData->Waiting = FALSE;
> > @@ -2131,7 +2131,7 @@ StartupAllAPsWorker (
> >          // Mark this processor as responsible for current calling.
> >          //
> >          CpuData->Waiting = TRUE;
> > -        CpuMpData->StartCount++;
> > +        CpuMpData->RunningCount++;
> >        }
> >      }
> >    }
> > @@ -2140,7 +2140,6 @@ StartupAllAPsWorker (
> >    CpuMpData->ProcArguments = ProcedureArgument;
> >    CpuMpData->SingleThread  = SingleThread;
> >    CpuMpData->FinishedCount = 0;
> > -  CpuMpData->RunningCount  = 0;
> >    CpuMpData->FailedCpuList = FailedCpuList;
> >    CpuMpData->ExpectedTime  = CalculateTimeout (
> >                                 TimeoutInMicroseconds, diff --git
> > a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 90c09fb8fb..ad62acf766 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -210,9 +210,8 @@ struct _CPU_MP_DATA {
> >    UINTN                          BackupBuffer;
> >    UINTN                          BackupBufferSize;
> >
> > -  volatile UINT32                StartCount;
> >    volatile UINT32                FinishedCount;
> > -  volatile UINT32                RunningCount;
> > +  UINT32                         RunningCount;
> >    BOOLEAN                        SingleThread;
> >    EFI_AP_PROCEDURE               Procedure;
> >    VOID                           *ProcArguments;
> >
> 
> I got confused by the way you sent out this patch.
> 
> First I thought that you meant it separately (stand-alone). I tried to test it, but
> it didn't apply. Also my intent was to ask you whether you wanted to send a
> new version of
> 
>   [edk2] [Patch 1/2] UefiCpuPkg/MpInitLib: Not use disabled AP.
> 
> 
> However, upon seeing that this patch wouldn't apply, I'm now thinking that
> you would like to preserve
> 
>   [edk2] [Patch 1/2] UefiCpuPkg/MpInitLib: Not use disabled AP.
> 
> without any changes, and your intent is to only update
> 
>   [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Remove redundant parameter.
> 
> to version 2.
> 
> 
> In such cases, please do not post an independent patch. Instead, pick one of
> the following:
> 
> - Repost the entire series as v2, and mark in the cover letter that
>   patch #1 is unchanged from the v1 posting.
> 
> - Alternatively, post the patch in response to the *original* v1 thread
>   (using --in-reply-to= with git-send-email), and use the subject
> 
>   [edk2] [Patch v2 2/2] UefiCpuPkg/MpInitLib: Remove redundant
>                         parameter.
> 
> Either of these approaches will let reviewers know that you intend the two
> patches to go together (and in what order), and that only patch #2 has been
> updated.
> 
> 
> So... Assuming this was indeed your intent, I applied the following two patches
> locally, for testing:
> 
>   [edk2] [Patch 1_2] UefiCpuPkg_MpInitLib: Not use disabled AP.
>   Message-Id: <20180628112920.5296-1-eric.dong@intel.com>
>   http://mid.mail-archive.com/20180628112920.5296-1-eric.dong@intel.com
> 
>   [edk2] [Patch V2] UefiCpuPkg_MpInitLib: Remove redundant parameter.
>   Message-Id: <20180629032047.6340-1-eric.dong@intel.com>
>   http://mid.mail-archive.com/20180629032047.6340-1-eric.dong@intel.com
>   (i.e., this patch)
> 
> I tested the following three scenarios with QEMU/KVM, using normal boot
> and S3, and 8 virtual CPUs (1 socket, 4 cores, 2 threads -- same topology as
> my physical laptop CPU):
> 
> (1) i440fx machine type, X64 build, without SMM
> (2) q35 machine type, IA32 build, with SMM
> (3) q35 machine type, IA32X64 build, with SMM
> 
> The guest OS was Fedora in every case.
> 
> The series keeps test case (1) functional.
> 
> The series breaks test cases (2) and (3) however; normal boot for each
> (S3 is not possible to attempt). The symptom is that, for both cases (2) and (3),
> the ExitBootServices() handlers are invoked, but the following message is
> never logged:
> 
>     MpInitChangeApLoopCallback() done!
> 
> and the boot process gets stuck.
> 
> With more context, a before/after log file diff, for case (2):
> 
> >  VirtioScsiExitBoot: Context=0x7DF3F010
> >  SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
> > -MpInitChangeApLoopCallback() done!
> 
> With more context, a before/after log file diff, for case (3):
> 
> >  VirtioScsiExitBoot: Context=0x7DBD4398
> >  VirtioRngExitBoot: Context=0x7DC47318
> >  SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
> > -MpInitChangeApLoopCallback() done!
> 
> I guess the patch series breaks something in
> MpInitChangeApLoopCallback() when the SMM driver stack is included in the
> build.
> 
> ... After repeated testing, the boot succeeds *sometimes* (rarely). In most
> cases, the boot fails as described above.
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel