Re: [PATCH 2/3] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB

[Laszlo Ersek <lersek@redhat.com>]

On 11/24/16 03:23, Fan, Jeff wrote:
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Thursday, November 24, 2016 2:31 AM
> To: Fan, Jeff; edk2-devel@ml01.01.org
> Cc: Tian, Feng; Kinney, Michael D
> Subject: Re: [PATCH 2/3] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB

> I have some more questions about the preexistent code:
> 
> (8) The MONITOR statement seems to set up an address *range* for
> monitoring with MWAIT. EAX provides the base address of the range,
> and we point it to our new stack. However, what determines the *size*
> of the address range? Obviously, it must fit in our new stack.

> [Jeff] Yes. Monitor has size requirement to avoid fault wakeup. But
> this case, fault wakeup has no any real impact. It has rare case to
> write the memory. Even though fault wakeup happens, safe mwait-loop
> could make sure AP enter into C-state again.

I'm sorry, I think my question was not clear enough. I'm actually
interested in three things here.

* The first question is just for my general education, because I'm not
familiar with the MONITOR configuration. The question is independent of
the code, it is just for me to learn about MONITOR.

So, how does the programmer configure the size of the monitored area?
The base address is taken from EAX. Where does the size come from?

* My second note is that whatever size we configure for the monitor, it
should fit into our new stack. I see that you agree (although spurious
wakeups don't matter), so this part is done then.

* And third, I thought of something else last night. Please consider the
Ia32 case:

> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 64e51d8..4e55760 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -217,14 +217,19 @@ RendezvousFunnelProcEnd:
>  global ASM_PFX(AsmRelocateApLoop)
>  ASM_PFX(AsmRelocateApLoop):
>  AsmRelocateApLoopStart:
> -    cmp        byte [esp + 4], 1
> +    push       ebp
> +    mov        ebp, esp
> +    mov        ecx, [ebp + 8]      ; MwaitSupport
> +    mov        ebx, [ebp + 12]     ; ApTargetCState
> +    mov        esp, [ebp + 20]     ; TopOfApStack

We set ESP precisely to TopOfApStack here.

> +    cmp        cl,  1              ; Check mwait-monitor support
>      jnz        HltLoop
>  MwaitLoop:
>      mov        eax, esp

Here we move the same value to EAX -- so EAX equals TopOfApStack.

>      xor        ecx, ecx
>      xor        edx, edx
>      monitor

And here we point the monitor to TopOfApStack exactly. Note that nothing
has been pushed.

> -    mov        eax, [esp + 8]    ; Mwait Cx, Target C-State per eax[7:4]
> +    mov        eax, ebx            ; Mwait Cx, Target C-State per eax[7:4]
>      shl        eax, 4
>      mwait
>      jmp        MwaitLoop

The stack grows down. Whenever we push something, the stack pointer is
decremented first, and then the value being pushed is written to the
location pointed-to by the new (decremented) stack pointer. (I verified
this order of operations in the Intel SDM).

"mReservedTopOfApStack" points to the byte *right past* the allocated
area. In turn, for ProcessorNumber = 0, "TopOfApStack" is set exactly to
"mReservedTopOfApStack".

This is valid if we push something, because then the pointer is
decremented first, and the value will be written into the allocated
area.

However, with MONITOR, the area to watch extends *upward* from EAX. This
means that for ProcessorNumber = 0, we will be monitoring an area that
is strictly outside of the allocated range, regardless of the size of
the monitored range. And, for ProcessorNumber (N+1), the monitored area
will actually be located in the stack slice of ProcessorNumber (N).

So, my point is, we should determine the size of the monitored area
exactly, and push that much empty space (== decrement ESP manually)
before invoking MONITOR.

... Hm, okay, here's what the Intel SDM tells us about MONITOR: "the
address range that the monitoring hardware checks for store operations
can be determined by using CPUID".

We should consider this address range in both the assembly code, and in
the original allocation (so replace AP_SAFE_STACK_SIZE with a dynamic
value, or at least make sure that AP_SAFT_STACK_SIZE is larger than any
imaginable monitor area size).

The same issue could apply to the X64 code, but that's more complex so I
didn't check it.

Thank you,
Laszlo