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

[Laszlo Ersek <lersek@redhat.com>]

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

> 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.

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