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

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

Hi Laszlo,


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, July 25, 2018 6:14 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.
> 
> On 07/25/18 05:50, Dong, Eric wrote:
> > Hi Laszlo,
> >
> > I have root cause this issue, the AP hangs in the procedure when
> > PiSmmCpuDxeSmm driver start up trigged this issue.
> >
> > When PiSmmCpuDxeSmm driver start up, it will call StartAllAps to set
> > memory attribute.  In StartAllAps function, after call WakeUpAp to
> > start Aps, it calls CheckAllAps to wait all Aps finished the task. In
> > CheckAllAps function, it detect AP state to know whether the AP has
> > finished its task. In old code, it check whether the AP state is
> > CpuStateFinished to know whether AP has finished tasks. This state is
> > only set by AP when it truly finished task. In new logic,
> > CpuStateFinished been replace with CpuStateIdle. And CpuStateIdle is
> > also the begin state of the AP. AP will change state from CpuStateIdle
> > to CpuStateBusy when it start execute the procedure. And after it
> > finished the procedure, it will change state back to CpuStateIdle.
> >
> > So when the hang issue raised, AP state is not been changed to
> > CpuStateBusy when BSP calls CheckAllAps to check whether the AP has
> > finished its task. So the state for the AP still in CpuStateIdle, but
> > BSP think AP has finished its task. In this case, BSP think all the
> > Aps has finished their tasks and it continues boot.
> 
> Awesome analysis!
> 
> So, this looks like an "inverse" variant of the classic "ABA problem":
> 
> https://en.wikipedia.org/wiki/ABA_problem

Yes, it's an ABA problem. New patch keep the CpuStateReady to distinguish from the CpuStateIdle state.

> 
> > But some AP may wake up later and it failed to return from the
> > procedure.
> 
> Ah! So that explains another symptom I've since seen as well -- although
> *very* rarely. Namely, if an AP wakes up *after* PiSmmCpuDxeSmm moves
> on, thinking that all APs are finished, the AP can execute garbage in "no man's
> land" -- and that crashes the guest. Basically, QEMU/KVM pause the guest
> with "emulation failure", and QEMU dumps the VCPU register state to the
> standard error on the host side. In particular, the register state indicates that
> the crashed VCPU is *not* in SMM.

Where to get these QEMU dumps info?

I'm not clear who calls StartAllAPs function when I send this mail, I just based on the debug message found StartAllAps trigged right after PiSmmCpuDxeSmm driver. but I think this not blocks my patches for this issue. So I sent the patches and back to dig it more.

Now I found it's PiSmmIpl driver calls gDS->SetMemorySpaceAttributes in SmmIplDxeDispatchEventNotify function which finally calls the StartAllAps.
The Ap procedure is SetMtrrsFromBuffer function in CpuDxe driver.  In SetMtrrsFromBuffer function, it just call MtrrSetAllMtrrs to update the MTRR, and it use local variable to save the MTRR settings. 

When the hang issue raised, in the time the AP begin to run the procedure, the BSP has leave current function. So the saved MTRR setting in the local variable is not valid anymore. So the MtrrSetAllMtrrs function use an invalid Mtrr Setttings to set mtrrs and never exit the procedure. Do you think it's reasonable?

I found till the ExitBootService point, the AP still in busy state. I check the ApWakeupFunction function, found between the AP procedure and set AP state to Idle, no other possible code exist. So I think the AP should not return back from procedure. I try to add debug message to know where the AP is stopped at. But after I add debug message in MtrrSetAllMtrrs, the issue can't reproduce anymore. I tried about 30 times. Do you have other message to get to know where the AP is?

Thanks,
Eric

> 
> When I first encountered this symptom now, while playing some more with
> your patches, it reminded me of earlier problems with MpInitLib. And now
> your analysis makes perfect sense of this additional symptom!
> 
> > In this case, the AP state keeps at CpuStateBusy. So later in
> > ChangeApLoopCallback function, because this AP state still in
> > CpuStateBusy, this AP will not trig the procedure. But BSP wait all
> > APs to trig the procedure(BSP wait the Aps to reduce the
> > mNumberToFinish value in procedure to continue boot) to continue the
> > boot, so the hang occurred.
> 
> This completes the explanation.
> 
> > I think we should keep a middle state to let us know whether the AP
> > truly finished its task. I will send  another serial patch for this
> > issue. Please help to check the new patches.
> 
> Yes, I'll test them too.
> 
> Thanks!
> Laszlo