[edk2-devel] [Patch V3] UefiCpuPkg/CpuExceptionHandlerLib: Fix split lock
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2150 V3 changes: change to mov instruction (non locking instuction) instead of xchg to simplify design. V2 changes: Add xchg 16 bit instructions to handle sgdt and sidt base 63:48 bits and 47:32 bits. Add comment to explain why xchg 64bit isnt being used Split lock happens when a locking instruction is used on mis-aligned data that crosses two cachelines. If close source platform enables Alignment Check Exception(#AC), They can hit a double fault due to split lock being in CpuExceptionHandlerLib. sigt and sgdt saves 10 bytes to memory, 8 bytes is base and 2 bytes is limit. The data is mis-aligned, can cross two cacheline, and a xchg instruction(locking instuction) is being utilize. Signed-off-by: John E Lofgren --- UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm index 4db1a09f28..19198f2731 100644 --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm @@ -184,17 +184,19 @@ HasErrorCode: pushrax pushrax sidt[rsp] -xchgrax, [rsp + 2] -xchgrax, [rsp] -xchgrax, [rsp + 8] +mov bx, word [rsp] +mov rax, qword [rsp + 2] +mov qword [rsp], rax +mov word [rsp + 8], bx xor rax, rax pushrax pushrax sgdt[rsp] -xchgrax, [rsp + 2] -xchgrax, [rsp] -xchgrax, [rsp + 8] +mov bx, word [rsp] +mov rax, qword [rsp + 2] +mov qword [rsp], rax +mov word [rsp + 8], bx ;; UINT64 Ldtr, Tr; xor rax, rax -- 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47484): https://edk2.groups.io/g/devel/message/47484 Mute This Topic: https://groups.io/mt/34181976/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V3] UefiCpuPkg/CpuExceptionHandlerLib: Fix split lock
Sorry. I forgot amend it to the commit. Ill fix it. Sorry Again, John >-Original Message- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Wednesday, September 18, 2019 1:52 AM >To: devel@edk2.groups.io; Lofgren, John E >Subject: Re: [edk2-devel] [Patch V3] UefiCpuPkg/CpuExceptionHandlerLib: Fix >split lock > >On 09/18/19 00:49, John E Lofgren wrote: >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2150 >> V3 changes: >> change to mov instruction (non locking instuction) instead of xchg to >> simplify design. > >I think something's wrong -- the v3 update described above isn't actually >implemented in the patch (it continues using XCHG, rather than MOV). > >Thanks >Laszlo > >> >> V2 changes: >> Add xchg 16 bit instructions to handle sgdt and sidt base >> 63:48 bits and 47:32 bits. >> Add comment to explain why xchg 64bit isnt being used >> >> Split lock happens when a locking instruction is used on mis-aligned >> data that crosses two cachelines. If close source platform enables >> Alignment Check Exception(#AC), They can hit a double fault due to >> split lock being in CpuExceptionHandlerLib. >> >> sigt and sgdt saves 10 bytes to memory, 8 bytes is base and 2 bytes is limit. >> The data is mis-aligned, can cross two cacheline, and a xchg >> instruction(locking instuction) is being utilize. >> >> Signed-off-by: John E Lofgren >> --- >> >> >UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas >m >> | 20 ++-- >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git >> >a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na >> sm >> >b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na >> sm >> index 4db1a09f28..7b7642b290 100644 >> --- >> >a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na >> sm >> +++ >b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAs >> +++ m.nasm >> @@ -180,21 +180,29 @@ HasErrorCode: >> pushqword [rbp + 24] >> >> ;; UINT64 Gdtr[2], Idtr[2]; >> +; sidt and sgdt saves 10 bytes to memory, 8 bytes = base and 2 bytes = >limit. >> +; To avoid #AC split lock when separating base and limit into their >> +; own separate 64 bit memory, we can’t use 64 bit xchg since base >> [63:48] >bits >> +; may cross the cache line. >> xor rax, rax >> pushrax >> pushrax >> sidt[rsp] >> -xchgrax, [rsp + 2] >> -xchgrax, [rsp] >> -xchgrax, [rsp + 8] >> +xchgeax, [rsp + 2] >> +xchgeax, [rsp] >> +xchgeax, [rsp + 8] >> +xchg ax, [rsp + 6] >> +xchg ax, [rsp + 4] >> >> xor rax, rax >> pushrax >> pushrax >> sgdt[rsp] >> -xchgrax, [rsp + 2] >> -xchgrax, [rsp] >> -xchgrax, [rsp + 8] >> +xchgeax, [rsp + 2] >> +xchgeax, [rsp] >> +xchgeax, [rsp + 8] >> +xchg ax, [rsp + 6] >> +xchg ax, [rsp + 4] >> >> ;; UINT64 Ldtr, Tr; >> xor rax, rax >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47483): https://edk2.groups.io/g/devel/message/47483 Mute This Topic: https://groups.io/mt/34181976/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [Patch V3] UefiCpuPkg/CpuExceptionHandlerLib: Fix split lock
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2150 V3 changes: change to mov instruction (non locking instuction) instead of xchg to simplify design. V2 changes: Add xchg 16 bit instructions to handle sgdt and sidt base 63:48 bits and 47:32 bits. Add comment to explain why xchg 64bit isnt being used Split lock happens when a locking instruction is used on mis-aligned data that crosses two cachelines. If close source platform enables Alignment Check Exception(#AC), They can hit a double fault due to split lock being in CpuExceptionHandlerLib. sigt and sgdt saves 10 bytes to memory, 8 bytes is base and 2 bytes is limit. The data is mis-aligned, can cross two cacheline, and a xchg instruction(locking instuction) is being utilize. Signed-off-by: John E Lofgren --- UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm | 20 ++-- 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm index 4db1a09f28..7b7642b290 100644 --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm @@ -180,21 +180,29 @@ HasErrorCode: pushqword [rbp + 24] ;; UINT64 Gdtr[2], Idtr[2]; +; sidt and sgdt saves 10 bytes to memory, 8 bytes = base and 2 bytes = limit. +; To avoid #AC split lock when separating base and limit into their +; own separate 64 bit memory, we can’t use 64 bit xchg since base [63:48] bits +; may cross the cache line. xor rax, rax pushrax pushrax sidt[rsp] -xchgrax, [rsp + 2] -xchgrax, [rsp] -xchgrax, [rsp + 8] +xchgeax, [rsp + 2] +xchgeax, [rsp] +xchgeax, [rsp + 8] +xchg ax, [rsp + 6] +xchg ax, [rsp + 4] xor rax, rax pushrax pushrax sgdt[rsp] -xchgrax, [rsp + 2] -xchgrax, [rsp] -xchgrax, [rsp + 8] +xchgeax, [rsp + 2] +xchgeax, [rsp] +xchgeax, [rsp + 8] +xchg ax, [rsp + 6] +xchg ax, [rsp + 4] ;; UINT64 Ldtr, Tr; xor rax, rax -- 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47438): https://edk2.groups.io/g/devel/message/47438 Mute This Topic: https://groups.io/mt/34181976/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V2] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock
Hi Laszlo,
2. Yes, I can change commit message/comments to separate split lock and #AC.
3. Yes it’s a close platform that is enabling #AC which hits double fault
because split lock inside CpuExceptionHandlerLib.
Code:
I was wondering same thing, why are they using locking mechanism. I wasn’t sure
so that’s why keep xchg instead of changing to mov. I have yet to think of any
reasons. I think it's a good idea to use mov instead it accomplishes the same
thing and easier to understand.
Thank you,
John
>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Friday, September 13, 2019 9:32 AM
>To: devel@edk2.groups.io; Lofgren, John E
>Cc: Ni, Ray ; Gao, Liming ; Dong,
>Eric
>Subject: Re: [edk2-devel] [Patch V2] UefiCpuPkg/CpuExceptionHandlerLib: Fix
>#AC split lock
>
>On 09/09/19 20:40, John E Lofgren wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2150
>>
>> V2 changes:
>> Add xchg 16 bit instructions to handle sgdt and sidt base
>> 63:48 bits and 47:32 bits.
>> Add comment to explain why xchg 64bit isnt being used
>>
>> Fix #AC split lock's caused by seperating base and limit from sgdt and
>> sidt by changing xchg operands to 32-bit to stop from crossing
>> cacheline.
>>
>> Signed-off-by: John E Lofgren
>> ---
>>
>>
>UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas
>m
>> | 20 ++--
>> 1 file changed, 14 insertions(+), 6 deletions(-)
>
>The commit message (and the bug report) are very difficult to understand.
>
>This is the first time I hear about "split lock" and "alignment check
>exception",
>so please bear with me. This is my take on the problem.
>
>(1) "split lock" is explained well here:
>
>https://lwn.net/Articles/784864/
>
>In short, we can consider it a performance anti-pattern. A locking instruction
>(such as XCHG) is invoked on incorrectly aligned data, which causes
>performance degradation for the whole system. In some cases this can be a
>security issue even, because less privileged code can block (slow down) more
>privileged code running on a different CPU.
>
>(2) Alignment Check Exception is a way to detect the occurrence of "split
>lock". It must be configured explicitly, when the system software wishes to be
>informed explicitly about a split lock event.
>
>Therefore, the "#AC split lock" expression in the commit message is very
>confusing. Those are two different things. One is the problem ("split lock"),
>and the other is the (kind of) solution ("alignment check exception").
>
>We don't care about #AC (the exception) here. My understanding is that the
>open source edk2 tree does not enable #AC. The following include file:
>
> MdePkg/Include/Register/Intel/Msr/P6Msr.h
>
>defines MSR_P6_TEST_CTL (value 0x33), which the above LWN article
>references as MSR_TEST_CTL. The article refers to bit 29, but that seems to be
>part of the "Reserved1" bit-field in the MSR_P6_TEST_CTL_REGISTER.
>
>There seems to be a bit field that could be related (Disable_LOCK, bit#31), but
>again, there is no reference to Disable_LOCK in the edk2 codebase.
>
>So my whole point here is that we should clearly separate "#AC" from "split
>lock" in the commit message (and in the code comment). Those are separate
>things. And "split lock" may apply to edk2, but "#AC" does not (to the open
>source tree anyway).
>
>(3) OK, assuming this code indeed triggers a "split lock" event. Why is that a
>problem? Yes, it may degrade performance for the system. Why do we care?
>We are *already* handling an exception. That should not be a very frequent
>event, for any processor (BSP or AP) in the system.
>
>Is the problem that a closed source platform enables #AC, and then the fault
>handler in CpuExceptionHandlerLib -- which gets invoked due to an
>independent fault -- runs into a *double* fault, due to the split lock raising
>#AC?
>
>This should be clearly explained in the commit message. I'm not prying at
>proprietary platform details, but the circumstances / symptoms of the issue
>should be clearly described.
>
>More comments below, regarding the original code:
>
>>
>> diff --git
>>
>a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na
>> sm
>>
>b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na
>> sm
>> index 4db1a09f28..7b7642b290 100644
>> ---
>>
>a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na
>> sm
>> +++
>b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHan
[edk2-devel] [Patch V2] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2150 V2 changes: Add xchg 16 bit instructions to handle sgdt and sidt base 63:48 bits and 47:32 bits. Add comment to explain why xchg 64bit isnt being used Fix #AC split lock's caused by seperating base and limit from sgdt and sidt by changing xchg operands to 32-bit to stop from crossing cacheline. Signed-off-by: John E Lofgren --- UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm | 20 ++-- 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm index 4db1a09f28..7b7642b290 100644 --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm @@ -180,21 +180,29 @@ HasErrorCode: pushqword [rbp + 24] ;; UINT64 Gdtr[2], Idtr[2]; +; sidt and sgdt saves 10 bytes to memory, 8 bytes = base and 2 bytes = limit. +; To avoid #AC split lock when separating base and limit into their +; own separate 64 bit memory, we can’t use 64 bit xchg since base [63:48] bits +; may cross the cache line. xor rax, rax pushrax pushrax sidt[rsp] -xchgrax, [rsp + 2] -xchgrax, [rsp] -xchgrax, [rsp + 8] +xchgeax, [rsp + 2] +xchgeax, [rsp] +xchgeax, [rsp + 8] +xchg ax, [rsp + 6] +xchg ax, [rsp + 4] xor rax, rax pushrax pushrax sgdt[rsp] -xchgrax, [rsp + 2] -xchgrax, [rsp] -xchgrax, [rsp + 8] +xchgeax, [rsp + 2] +xchgeax, [rsp] +xchgeax, [rsp + 8] +xchg ax, [rsp + 6] +xchg ax, [rsp + 4] ;; UINT64 Ldtr, Tr; xor rax, rax -- 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47045): https://edk2.groups.io/g/devel/message/47045 Mute This Topic: https://groups.io/mt/34083544/21656 Mute #ac: https://groups.io/mk?hashtag=ac=3846945 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock
Hi Eric, 1. you are correct, we need to need to handle 63-32 bits. I attach image which I think will help explain it better. sidt and sgdt instructions, save 10 bytes (8 bytes = base, 2 bytes = limit) of data to memory. In the code, its separating the base and limit to their own 64 bit space. Since base is offset by 2 bytes and 64bit, it can cross a cache line causing #ac split lock. We can use two addition 16 bit xchg to fix it. We need to use 16bit version since the split of cache line is occurring between 63-48 and 47-32. 2. no, we don't need to worry about those two xchg because it works with aligned memory. Thank you, John >-Original Message- >From: Dong, Eric >Sent: Thursday, September 5, 2019 12:37 AM >To: devel@edk2.groups.io; Lofgren, John E >Cc: Ni, Ray ; Laszlo Ersek (ler...@redhat.com) >; Dong, Eric >Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix >#AC split lock > >Hi John, > >I'm not sure whether I understand the code correctly. If not, please correct >me. > >1. You change to the code to only exchange 32 bits(eax) instead of 64 >bits(rax). After your change, how to handle the above 32 bits value (from bit >32 to bit 63)? >2. In this file, also have another two xchg codes, do them need to be updated >also? > >Thanks, >Eric >> -Original Message- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >> John E Lofgren >> Sent: Wednesday, September 4, 2019 2:15 AM >> To: devel@edk2.groups.io >> Subject: [edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix >> #AC split lock >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2150 >> >> Fix #AC split lock's caused by seperating base and limit from sgdt and >> sidt by changing xchg operands to 32-bit to stop from crossing cacheline. >> >> Signed-off-by: John E Lofgren >> --- >> >> >UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas >m >> | 12 ++-- >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git >> >a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na >> s >> m >> >b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na >> s >> m >> index 4db1a09f28..6d83dca4b9 100644 >> --- >> >a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na >> s >> m >> +++ >b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm. >> +++ nasm >> @@ -184,17 +184,17 @@ HasErrorCode: >> pushrax >> pushrax >> sidt[rsp] >> -xchgrax, [rsp + 2] >> -xchgrax, [rsp] >> -xchgrax, [rsp + 8] >> +xchgeax, [rsp + 2] >> +xchgeax, [rsp] >> +xchgeax, [rsp + 8] >> >> xor rax, rax >> pushrax >> pushrax >> sgdt[rsp] >> -xchgrax, [rsp + 2] >> -xchgrax, [rsp] >> -xchgrax, [rsp + 8] >> +xchgeax, [rsp + 2] >> +xchgeax, [rsp] >> +xchgeax, [rsp + 8] >> >> ;; UINT64 Ldtr, Tr; >> xor rax, rax >> -- >> 2.16.2.windows.1 >> >> >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46984): https://edk2.groups.io/g/devel/message/46984 Mute This Topic: https://groups.io/mt/33129650/21656 Mute #ac: https://groups.io/mk?hashtag=ac=3846945 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2150 Fix #AC split lock's caused by seperating base and limit from sgdt and sidt by changing xchg operands to 32-bit to stop from crossing cacheline. Signed-off-by: John E Lofgren --- UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm index 4db1a09f28..6d83dca4b9 100644 --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm @@ -184,17 +184,17 @@ HasErrorCode: pushrax pushrax sidt[rsp] -xchgrax, [rsp + 2] -xchgrax, [rsp] -xchgrax, [rsp + 8] +xchgeax, [rsp + 2] +xchgeax, [rsp] +xchgeax, [rsp + 8] xor rax, rax pushrax pushrax sgdt[rsp] -xchgrax, [rsp + 2] -xchgrax, [rsp] -xchgrax, [rsp + 8] +xchgeax, [rsp + 2] +xchgeax, [rsp] +xchgeax, [rsp + 8] ;; UINT64 Ldtr, Tr; xor rax, rax -- 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46723): https://edk2.groups.io/g/devel/message/46723 Mute This Topic: https://groups.io/mt/33129650/21656 Mute #ac: https://groups.io/mk?hashtag=ac=3846945 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
