Re: [PATCH V2 0/6] Enable SMM page level protection.

[Paolo Bonzini <pbonzini@redhat.com>]

On 09/11/2016 16:01, Yao, Jiewen wrote:
> 1)      CpuS3.c – EarlyInitializeCpu()
> 2)      CpuS3.c – SmmRelocateBases()
> 3)      CpuS3.c – InitializeCpu()
> 4)      S3Resume.c – SendSmiIpiAllExcludingSelf()
> 
> I believe we can guarantee 1/2/3 is good, because I found we check BSP
> check mNumberToFinish.
> 
> 4 is a risk, because there is no AP finish check. If the AP is in below
> 1M with CR3 in SMRAM, it will be a trouble.
> 
> Once the AP executes RSM and return to non-SMM, the CR3 is no longer
> valid and AP must be crashed immediately. WoW!
> 
> The fix, I believe, is same.
> 
> We should make 1) AP is in above 1M reserved memory,

Is this because of the NMI case?

> and 2) AP is in protected mode with paging disabled.

It is not clear to me what the (4) SIPI done is there for, and why it is
triggered in S3Resume.c rather than CpuS3.c.  And why does it take so
much for APs to complete it?

That said, by the time you close and lock SMRAM, you aren't even sure
that you have reached the cli;hlt loop in the rendezvous funnel.  In
practice you will be there, but there is still a theoretical race.

InterlockedDecrement (&mNumberToFinish) should be moved from
EarlyMPRendezvousProcedure/MPRendezvousProcedure to GoToSleep, and
GoToSleep should leave 64-bit mode before doing it.  This will fix the
S3 bug as well.  It's only needed for 64-bit mode, but it is doable for
the Ia32 version as well.

Perhaps EarlyMPRendezvousProcedure and MPRendezvousProcedure can return
&mNumberToFinish; what do you think?

Paolo