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


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


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



John E Lofgren
 

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



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