Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Aug 2023 12:55:09 -0700
In-Reply-To: <20230712075910.22480-3-thuth@redhat.com>
Mime-Version: 1.0
References: <20230712075910.22480-1-thuth@redhat.com> <20230712075910.22480-3-thuth@redhat.com>
Message-ID: <ZMq0nYYDbOX1cOKN@google.com>
Subject: Re: [PATCH 2/4] KVM: selftests: x86: Use TAP interface in the
 sync_regs test
From:   Sean Christopherson <seanjc@google.com>
To:     Thomas Huth <thuth@redhat.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        linux-kselftest@vger.kernel.org,
        David Matlack <dmatlack@google.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Wed, Jul 12, 2023, Thomas Huth wrote:
> The sync_regs test currently does not have any output (unless one
> of the TEST_ASSERT statement fails), so it's hard to say for a user
> whether a certain new sub-test has been included in the binary or
> not. Let's make this a little bit more user-friendly and include
> some TAP output via the kselftest_harness.h interface.
> To be able to use the interface, we have to break up the huge main()
> function here in more fine grained parts - then we can use the
> TEST_F() macro to define the individual tests. Since these are run
> with a separate VM now, we have also to make sure to create the
> expected state at the beginning of each test, so some parts grow
> a little bit - which should be OK considering that the individual
> tests are more self-contained now.
> 
> Suggested-by: David Matlack <dmatlack@google.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  .../selftests/kvm/x86_64/sync_regs_test.c     | 113 +++++++++++++++---

FYI, there's an in-flight patch[*] to expand this test's coverage, and I plan on
grabbing that in some form before this one (sorry).  Let me know if there are
any tweaks that can be done to Michal's patch to make it easier to convert the
test to tap.

I'll also try to get Michal's patch into kvm-x86/next sooner than later so that
you can use that as the basic.

Oh, and no need to post "KVM: selftests: Rename the ASSERT_EQ macro" in the next
version, I'm planning on grabbing that one straightaway.

[*] https://lore.kernel.org/all/20230728001606.2275586-3-mhal@rbox.co

>  1 file changed, 98 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> index 2da89fdc2471a..e1359a4a07fea 100644
> --- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> @@ -16,6 +16,7 @@
>  #include <string.h>
>  #include <sys/ioctl.h>
>  
> +#include "kselftest_harness.h"
>  #include "test_util.h"
>  #include "kvm_util.h"
>  #include "processor.h"
> @@ -80,23 +81,24 @@ static void compare_vcpu_events(struct kvm_vcpu_events *left,
>  #define TEST_SYNC_FIELDS   (KVM_SYNC_X86_REGS|KVM_SYNC_X86_SREGS|KVM_SYNC_X86_EVENTS)
>  #define INVALID_SYNC_FIELD 0x80000000
>  
> -int main(int argc, char *argv[])
> -{
> -	struct kvm_vcpu *vcpu;
> +FIXTURE(sync_regs_test) {
>  	struct kvm_vm *vm;
> -	struct kvm_run *run;
> -	struct kvm_regs regs;
> -	struct kvm_sregs sregs;
> -	struct kvm_vcpu_events events;
> -	int rv, cap;
> +	struct kvm_vcpu *vcpu;
> +};
>  
> -	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
> -	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
> -	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
> +FIXTURE_SETUP(sync_regs_test) {
> +	self->vm = vm_create_with_one_vcpu(&self->vcpu, guest_code);
> +}
>  
> -	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +FIXTURE_TEARDOWN(sync_regs_test) {
> +	kvm_vm_free(self->vm);
> +}
>  
> -	run = vcpu->run;
> +TEST_F(sync_regs_test, read_invalid)
> +{
> +	struct kvm_vcpu *vcpu = self->vcpu;
> +	struct kvm_run *run = vcpu->run;
> +	int rv;
>  
>  	/* Request reading invalid register set from VCPU. */
>  	run->kvm_valid_regs = INVALID_SYNC_FIELD;
> @@ -112,6 +114,13 @@ int main(int argc, char *argv[])
>  		    "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
>  		    rv);
>  	run->kvm_valid_regs = 0;
> +}
> +
> +TEST_F(sync_regs_test, set_invalid)
> +{
> +	struct kvm_vcpu *vcpu = self->vcpu;
> +	struct kvm_run *run = vcpu->run;
> +	int rv;
>  
>  	/* Request setting invalid register set into VCPU. */
>  	run->kvm_dirty_regs = INVALID_SYNC_FIELD;
> @@ -127,11 +136,22 @@ int main(int argc, char *argv[])
>  		    "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
>  		    rv);
>  	run->kvm_dirty_regs = 0;
> +}
> +
> +TEST_F(sync_regs_test, req_and_verify_all_valid)
> +{
> +	struct kvm_vcpu *vcpu = self->vcpu;
> +	struct kvm_run *run = vcpu->run;
> +	struct kvm_vcpu_events events;
> +	struct kvm_sregs sregs;
> +	struct kvm_regs regs;
> +	int rv;
>  
>  	/* Request and verify all valid register sets. */
>  	/* TODO: BUILD TIME CHECK: TEST_ASSERT(KVM_SYNC_X86_NUM_FIELDS != 3); */
>  	run->kvm_valid_regs = TEST_SYNC_FIELDS;
>  	rv = _vcpu_run(vcpu);
> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);

Just use vcpu_run() instead of _vcpu_run().  And please post that as a separate
patch, I think/hope it will make the conversion-to-tap patch smaller.

