[PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock

[PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock

[john.e.lofgren]

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

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock

[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
> 
> 
> 

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock

[John E Lofgren]

[-- 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 --]

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: Fix #AC split lock

[Dong, Eric]

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
> >>
> >>
> >>