[pve-devel] [PATCH edk2-firmware v2] fix #6430: backport patch to fix split lock detection warnings

[pve-devel] [PATCH edk2-firmware v2] fix #6430: backport patch to fix split lock detection warnings

[Friedrich Weber]

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, a kernel >= 5.19 slows down the corresponding
thread for 10ms when it detects a split lock operation, 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. However, another
downside is that, since the warning is only printed once per vCPU
thread, these spurious warnings mask any subsequent split lock
operations performed by the same vCPU thread. As a result, users are
not warned when their actual VM workload triggers split lock
detection, where the slowdowns would potentially be more noticeable.

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 trixie, the dependency
    python3-distutils doesn't seem to exist?)
    
    changes since v2:
    - reworded commit message
    
    [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

[pve-devel] applied: [PATCH edk2-firmware v2] fix #6430: backport patch to fix split lock detection warnings

[Thomas Lamprecht]

On Thu, 10 Jul 2025 14:50:29 +0200, Friedrich Weber wrote:
> 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, a kernel >= 5.19 slows down the corresponding
> thread for 10ms when it detects a split lock operation, as documented
> in [2]. A warning is logged, e.g.:
> 
> [...]

Applied, thanks!

But I had to re-export the patch to get the \r\n line endings back, the reason
for that is that you did not explicitly passed `--transfer-encoding=base64` to
git send-email as the "auto" default value is seemingly not smart enough to go
for that encoding when it sees such line endings..

btw. you git note suggested to me that you tried this on PVE 9, but you
at least did not try to build there, as I had to fix a bit of stuff (mostly
python 3.13 related).

[1/1] fix #6430: backport patch to fix split lock detection warnings
      commit: 47055b2c7b56fecd84c6441c4f4c419738cc1df8


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] applied: [PATCH edk2-firmware v2] fix #6430: backport patch to fix split lock detection warnings

[Friedrich Weber]

On 10/07/2025 15:33, Thomas Lamprecht wrote:
> On Thu, 10 Jul 2025 14:50:29 +0200, Friedrich Weber wrote:
>> 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, a kernel >= 5.19 slows down the corresponding
>> thread for 10ms when it detects a split lock operation, as documented
>> in [2]. A warning is logged, e.g.:
>>
>> [...]
> 
> Applied, thanks!

Thanks for the merge!

> But I had to re-export the patch to get the \r\n line endings back, the reason
> for that is that you did not explicitly passed `--transfer-encoding=base64` to
> git send-email as the "auto" default value is seemingly not smart enough to go
> for that encoding when it sees such line endings..

Oops, sorry about that, will keep this in mind for next time with a CRLF
patch.

> btw. you git note suggested to me that you tried this on PVE 9, but you
> at least did not try to build there, as I had to fix a bit of stuff (mostly
> python 3.13 related).

I did mention I only got it to build on bookworm, but forgot to mention
I only tested on bookworm too. My workstation is still on bookworm and I
didn't have any luck getting split-lock detection to work in a nested
setup (even with CPU type 'host', the PVE VM doesn't see the
'split_lock_detect' CPU flag). Should have made this more clear though,
sorry for the confusion.

> 
> [1/1] fix #6430: backport patch to fix split lock detection warnings
>       commit: 47055b2c7b56fecd84c6441c4f4c419738cc1df8



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] applied: [PATCH edk2-firmware v2] fix #6430: backport patch to fix split lock detection warnings

[Fiona Ebner]

Am 10.07.25 um 15:33 schrieb Thomas Lamprecht:
> On Thu, 10 Jul 2025 14:50:29 +0200, Friedrich Weber wrote:
>> 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, a kernel >= 5.19 slows down the corresponding
>> thread for 10ms when it detects a split lock operation, as documented
>> in [2]. A warning is logged, e.g.:
>>
>> [...]
> 
> Applied, thanks!
> 
> But I had to re-export the patch to get the \r\n line endings back, the reason
> for that is that you did not explicitly passed `--transfer-encoding=base64` to
> git send-email as the "auto" default value is seemingly not smart enough to go
> for that encoding when it sees such line endings..
IIRC, it's mailman who eats the line endings, not git [0]. If you send a
mail with such line endings to yourself it works without
'--transfer-encoding=base64' too, but it doesn't work if you send it to
the list.

[0]:
https://lore.proxmox.com/pve-devel/c72e637b-2e92-2ff5-0bc5-0c99d55b2ec8@proxmox.com/


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel