| Message ID | 20240517173926.965351-6-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID overhaul, fixes, and caching | expand |
On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > Add a sanity check in get_cpuid_entry() to provide a friendlier error than > a segfault when a test developer tries to use a vCPU CPUID helper on a > barebones vCPU. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > tools/testing/selftests/kvm/lib/x86_64/processor.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > index c664e446136b..f0f3434d767e 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > @@ -1141,6 +1141,8 @@ const struct kvm_cpuid_entry2 *get_cpuid_entry(const struct kvm_cpuid2 *cpuid, > { > int i; > > + TEST_ASSERT(cpuid, "Must do vcpu_init_cpuid() first (or equivalent)"); > + > for (i = 0; i < cpuid->nent; i++) { > if (cpuid->entries[i].function == function && > cpuid->entries[i].index == index) Hi, Maybe it is better to do this assert in __vcpu_get_cpuid_entry() because the assert might confuse the reader, since it just tests for NULL but when it fails, it complains that you need to call some function. There is also another call to get_cpuid_entry() in kvm_cpu_fms but this call can't have this issue. Besides this nitpick, looks good to me. Best regards, Maxim Levitsky
On Mon, 2024-07-08 at 19:33 +0000, Sean Christopherson wrote: > On Thu, Jul 04, 2024, Maxim Levitsky wrote: > > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > > > Add a sanity check in get_cpuid_entry() to provide a friendlier error than > > > a segfault when a test developer tries to use a vCPU CPUID helper on a > > > barebones vCPU. > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > tools/testing/selftests/kvm/lib/x86_64/processor.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > > index c664e446136b..f0f3434d767e 100644 > > > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > > > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > > @@ -1141,6 +1141,8 @@ const struct kvm_cpuid_entry2 *get_cpuid_entry(const struct kvm_cpuid2 *cpuid, > > > { > > > int i; > > > > > > + TEST_ASSERT(cpuid, "Must do vcpu_init_cpuid() first (or equivalent)"); > > > + > > > for (i = 0; i < cpuid->nent; i++) { > > > if (cpuid->entries[i].function == function && > > > cpuid->entries[i].index == index) > > > > Hi, > > > > Maybe it is better to do this assert in __vcpu_get_cpuid_entry() because the > > assert might confuse the reader, since it just tests for NULL but when it > > fails, it complains that you need to call some function. > > IIRC, I originally added the assert in __vcpu_get_cpuid_entry(), but I didn't > like leaving get_cpuid_entry() unprotected. What if I add an assert in both? > E.g. have __vcpu_get_cpuid_entry() assert with the (hopefully) hepful message, > and have get_cpuid_entry() do a simple TEST_ASSERT_NE()? > This looks like a great idea. Best regards, Maxim Levitsky
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index c664e446136b..f0f3434d767e 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -1141,6 +1141,8 @@ const struct kvm_cpuid_entry2 *get_cpuid_entry(const struct kvm_cpuid2 *cpuid, { int i; + TEST_ASSERT(cpuid, "Must do vcpu_init_cpuid() first (or equivalent)"); + for (i = 0; i < cpuid->nent; i++) { if (cpuid->entries[i].function == function && cpuid->entries[i].index == index)
Add a sanity check in get_cpuid_entry() to provide a friendlier error than a segfault when a test developer tries to use a vCPU CPUID helper on a barebones vCPU. Signed-off-by: Sean Christopherson <seanjc@google.com> --- tools/testing/selftests/kvm/lib/x86_64/processor.c | 2 ++ 1 file changed, 2 insertions(+)