>  	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>  
>  	vcpu_regs_get(vcpu, &regs);
> @@ -142,6 +162,22 @@ int main(int argc, char *argv[])
>  
>  	vcpu_events_get(vcpu, &events);
>  	compare_vcpu_events(&events, &run->s.regs.events);
> +}
> +
> +TEST_F(sync_regs_test, set_and_verify_various)
> +{
> +	struct kvm_vcpu *vcpu = self->vcpu;
> +	struct kvm_run *run = vcpu->run;
> +	struct kvm_vcpu_events events;
> +	struct kvm_sregs sregs;
> +	struct kvm_regs regs;
> +	int rv;
> +
> +	/* Run once to get register set */
> +	run->kvm_valid_regs = TEST_SYNC_FIELDS;
> +	rv = _vcpu_run(vcpu);
> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);

Same comment here.

> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>  
>  	/* Set and verify various register values. */
>  	run->s.regs.regs.rbx = 0xBAD1DEA;
> @@ -151,6 +187,7 @@ int main(int argc, char *argv[])
>  	run->kvm_valid_regs = TEST_SYNC_FIELDS;
>  	run->kvm_dirty_regs = KVM_SYNC_X86_REGS | KVM_SYNC_X86_SREGS;
>  	rv = _vcpu_run(vcpu);
> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);

And here.

>  	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>  	TEST_ASSERT(run->s.regs.regs.rbx == 0xBAD1DEA + 1,
>  		    "rbx sync regs value incorrect 0x%llx.",
> @@ -167,6 +204,13 @@ int main(int argc, char *argv[])
>  
>  	vcpu_events_get(vcpu, &events);
>  	compare_vcpu_events(&events, &run->s.regs.events);
> +}
> +
> +TEST_F(sync_regs_test, clear_kvm_dirty_regs_bits)
> +{
> +	struct kvm_vcpu *vcpu = self->vcpu;
> +	struct kvm_run *run = vcpu->run;
> +	int rv;
>  
>  	/* Clear kvm_dirty_regs bits, verify new s.regs values are
>  	 * overwritten with existing guest values.
> @@ -175,10 +219,25 @@ int main(int argc, char *argv[])
>  	run->kvm_dirty_regs = 0;
>  	run->s.regs.regs.rbx = 0xDEADBEEF;
>  	rv = _vcpu_run(vcpu);
> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);

Here too.

>  	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>  	TEST_ASSERT(run->s.regs.regs.rbx != 0xDEADBEEF,
>  		    "rbx sync regs value incorrect 0x%llx.",
>  		    run->s.regs.regs.rbx);
> +}
> +
> +TEST_F(sync_regs_test, clear_kvm_valid_and_dirty_regs)
> +{
> +	struct kvm_vcpu *vcpu = self->vcpu;
> +	struct kvm_run *run = vcpu->run;
> +	struct kvm_regs regs;
> +	int rv;
> +
> +	/* Run once to get register set */
> +	run->kvm_valid_regs = TEST_SYNC_FIELDS;
> +	rv = _vcpu_run(vcpu);
> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);

At least you're consistent :-)

> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>  
>  	/* Clear kvm_valid_regs bits and kvm_dirty_bits.
>  	 * Verify s.regs values are not overwritten with existing guest values
> @@ -187,9 +246,11 @@ int main(int argc, char *argv[])
>  	run->kvm_valid_regs = 0;
>  	run->kvm_dirty_regs = 0;
>  	run->s.regs.regs.rbx = 0xAAAA;
> +	vcpu_regs_get(vcpu, &regs);

Can you split this change to its own patch too?  I'm pretty sure that change
stands on its own, and slotting it in here made me do a double-take.

>  	regs.rbx = 0xBAC0;
>  	vcpu_regs_set(vcpu, &regs);
>  	rv = _vcpu_run(vcpu);
> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
>  	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>  	TEST_ASSERT(run->s.regs.regs.rbx == 0xAAAA,
>  		    "rbx sync regs value incorrect 0x%llx.",
> @@ -198,6 +259,20 @@ int main(int argc, char *argv[])
>  	TEST_ASSERT(regs.rbx == 0xBAC0 + 1,
>  		    "rbx guest value incorrect 0x%llx.",
>  		    regs.rbx);
> +}
> +
> +TEST_F(sync_regs_test, clear_kvm_valid_regs_bits)
> +{
> +	struct kvm_vcpu *vcpu = self->vcpu;
> +	struct kvm_run *run = vcpu->run;
> +	struct kvm_regs regs;
> +	int rv;
> +
> +	/* Run once to get register set */
> +	run->kvm_valid_regs = TEST_SYNC_FIELDS;
> +	rv = _vcpu_run(vcpu);
> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);

Once more, with feeling!

> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>  
>  	/* Clear kvm_valid_regs bits. Verify s.regs values are not overwritten
>  	 * with existing guest values but that guest values are overwritten
> @@ -207,6 +282,7 @@ int main(int argc, char *argv[])
>  	run->kvm_dirty_regs = TEST_SYNC_FIELDS;
>  	run->s.regs.regs.rbx = 0xBBBB;
>  	rv = _vcpu_run(vcpu);
> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);

Heh.

>  	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>  	TEST_ASSERT(run->s.regs.regs.rbx == 0xBBBB,
>  		    "rbx sync regs value incorrect 0x%llx.",
> @@ -215,8 +291,15 @@ int main(int argc, char *argv[])
>  	TEST_ASSERT(regs.rbx == 0xBBBB + 1,
>  		    "rbx guest value incorrect 0x%llx.",
>  		    regs.rbx);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int cap;
>  
> -	kvm_vm_free(vm);
> +	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
> +	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
> +	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
>  
> -	return 0;
> +	return test_harness_run(argc, argv);
>  }
> -- 
> 2.39.3
>