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

[Laszlo Ersek <lersek@redhat.com>]

On 11/24/16 14:48, Fan, Jeff wrote:
> Laszlo,
> 
> I added my feedback for your three questions.
> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Thursday, November 24, 2016 5:21 PM
> 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
> 
> 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?
> [Jeff] Monitor size is from CPUID. I observed it equal to the cache line size. But we needn't to set size of Monitor and only set base address of Monitor buffer when executing MONITOR instruction.
> 
> * 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.
> [Jeff] Yes. It should be in new stack.
> 
> * 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.
> 
> [Jeff] On normal case, we DO should check monitor size and make sure we write the correct monitor buffer to wakeup Aps. But for this case, we do not want to wake up Aps at all. So, we just have one mwait deadloop here. Once we found AP is wake up by any reason (SMI, Interrupt, or monitor buffer writing), we will go on executing mwait and place Aps into C-State again.
> As long as this mwait loop does not touch any unsafe code/stack, it should be OK. This could simply the implementation.

But, if we don't actually care where the monitor / mwait are looking
(and we only want to make sure that the base address is in accessible
address space), then what do we need the AP stacks for at all? As far as
I recall from last night when I was looking at the code, the HLT loop
doesn't seem to need a stack at all, and the MONITOR / MWAIT loops could
share a single address between all of the APs. No need to allocate an
array of stacks.

... I'm not trying to block this series, just to understand why the code
is designed like this.

Anyway, I think I have no more open questions, so if you wish, you could
post v2, and I'd try to test it soon.

Thanks!
Laszlo