From: "Fan, Jeff" <jeff.fan@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
"edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>
Cc: "Tian, Feng" <feng.tian@intel.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [PATCH 2/3] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB
Date: Thu, 24 Nov 2016 13:48:26 +0000 [thread overview]
Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2E7262@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <e173587e-cdaf-906c-4f13-8f835eea2810@redhat.com>
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.
Thank you,
Laszlo
next prev parent reply other threads:[~2016-11-24 13:48 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-23 14:01 [PATCH 0/3] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB Jeff Fan
2016-11-23 14:01 ` [PATCH 1/3] UefiCpuPkg/DxeMpLib: Get safe AP loop handler from global variable Jeff Fan
2016-11-23 17:08 ` Laszlo Ersek
2016-11-23 14:01 ` [PATCH 2/3] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB Jeff Fan
2016-11-23 18:31 ` Laszlo Ersek
2016-11-24 2:23 ` Fan, Jeff
2016-11-24 9:20 ` Laszlo Ersek
2016-11-24 13:48 ` Fan, Jeff [this message]
2016-11-24 21:13 ` Laszlo Ersek
2016-11-23 14:01 ` [PATCH 3/3] UefiCpuPkg/DxeMpLib: Make sure APs in safe loop code Jeff Fan
2016-11-23 18:52 ` Laszlo Ersek
2016-11-24 2:37 ` Fan, Jeff
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=542CF652F8836A4AB8DBFAAD40ED192A4A2E7262@shsmsx102.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox