* [pve-devel] [PATCH edk2-firmware] fix #6430: backport patch to fix split lock detection warnings
@ 2025-07-10 12:13 Friedrich Weber
2025-07-10 12:54 ` [pve-devel] superseded: " Friedrich Weber
0 siblings, 1 reply; 2+ messages in thread
From: Friedrich Weber @ 2025-07-10 12:13 UTC (permalink / raw)
To: pve-devel
On host CPUs with the split_lock_detect flag (newer Intel CPUs),
booting an OVMF VM with more than one core may trigger the host
kernel's split-lock detection, as reported in [1].
With default settings, kernels >= 5.19 slow down the corresponding
thread for 10ms when it detects a split lock, as documented in [2].
A warning is logged, e.g.:
> kernel: x86/split lock detection: #AC: CPU 0/KVM/5677 took a split_lock trap at address: 0x56339a9888c3
The kernel only prints this warning once per thread.
Booting an OVMF VM apparently only performs 0-2 split lock operations
per vCPU thread, so the slowdown is negligible, but another downside
is that, since the warning is only printed once per vCPU thread, that
these spurious messages mask any subsequent split locks by the same
vCPU thread, so users might not notice if split locks occur with their
actual VM workload, which would be more problematic.
Hence, backport an upstream patch that fixes the split lock detection
warnings.
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=6430
[2] https://pve.proxmox.com/wiki/Split_lock_detection
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
Notes:
Seems like a new upstream release is planned for August anyway [1],
but it might be nice to get this fixed for PVE 9 already.
Apparently needs to be applied with `git am --keep-cr`.
Only got it to build on bookworm (on trxie, the dependency
python3-distutils doesn't seem to exist?)
[1] https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning/cb019c651d162e37f6cc7fed9a32bf7ca4153336
...tLib-Fix-split-lock-violation-from-M.patch | 163 ++++++++++++++++++
debian/patches/series | 1 +
2 files changed, 164 insertions(+)
create mode 100644 debian/patches/UefiCpuPkg-MpInitLib-Fix-split-lock-violation-from-M.patch
diff --git a/debian/patches/UefiCpuPkg-MpInitLib-Fix-split-lock-violation-from-M.patch b/debian/patches/UefiCpuPkg-MpInitLib-Fix-split-lock-violation-from-M.patch
new file mode 100644
index 0000000..2583417
--- /dev/null
+++ b/debian/patches/UefiCpuPkg-MpInitLib-Fix-split-lock-violation-from-M.patch
@@ -0,0 +1,163 @@
+From 98b8e5055073989cfd838b4f2cea39b2718dc5bb Mon Sep 17 00:00:00 2001
+From: Aaron Young <Aaron.Young@oracle.com>
+Date: Thu, 15 May 2025 09:54:00 -0700
+Subject: [PATCH] UefiCpuPkg/MpInitLib: Fix split-lock violation from
+ MP_CPU_EXCHANGE_INFO
+
+A split-lock violation in OVMF was discovered due to the
+NumApsExecuting field of the MP_CPU_EXCHANGE_INFO
+struct (which is used atomically by the AP Reset Vector
+assembly code) crossing a cacheline boundary.
+
+Since the MP_CPU_EXCHANGE_INFO struct is unaligned and
+the NumApsExecuting field resides after other non-UINTN aligned
+fields in the struct (i.e. GdtrProfile/IdtrProfile), the
+NumApsExecuting field was allocated at a non-UINTN aligned
+address (crossing a cache-line) resulting in the split-lock
+violation.
+
+Therefore, align the MP_CPU_EXCHANGE_INFO struct (on a UINTN
+boundary) and move the NumApsExecuting field to before the
+GdtrProfile/IdtrProfile fields to ensure it is UINTN aligned and
+thus resides within a single cacheline avoiding the split-lock.
+Do the same for the ApIndex field as it is also used atomically
+and thus subject to a split-lock violation.
+
+Cc: Ray Ni <ray.ni@intel.com>
+Cc: Jiaxin Wu <jiaxin.wu@intel.com>
+Cc: Zhiguang Liu <zhiguang.liu@intel.com>
+Cc: Dun Tan <dun.tan@intel.com>
+Cc: Rahul Kumar <rahul1.kumar@intel.com>
+Cc: Gerd Hoffmann <kraxel@redhat.com>
+Cc: Star Zeng <star.zeng@intel.com>
+Signed-off-by: Aaron Young <aaron.young@oracle.com>
+(cherry picked from commit b0bc23d1f246dac977b639470a51bcef1bcd6e1d)
+Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
+---
+ UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 15 ++++++++++++---
+ UefiCpuPkg/Library/MpInitLib/MpLib.c | 15 ++++++++++-----
+ UefiCpuPkg/Library/MpInitLib/MpLib.h | 9 +++++++--
+ 3 files changed, 29 insertions(+), 10 deletions(-)
+
+diff --git a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
+index 317e627b58..ded603f8f8 100644
+--- a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
++++ b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
+@@ -74,18 +74,18 @@ struc MP_CPU_EXCHANGE_INFO
+ .StackStart: CTYPE_UINTN 1
+ .StackSize: CTYPE_UINTN 1
+ .CFunction: CTYPE_UINTN 1
++ .NumApsExecuting: CTYPE_UINTN 1
++ .ApIndex: CTYPE_UINTN 1
+ .GdtrProfile: CTYPE_UINT8 IA32_DESCRIPTOR_size
+ .IdtrProfile: CTYPE_UINT8 IA32_DESCRIPTOR_size
+ .BufferStart: CTYPE_UINTN 1
+ .ModeOffset: CTYPE_UINTN 1
+- .ApIndex: CTYPE_UINTN 1
+ .CodeSegment: CTYPE_UINTN 1
+ .DataSegment: CTYPE_UINTN 1
+ .EnableExecuteDisable: CTYPE_UINTN 1
+ .Cr3: CTYPE_UINTN 1
+ .InitFlag: CTYPE_UINTN 1
+ .CpuInfo: CTYPE_UINTN 1
+- .NumApsExecuting: CTYPE_UINTN 1
+ .CpuMpData: CTYPE_UINTN 1
+ .InitializeFloatingPointUnits: CTYPE_UINTN 1
+ .ModeTransitionMemory: CTYPE_UINT32 1
+@@ -99,5 +99,14 @@ struc MP_CPU_EXCHANGE_INFO
+ .ExtTopoAvail: CTYPE_BOOLEAN 1
+ endstruc
+
+-MP_CPU_EXCHANGE_INFO_OFFSET equ (Flat32Start - RendezvousFunnelProcStart)
++;
++; Declare a UINTN struct for the purpose of
++; of obtaining the size of a UINTN (UINTN_size).
++;
++struc UINTN
++ .Data CTYPE_UINTN 1
++endstruc
++
++; MP_CPU_EXCHANGE_INFO Offset (UINTN aligned)
++MP_CPU_EXCHANGE_INFO_OFFSET equ (((Flat32Start - RendezvousFunnelProcStart) + (UINTN_size - 1)) & ~(UINTN_size - 1))
+ %define MP_CPU_EXCHANGE_INFO_FIELD(Field) (MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO. %+ Field)
+diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
+index fdcc21d794..ffaff1855f 100644
+--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
++++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
+@@ -960,7 +960,7 @@ GetApResetVectorSize (
+ )
+ {
+ if (SizeBelow1Mb != NULL) {
+- *SizeBelow1Mb = AddressMap->ModeTransitionOffset + sizeof (MP_CPU_EXCHANGE_INFO);
++ *SizeBelow1Mb = ALIGN_VALUE (AddressMap->ModeTransitionOffset, sizeof (UINTN)) + sizeof (MP_CPU_EXCHANGE_INFO);
+ }
+
+ if (SizeAbove1Mb != NULL) {
+@@ -1092,7 +1092,7 @@ BackupAndPrepareWakeupBuffer (
+ CopyMem (
+ (VOID *)CpuMpData->WakeupBuffer,
+ (VOID *)CpuMpData->AddressMap.RendezvousFunnelAddress,
+- CpuMpData->BackupBufferSize - sizeof (MP_CPU_EXCHANGE_INFO)
++ CpuMpData->AddressMap.ModeTransitionOffset
+ );
+ }
+
+@@ -1126,17 +1126,22 @@ AllocateResetVectorBelow1Mb (
+ UINTN ApResetStackSize;
+
+ if (CpuMpData->WakeupBuffer == (UINTN)-1) {
+- CpuMpData->WakeupBuffer = GetWakeupBuffer (CpuMpData->BackupBufferSize);
++ CpuMpData->WakeupBuffer = GetWakeupBuffer (CpuMpData->BackupBufferSize);
++ //
++ // Align MpCpuExchangeInfo to avoid split-lock violations.
++ //
+ CpuMpData->MpCpuExchangeInfo = (MP_CPU_EXCHANGE_INFO *)(UINTN)
+- (CpuMpData->WakeupBuffer + CpuMpData->BackupBufferSize - sizeof (MP_CPU_EXCHANGE_INFO));
++ (CpuMpData->WakeupBuffer +
++ ALIGN_VALUE (CpuMpData->AddressMap.ModeTransitionOffset, sizeof (UINTN)));
+ DEBUG ((
+ DEBUG_INFO,
+ "AP Vector: 16-bit = %p/%x, ExchangeInfo = %p/%x\n",
+ CpuMpData->WakeupBuffer,
+- CpuMpData->BackupBufferSize - sizeof (MP_CPU_EXCHANGE_INFO),
++ CpuMpData->AddressMap.ModeTransitionOffset,
+ CpuMpData->MpCpuExchangeInfo,
+ sizeof (MP_CPU_EXCHANGE_INFO)
+ ));
++
+ //
+ // The AP reset stack is only used by SEV-ES guests. Do not allocate it
+ // if SEV-ES is not enabled. An SEV-SNP guest is also considered
+diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
+index 145538b6ee..fc08ae2ce6 100644
+--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
++++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
+@@ -213,18 +213,23 @@ typedef struct {
+ UINTN StackStart;
+ UINTN StackSize;
+ UINTN CFunction;
++ //
++ // NumApsExecuting and ApIndex are used atomically (in the
++ // assembly code). To avoid split-lock violations, keep them
++ // naturally aligned within a single cacheline.
++ //
++ UINTN NumApsExecuting;
++ UINTN ApIndex;
+ IA32_DESCRIPTOR GdtrProfile;
+ IA32_DESCRIPTOR IdtrProfile;
+ UINTN BufferStart;
+ UINTN ModeOffset;
+- UINTN ApIndex;
+ UINTN CodeSegment;
+ UINTN DataSegment;
+ UINTN EnableExecuteDisable;
+ UINTN Cr3;
+ UINTN InitFlag;
+ CPU_INFO_IN_HOB *CpuInfo;
+- UINTN NumApsExecuting;
+ CPU_MP_DATA *CpuMpData;
+ UINTN InitializeFloatingPointUnitsAddress;
+ UINT32 ModeTransitionMemory;
+--
+2.47.2
+
diff --git a/debian/patches/series b/debian/patches/series
index 45b3050..f9e3582 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -4,3 +4,4 @@ x64-baseline-abi.patch
Revert-ArmVirtPkg-make-EFI_LOADER_DATA-non-executabl.patch
ArmVirtPkg-disable-the-EFI_MEMORY_ATTRIBUTE-protocol.patch
Revert-UefiCpuPkg-Produce-EFI-memory-attributes-prot.patch
+UefiCpuPkg-MpInitLib-Fix-split-lock-violation-from-M.patch
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [pve-devel] superseded: [PATCH edk2-firmware] fix #6430: backport patch to fix split lock detection warnings
2025-07-10 12:13 [pve-devel] [PATCH edk2-firmware] fix #6430: backport patch to fix split lock detection warnings Friedrich Weber
@ 2025-07-10 12:54 ` Friedrich Weber
0 siblings, 0 replies; 2+ messages in thread
From: Friedrich Weber @ 2025-07-10 12:54 UTC (permalink / raw)
To: pve-devel
Superseded by:
https://lore.proxmox.com/pve-devel/20250710125029.149536-1-f.weber@proxmox.com/T/#u
Noticed a few annoying typos right after sending, which prompted me to
rephrase the commit message altogether. Sorry for the noise.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-07-10 12:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-07-10 12:13 [pve-devel] [PATCH edk2-firmware] fix #6430: backport patch to fix split lock detection warnings Friedrich Weber
2025-07-10 12:54 ` [pve-devel] superseded: " Friedrich Weber
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox