Re: [PATCH V8 1/2] KVM: selftests: Add x86_64 guest udelay() utility

[Reinette Chatre]

Hi Sean,

On 6/11/24 6:15 PM, Sean Christopherson wrote:
> 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).

Thank you very much for explaining these details. Next version intends to address
all your feedback and I will send that shortly.

Reinette