Received-SPF: pass (google.com: domain of linux-kernel+bounces-210647-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 11 Jun 2024 15:03:11 -0700
In-Reply-To: <a44d4534-3ba1-4bee-b06d-bb2a77fe3856@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>
Message-ID: <ZmjJnzBkOe58fFL6@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:
> > Heh, the docs are stale.  KVM hasn't returned an error since commit cc578287e322
> > ("KVM: Infrastructure for software and hardware based TSC rate scaling"), which
> > again predates selftests by many years (6+ in this case).  To make our lives
> > much simpler, I think we should assert that KVM_GET_TSC_KHZ succeeds, and maybe
> > throw in a GUEST_ASSERT(thz_khz) in udelay()?
> 
> I added the GUEST_ASSERT() but I find that it comes with a caveat (more below).
> 
> I plan an assert as below that would end up testing the same as what a
> GUEST_ASSERT(tsc_khz) would accomplish:
> 
> 	r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
> 	TEST_ASSERT(r > 0, "KVM_GET_TSC_KHZ did not provide a valid TSC freq.");
> 	tsc_khz = r;
> 
> 
> Caveat is: the additional GUEST_ASSERT() requires all tests that use udelay() in
> the guest to now subtly be required to implement a ucall (UCALL_ABORT) handler.
> I did a crude grep check to see and of the 69 x86_64 tests there are 47 that do
> indeed have a UCALL_ABORT handler. If any of the other use udelay() then the
> GUEST_ASSERT() will of course still trigger, but will be quite cryptic. For
> example, "Unhandled exception '0xe' at guest RIP '0x0'" vs. "tsc_khz".

Yeah, we really need to add a bit more infrastructure, there is way, way, waaaay
too much boilerplate needed just to run a guest and handle the basic ucalls.
Reporting guest asserts should Just Work for 99.9% of tests.

Anyways, is it any less cryptic if ucall_assert() forces a failure?  I forget if
the problem with an unhandled GUEST_ASSERT() is that the test re-enters the guest,
or if it's something else.

I don't think we need a perfect solution _now_, as tsc_khz really should never
be 0, just something to not make life completely miserable for future developers.

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);
 }