* [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock @ 2019-09-03 18:15 john.e.lofgren 2019-09-05 7:36 ` [edk2-devel] " Dong, Eric 0 siblings, 1 reply; 4+ messages in thread From: john.e.lofgren @ 2019-09-03 18:15 UTC (permalink / raw) To: devel 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 <john.e.lofgren@intel.com> --- 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: push rax push rax sidt [rsp] - xchg rax, [rsp + 2] - xchg rax, [rsp] - xchg rax, [rsp + 8] + xchg eax, [rsp + 2] + xchg eax, [rsp] + xchg eax, [rsp + 8] xor rax, rax push rax push rax sgdt [rsp] - xchg rax, [rsp + 2] - xchg rax, [rsp] - xchg rax, [rsp + 8] + xchg eax, [rsp + 2] + xchg eax, [rsp] + xchg eax, [rsp + 8] ;; UINT64 Ldtr, Tr; xor rax, rax -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock 2019-09-03 18:15 [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock john.e.lofgren @ 2019-09-05 7:36 ` Dong, Eric 2019-09-06 19:00 ` John E Lofgren 0 siblings, 1 reply; 4+ messages in thread From: Dong, Eric @ 2019-09-05 7:36 UTC (permalink / raw) To: devel@edk2.groups.io, Lofgren, John E Cc: Ni, Ray, Laszlo Ersek (lersek@redhat.com), Dong, Eric 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 <john.e.lofgren@intel.com> > --- > UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm > | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas > m > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas > m > index 4db1a09f28..6d83dca4b9 100644 > --- > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas > m > +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm. > +++ nasm > @@ -184,17 +184,17 @@ HasErrorCode: > push rax > push rax > sidt [rsp] > - xchg rax, [rsp + 2] > - xchg rax, [rsp] > - xchg rax, [rsp + 8] > + xchg eax, [rsp + 2] > + xchg eax, [rsp] > + xchg eax, [rsp + 8] > > xor rax, rax > push rax > push rax > sgdt [rsp] > - xchg rax, [rsp + 2] > - xchg rax, [rsp] > - xchg rax, [rsp + 8] > + xchg eax, [rsp + 2] > + xchg eax, [rsp] > + xchg eax, [rsp + 8] > > ;; UINT64 Ldtr, Tr; > xor rax, rax > -- > 2.16.2.windows.1 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock 2019-09-05 7:36 ` [edk2-devel] " Dong, Eric @ 2019-09-06 19:00 ` John E Lofgren 2019-09-09 0:34 ` Dong, Eric 0 siblings, 1 reply; 4+ messages in thread From: John E Lofgren @ 2019-09-06 19:00 UTC (permalink / raw) To: Dong, Eric, devel@edk2.groups.io Cc: Ni, Ray, Laszlo Ersek (lersek@redhat.com) [-- Attachment #1: Type: text/plain, Size: 3255 bytes --] 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 <john.e.lofgren@intel.com> >Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek (lersek@redhat.com) ><lersek@redhat.com>; Dong, Eric <eric.dong@intel.com> >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 <john.e.lofgren@intel.com> >> --- >> >> >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: >> push rax >> push rax >> sidt [rsp] >> - xchg rax, [rsp + 2] >> - xchg rax, [rsp] >> - xchg rax, [rsp + 8] >> + xchg eax, [rsp + 2] >> + xchg eax, [rsp] >> + xchg eax, [rsp + 8] >> >> xor rax, rax >> push rax >> push rax >> sgdt [rsp] >> - xchg rax, [rsp + 2] >> - xchg rax, [rsp] >> - xchg rax, [rsp + 8] >> + xchg eax, [rsp + 2] >> + xchg eax, [rsp] >> + xchg eax, [rsp + 8] >> >> ;; UINT64 Ldtr, Tr; >> xor rax, rax >> -- >> 2.16.2.windows.1 >> >> >> [-- Attachment #2: ac_split_lock.png --] [-- Type: image/png, Size: 29368 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock 2019-09-06 19:00 ` John E Lofgren @ 2019-09-09 0:34 ` Dong, Eric 0 siblings, 0 replies; 4+ messages in thread From: Dong, Eric @ 2019-09-09 0:34 UTC (permalink / raw) To: Lofgren, John E, devel@edk2.groups.io Cc: Ni, Ray, Laszlo Ersek (lersek@redhat.com) Hi John, Thanks for your explanation. I agree with your analysis. I think you can submit your next version patch. Thanks, Eric > -----Original Message----- > From: Lofgren, John E > Sent: Saturday, September 7, 2019 3:01 AM > To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek (lersek@redhat.com) > <lersek@redhat.com> > Subject: 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 <john.e.lofgren@intel.com> > >Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek (lersek@redhat.com) > ><lersek@redhat.com>; Dong, Eric <eric.dong@intel.com> > >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 <john.e.lofgren@intel.com> > >> --- > >> > >> > >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: > >> push rax > >> push rax > >> sidt [rsp] > >> - xchg rax, [rsp + 2] > >> - xchg rax, [rsp] > >> - xchg rax, [rsp + 8] > >> + xchg eax, [rsp + 2] > >> + xchg eax, [rsp] > >> + xchg eax, [rsp + 8] > >> > >> xor rax, rax > >> push rax > >> push rax > >> sgdt [rsp] > >> - xchg rax, [rsp + 2] > >> - xchg rax, [rsp] > >> - xchg rax, [rsp + 8] > >> + xchg eax, [rsp + 2] > >> + xchg eax, [rsp] > >> + xchg eax, [rsp + 8] > >> > >> ;; UINT64 Ldtr, Tr; > >> xor rax, rax > >> -- > >> 2.16.2.windows.1 > >> > >> > >> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-09 0:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-09-03 18:15 [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock john.e.lofgren 2019-09-05 7:36 ` [edk2-devel] " Dong, Eric 2019-09-06 19:00 ` John E Lofgren 2019-09-09 0:34 ` Dong, Eric
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
![[-- Attachment #2: ac_split_lock.png --] [-- Type: image/png, Size: 29368 bytes --]](../../C3DB6446E7D9C1469976333C5EE6D4E2E06E9011@FMSMSX106.amr.corp.intel.com/2-ac_split_lock.png){kind=link}