Re: [kvm-unit-tests PATCH v2 0/4] Better backtraces for leaf functions

Mathias Krause
2025-12-18T10:07:18Z

[-- Attachment #1.1: Type: text/plain, Size: 8430 bytes --]

Hi Sean,

On 18.12.25 02:44, Sean Christopherson wrote:
> On Fri, Nov 21, 2025, Mathias Krause wrote:
>> On 18.11.25 02:47, Mathias Krause wrote:
>>
>> Finally found it. It's register corruption within both host and guest.
>> [...]
>
> *sigh*
>
> I'm not going to type out the first dozen words that escaped my mouth when
> reading this. I happen to like my job :-)

Me too, that's why I had to switch tasks to gain back some sanity ;)

> [...]
>> I hacked up something to verify my theory and made 'regs' "per-cpu". It
>> needs quite some code churn and I'm not all that happy with it. IMHO,
>> 'regs' and lots of the other VMX management state should be part of some
>> vcpu struct or something. In fact, struct vmx_test already has a
>> 'guest_regs' but using it won't work, as we need offsetable absolute
>> memory references for the inline ASM in vmx_enter_guest() to work as it
>> cannot make use of register-based memory references at all. (My hack
>> uses a global 'scratch_regs' with mutual exclusion on its usage.)
>
> So, much to my chagrin, I started coding away before reading your entire mail
> (I got through the paragraph about 'regs' getting clobbered, and then came back
> to the rest later; that was a mistake).

A common mistake of mine: writing lengthy explanations that don't get
read. Maybe I should start included some tl;dr section for such cases ;)

> I had the exact same idea about making regs per-CPU, but without the quotes.

Actually, I wanted something quick to test, so I went the easy route and
used a global array. However, my plan for a proper fix was to do the
real per-cpu thing, as that would still fit the "offsetable absolute
memory reference" needs for inline asm, as GS/FS-prefixed addresses
would be just fine.

> Of course, because I didn't read your entire mail, converting only regs to be
> per-CPU didn't help. It actually made things worse (TIL the INIT test shares
> a VMCS between CPUs... WTF).

Hah! This rabbit hole has way to many branches!

>
> After making launch and in_guest per-CPU, and using RAX to communicate hypercalls,

Nice! The use of a global 'hypercall_field' for communicating the
intended hypercall was just puzzling to me. Using registers is such a
more natural way of doing it.

> I've got everything passing (module one unsolvable SIPI wart; see below). I need
> to write changelogs and squash a few fixups, but overall it ended up being a nice
> cleanuped (de-duplicating the VMX vs. SVM GPR handling drops a lot of code). I'm
> tempted to delete a blank lines just to get to net negative LoC :-D
>
> lib/x86/smp.h | 32 ++++++++++++++++++++++++++++++++
> lib/x86/virt.h | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> x86/Makefile.common | 14 ++++++++++++++
> x86/realmode.c | 3 +++
> x86/svm.c | 19 ++++++++-----------
> x86/svm.h | 61 +++++++++----------------------------------------------------
> x86/svm_tests.c | 5 +++--
> x86/vmx.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------
> x86/vmx.h | 72 +++---------------------------------------------------------------------
> x86/vmx_tests.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------
> 10 files changed, 247 insertions(+), 246 deletions(-)

Not too bad!

>
>> To see the register corruption, one could start the vmx_sipi_signal_test
>> test with -s -S, attach gdb to it and add a watch for regs.rax. Stepping
>> through the test will clearly show how 'regs' get overwritten wrongly.
>
> The test itself is also flawed. On top of this race that you spotted:
>
> @@ -9985,11 +9985,11 @@ static void vmx_sipi_signal_test(void)
> /* update CR3 on AP */
> on_cpu(1, update_cr3, (void *)read_cr3());
>
> + vmx_set_test_stage(0);
> +
> /* start AP */
> on_cpu_async(1, sipi_test_ap_thread, NULL);
>
> - vmx_set_test_stage(0);
> -
> /* BSP enter guest */
> enter_guest();
> }
>
> This snippet is also broken:
>
> vmx_set_test_stage(1);
>
> /* AP enter guest */
> enter_guest();
>
> because the BSP can think the AP has entered WFS before it has even attempted
> VMLAUNCH. It's "fine" so long as there are host CPUs available, but the test
> fails 100% for me if I run KUT in a VM with e.g. j<number of CPUs>, and on an
> Ivybridge server CPU, it fails pretty consistently. No idea why, maybe a slower
> nested VM-Enter path? E.g. the test passes on EMR even if I run with j<2x CPUs>.

Yeah, I noticed the failing synchronization between the vCPUs too. I
even tried to fix them by introducing a few more states (simply more
steps). However, then I ended up in deadlocks, as the AP re-enters
vmx_sipi_test_guest() multiple times (well, twice). It was likely
related to the register corruption, I only noticed later. So I believed
those magic sleeps (delay(SIPI_SIGNAL_TEST_DELAY)) are for the lack of
synchronization and should account for the time span of the AP setting a
new step value but not yet having switched to the guest.
It might be worth revisiting my attempts of explicitly adding new step
states, now with the register corrupting being handled...

>
> Unfortunately, I can't think of any way to fix that problem. To recognize the
> SIPI, the AP needs to do VM-Enter with a WFS activity state, and I'm struggling
> to think of a way to atomically write software-visible state at the time of VM-Enter.
> E.g. in theory, the test could peek at the LAUNCHED field in the VMCS, but that
> would require reverse engineering the VMCS layout for every CPU. If KVM emulated
> any VM-wide MSRs, maybe we could throw one in the MSR load list? But all the ones
> I can think of, e.g. Hyper-V's partition-wide MSRs, aren't guaranteed to be available.

Ahh, that explains the deadlock I observed. So adding more states won't
help. Yeah, it needs some trickery... or those magic sleeps :D

>
> Anywho, now that I know what to look for, I can ignore false failures easily enough.
> If someone cares enough to come up with a clever fix, then yay. Otherwise, I'll
> just deal with the intermittent failures (or maybe nuke and pave).

>
> As for this patch, I'll write changelogs and get a series posted with the backtrace
> and realmode patch at the end.

Thanks a lot for picking up the work! For me, KUT is far from my daily
work, so spending time on it cuts on the time budget what I'm actually
supposed to do. After having spent nearly a week in chasing that bug
made me realize, the work to fix it properly would be, at least for me,
yet another week and I didn't had the time for it back then.

Of course, if changes of mine cause regressions, I feel strongly
obligated to fix these -- even if it isn't my code's fault after all.
However, in this case, I just didn't found the time to do so yet. So
thanks again for working on this!

I'll be off for the season from tomorrow on, so may not provide instant
feedback for the series, in case you post it in the next couple of days.
But I'm looking forward reviewing it! :)

One more thing. I was able to land the gcc bug fix[1], so the realmode
patch could be reworded to mention that only "pre gcc-16" is broken but
the patch is still needed as gcc 16 isn't even released yet and far from
being the minimal supported version for KUT.

Thanks,
Mathias

[1] https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=114a19fae9bd

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]