On Tue, Jun 11, 2024, Reinette Chatre wrote: > > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > > index 42151e571953..1116bce5cdbf 100644 > > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > > @@ -98,6 +98,8 @@ void ucall_assert(uint64_t cmd, const char *exp, const char *file, > > ucall_arch_do_ucall((vm_vaddr_t)uc->hva); > > + ucall_arch_do_ucall(GUEST_UCALL_FAILED); > > + > > ucall_free(uc); > > } > > > > Thank you very much. > > With your suggestion an example unhandled GUEST_ASSERT() looks as below. > It does not guide on what (beyond vcpu_run()) triggered the assert but it > indeed provides a hint that adding ucall handling may be needed. > > [SNIP] > ==== Test Assertion Failure ==== > lib/ucall_common.c:154: addr != (void *)GUEST_UCALL_FAILED > pid=16002 tid=16002 errno=4 - Interrupted system call > 1 0x000000000040da91: get_ucall at ucall_common.c:154 > 2 0x0000000000410142: assert_on_unhandled_exception at processor.c:614 > 3 0x0000000000406590: _vcpu_run at kvm_util.c:1718 > 4 (inlined by) vcpu_run at kvm_util.c:1729 > 5 0x00000000004026cf: test_apic_bus_clock at apic_bus_clock_test.c:115 > 6 (inlined by) run_apic_bus_clock_test at apic_bus_clock_test.c:164 > 7 (inlined by) main at apic_bus_clock_test.c:201 > 8 0x00007fb1d8429d8f: ?? ??:0 > 9 0x00007fb1d8429e3f: ?? ??:0 > 10 0x00000000004027a4: _start at ??:? > Guest failed to allocate ucall struct /facepalm No, it won't work, e.g. relies on get_ucall() being invoked. I'm also being unnecessarily clever, and missing the obvious, simple solution. The only reason tests manually handle UCALL_ABORT is because back when it was added, there was no sprintf support in the guest, i.e. the guest could only spit out raw information, it couldn't format a human-readable error message. And so tests manually handled UCALL_ABORT with a custom message. When we added sprintf support, (almost) all tests moved formatting to the guest and converged on using REPORT_GUEST_ASSERT(), but we never completed the cleanup by moving REPORT_GUEST_ASSERT() to common code. Even more ridiculous is that assert_on_unhandled_exception() is still a thing. That code exists _literally_ to handle this scenario, where common guest library code needs to signal a failure. In short, the right way to resolve this is to have _vcpu_run() (or maybe even __vcpu_run()) handle UCALL_ABORT. The the bajillion REPORT_GUEST_ASSERT() calls can be removed, as can UCALL_UNHANDLED and assert_on_unhandled_exception() since they can and should use a normal GUEST_ASSERT() now that guest code can provide the formating, and library code will ensure the assert is reported. For this series, just ignore the GUEST_ASSERT() wonkiness. If someone develops a test that uses udelay(), doesn't handle ucalls, _and_ runs on funky hardware, then so be it, they can come yell at me :-) And I'll work on a series to handle UCALL_ABORT in _vcpu_run() (and poke around a bit more to see if there's other low hanging cleanup fruit).
