KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS

[PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS

Posted by Piotr Zarycki 1 month ago

Use KVM_SYNC_X86_VALID_FIELDS instead of open-coding the same three
flags, ensuring the test stays in sync with any future additions.

Signed-off-by: Piotr Zarycki <piotr.zarycki@gmail.com>
---
 tools/testing/selftests/kvm/x86/sync_regs_test.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86/sync_regs_test.c b/tools/testing/selftests/kvm/x86/sync_regs_test.c
index e0c52321f87c..8f9239f9d357 100644
--- a/tools/testing/selftests/kvm/x86/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86/sync_regs_test.c
@@ -79,7 +79,7 @@ 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 TEST_SYNC_FIELDS   KVM_SYNC_X86_VALID_FIELDS
 #define INVALID_SYNC_FIELD 0x80000000
 
 /*
@@ -255,7 +255,6 @@ KVM_ONE_VCPU_TEST(sync_regs_test, req_and_verify_all_valid, guest_code)
 	struct kvm_regs regs;
 
 	/* 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;
 	vcpu_run(vcpu);
 	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
-- 
2.54.0

Re: [PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS

Posted by Sean Christopherson 1 month ago

On Mon, May 11, 2026, Piotr Zarycki wrote:
> Use KVM_SYNC_X86_VALID_FIELDS instead of open-coding the same three
> flags, ensuring the test stays in sync with any future additions.
> 
> Signed-off-by: Piotr Zarycki <piotr.zarycki@gmail.com>
> ---
>  tools/testing/selftests/kvm/x86/sync_regs_test.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86/sync_regs_test.c b/tools/testing/selftests/kvm/x86/sync_regs_test.c
> index e0c52321f87c..8f9239f9d357 100644
> --- a/tools/testing/selftests/kvm/x86/sync_regs_test.c
> +++ b/tools/testing/selftests/kvm/x86/sync_regs_test.c
> @@ -79,7 +79,7 @@ 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 TEST_SYNC_FIELDS   KVM_SYNC_X86_VALID_FIELDS

Explicitly defining the set of fields to test is very deliberate.  (a) we want
to detect ABI breakage, e.g. if KVM removes a valid field.  (b) if a field is
added in the future, the test likely needs to be updated, i.e. would break if
KVM extends KVM_SYNC_X86_VALID_FIELDS.  (c) KVM_SYNC_X86_VALID_FIELDS probably
shouldn't be defined in a uapi header; for all intents and purposes it's a KVM-
internal details.

>  #define INVALID_SYNC_FIELD 0x80000000
>  
>  /*
> @@ -255,7 +255,6 @@ KVM_ONE_VCPU_TEST(sync_regs_test, req_and_verify_all_valid, guest_code)
>  	struct kvm_regs regs;
>  
>  	/* 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;
>  	vcpu_run(vcpu);
>  	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
> -- 
> 2.54.0
>

Re: [PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS

Posted by Piotr Zarycki 1 month ago

On Mon, May 11, 2026 at 07:56:50AM -0700, Sean Christopherson wrote:
> On Mon, May 11, 2026, Piotr Zarycki wrote:
> > Use KVM_SYNC_X86_VALID_FIELDS instead of open-coding the same three
> > flags, ensuring the test stays in sync with any future additions.
> > 
> > Signed-off-by: Piotr Zarycki <piotr.zarycki@gmail.com>
> > ---
> >  tools/testing/selftests/kvm/x86/sync_regs_test.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/x86/sync_regs_test.c b/tools/testing/selftests/kvm/x86/sync_regs_test.c
> > index e0c52321f87c..8f9239f9d357 100644
> > --- a/tools/testing/selftests/kvm/x86/sync_regs_test.c
> > +++ b/tools/testing/selftests/kvm/x86/sync_regs_test.c
> > @@ -79,7 +79,7 @@ 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 TEST_SYNC_FIELDS   KVM_SYNC_X86_VALID_FIELDS
> 
> Explicitly defining the set of fields to test is very deliberate.  (a) we want
> to detect ABI breakage, e.g. if KVM removes a valid field.  (b) if a field is
> added in the future, the test likely needs to be updated, i.e. would break if
> KVM extends KVM_SYNC_X86_VALID_FIELDS.  (c) KVM_SYNC_X86_VALID_FIELDS probably
> shouldn't be defined in a uapi header; for all intents and purposes it's a KVM-
> internal details.
> 
> >  #define INVALID_SYNC_FIELD 0x80000000
> >  
> >  /*
> > @@ -255,7 +255,6 @@ KVM_ONE_VCPU_TEST(sync_regs_test, req_and_verify_all_valid, guest_code)
> >  	struct kvm_regs regs;
> >  
> >  	/* 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;
> >  	vcpu_run(vcpu);
> >  	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
> > -- 
> > 2.54.0
> > 

Thanks for the explanation. What's the preferred way to implement the build-time check mentioned in the TODO?

Re: [PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS

Posted by Sean Christopherson 1 month ago

On Mon, May 11, 2026, Piotr Zarycki wrote:
> On Mon, May 11, 2026 at 07:56:50AM -0700, Sean Christopherson wrote:
> > On Mon, May 11, 2026, Piotr Zarycki wrote:
> > > Use KVM_SYNC_X86_VALID_FIELDS instead of open-coding the same three
> > > flags, ensuring the test stays in sync with any future additions.
> > > 
> > > Signed-off-by: Piotr Zarycki <piotr.zarycki@gmail.com>
> > > ---
> > >  tools/testing/selftests/kvm/x86/sync_regs_test.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/kvm/x86/sync_regs_test.c b/tools/testing/selftests/kvm/x86/sync_regs_test.c
> > > index e0c52321f87c..8f9239f9d357 100644
> > > --- a/tools/testing/selftests/kvm/x86/sync_regs_test.c
> > > +++ b/tools/testing/selftests/kvm/x86/sync_regs_test.c
> > > @@ -79,7 +79,7 @@ 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 TEST_SYNC_FIELDS   KVM_SYNC_X86_VALID_FIELDS
> > 
> > Explicitly defining the set of fields to test is very deliberate.  (a) we want
> > to detect ABI breakage, e.g. if KVM removes a valid field.  (b) if a field is
> > added in the future, the test likely needs to be updated, i.e. would break if
> > KVM extends KVM_SYNC_X86_VALID_FIELDS.  (c) KVM_SYNC_X86_VALID_FIELDS probably
> > shouldn't be defined in a uapi header; for all intents and purposes it's a KVM-
> > internal details.
> > 
> > >  #define INVALID_SYNC_FIELD 0x80000000
> > >  
> > >  /*
> > > @@ -255,7 +255,6 @@ KVM_ONE_VCPU_TEST(sync_regs_test, req_and_verify_all_valid, guest_code)
> > >  	struct kvm_regs regs;
> > >  
> > >  	/* 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;
> > >  	vcpu_run(vcpu);
> > >  	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
> > > -- 
> > > 2.54.0
> > > 
> 
> Thanks for the explanation. What's the preferred way to implement the
> build-time check mentioned in the TODO? 

Honestly?  Just delete it.  It should be quite obvious if someone updates the
test to add more fields, without actually adding coverage for the new field(s).

[PATCH] KVM: selftests: sync_regs_test: drop stale TODO comment

Posted by Piotr Zarycki 1 month ago

The TODO asked for a build-time check to guard against missing new sync
fields. Remove it, as code review is sufficient to catch such issues.

Signed-off-by: Piotr Zarycki <piotr.zarycki@gmail.com>
---
 tools/testing/selftests/kvm/x86/sync_regs_test.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/x86/sync_regs_test.c b/tools/testing/selftests/kvm/x86/sync_regs_test.c
index e0c52321f87c..5b0c2359bbb4 100644
--- a/tools/testing/selftests/kvm/x86/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86/sync_regs_test.c
@@ -255,7 +255,6 @@ KVM_ONE_VCPU_TEST(sync_regs_test, req_and_verify_all_valid, guest_code)
 	struct kvm_regs regs;
 
 	/* 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;
 	vcpu_run(vcpu);
 	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
-- 
2.54.0

Re: [PATCH] KVM: selftests: sync_regs_test: drop stale TODO comment

Posted by Sean Christopherson 3 weeks, 3 days ago

On Tue, 12 May 2026 18:13:17 +0200, Piotr Zarycki wrote:
> The TODO asked for a build-time check to guard against missing new sync
> fields. Remove it, as code review is sufficient to catch such issues.

Applied to kvm-x86 selftests, thanks!

[1/1] KVM: selftests: sync_regs_test: drop stale TODO comment
      https://github.com/kvm-x86/linux/commit/64f1fa859c1e

--
https://github.com/kvm-x86/linux/tree/next