Received-SPF: pass (google.com: domain of linux-kernel+bounces-210745-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 11 Jun 2024 18:15:09 -0700
In-Reply-To: <88c65b89-5174-4076-82cd-7852c8c25b66@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <cover.1718043121.git.reinette.chatre@intel.com>
 <ad03cb58323158c1ea14f485f834c5dfb7bf9063.1718043121.git.reinette.chatre@intel.com>
 <ZmeYp8Sornz36ZkO@google.com> <a44d4534-3ba1-4bee-b06d-bb2a77fe3856@intel.com>
 <ZmjJnzBkOe58fFL6@google.com> <88c65b89-5174-4076-82cd-7852c8c25b66@intel.com>
Message-ID: <Zmj2nVhtVoGflaiG@google.com>
Subject: Re: [PATCH V8 1/2] KVM: selftests: Add x86_64 guest udelay() utility
From: Sean Christopherson <seanjc@google.com>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: isaku.yamahata@intel.com, pbonzini@redhat.com, erdemaktas@google.com, 
	vkuznets@redhat.com, vannapurve@google.com, jmattson@google.com, 
	mlevitsk@redhat.com, xiaoyao.li@intel.com, chao.gao@intel.com, 
	rick.p.edgecombe@intel.com, yuan.yao@intel.com, kvm@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"

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).