* [PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS
@ 2026-05-11 14:34 Piotr Zarycki
2026-05-11 14:56 ` Sean Christopherson
0 siblings, 1 reply; 5+ messages in thread
From: Piotr Zarycki @ 2026-05-11 14:34 UTC (permalink / raw)
To: seanjc, pbonzini; +Cc: shuah, kvm, linux-kselftest, linux-kernel, Piotr Zarycki
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS
2026-05-11 14:34 [PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS Piotr Zarycki
@ 2026-05-11 14:56 ` Sean Christopherson
2026-05-11 15:09 ` Piotr Zarycki
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2026-05-11 14:56 UTC (permalink / raw)
To: Piotr Zarycki; +Cc: pbonzini, shuah, kvm, linux-kselftest, linux-kernel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS
2026-05-11 14:56 ` Sean Christopherson
@ 2026-05-11 15:09 ` Piotr Zarycki
2026-05-12 13:52 ` Sean Christopherson
0 siblings, 1 reply; 5+ messages in thread
From: Piotr Zarycki @ 2026-05-11 15:09 UTC (permalink / raw)
To: Sean Christopherson; +Cc: pbonzini, shuah, kvm, linux-kselftest, linux-kernel
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?
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS
2026-05-11 15:09 ` Piotr Zarycki
@ 2026-05-12 13:52 ` Sean Christopherson
2026-05-12 16:13 ` [PATCH] KVM: selftests: sync_regs_test: drop stale TODO comment Piotr Zarycki
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2026-05-12 13:52 UTC (permalink / raw)
To: Piotr Zarycki; +Cc: pbonzini, shuah, kvm, linux-kselftest, linux-kernel
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).
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] KVM: selftests: sync_regs_test: drop stale TODO comment
2026-05-12 13:52 ` Sean Christopherson
@ 2026-05-12 16:13 ` Piotr Zarycki
0 siblings, 0 replies; 5+ messages in thread
From: Piotr Zarycki @ 2026-05-12 16:13 UTC (permalink / raw)
To: seanjc; +Cc: kvm, linux-kernel, linux-kselftest, pbonzini, shuah,
Piotr Zarycki
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-05-12 16:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-11 14:34 [PATCH] KVM: selftests: sync_regs_test: use KVM_SYNC_X86_VALID_FIELDS Piotr Zarycki
2026-05-11 14:56 ` Sean Christopherson
2026-05-11 15:09 ` Piotr Zarycki
2026-05-12 13:52 ` Sean Christopherson
2026-05-12 16:13 ` [PATCH] KVM: selftests: sync_regs_test: drop stale TODO comment Piotr Zarycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox