[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/CpuExceptionHandler
[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&subid=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&subid=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&subid=3846945 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-