| Message ID | 20210818212940.1382549-1-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests: KVM: Gracefully handle missing vCPU features | expand |
On Wed, Aug 18, 2021 at 09:29:40PM +0000, Oliver Upton wrote: > An error of ENOENT for the KVM_ARM_VCPU_INIT ioctl indicates that one of > the requested feature flags is not supported by the kernel/hardware. > Detect the case when KVM doesn't support the requested features and skip > the test rather than failing it. > > Cc: Andrew Jones <drjones@redhat.com> > Signed-off-by: Oliver Upton <oupton@google.com> > --- > Applies to 5.14-rc6. Tested by running all selftests on an Ampere Mt. > Jade system. > > .../testing/selftests/kvm/lib/aarch64/processor.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c > index 632b74d6b3ca..b1064a0c5e62 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c > @@ -216,6 +216,7 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini > { > struct kvm_vcpu_init default_init = { .target = -1, }; > uint64_t sctlr_el1, tcr_el1; > + int ret; > > if (!init) > init = &default_init; > @@ -226,7 +227,19 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini > init->target = preferred.target; > } > > - vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, init); > + ret = _vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, init); > + > + /* > + * Missing kernel feature support should result in skipping the test, > + * not failing it. > + */ > + if (ret && errno == ENOENT) { > + print_skip("requested vCPU features not supported; skipping test."); ", skipping test" will already be appended by print_skip(). > + exit(KSFT_SKIP); > + } > + > + TEST_ASSERT(!ret, "KVM_ARM_VCPU_INIT failed, rc: %i errno: %i (%s)", > + ret, errno, strerror(errno)); > > /* > * Enable FP/ASIMD to avoid trapping when accessing Q0-Q15 > -- > 2.33.0.rc1.237.g0d66db33f3-goog > I think I'd rather try to keep exit(KSFT_SKIP)'s out of the lib code. It'd be better if the test gets to decide whether to skip or not. How about moving this check+skip into the test instead? Thanks, drew
On 18/08/21 23:29, Oliver Upton wrote: > An error of ENOENT for the KVM_ARM_VCPU_INIT ioctl indicates that one of > the requested feature flags is not supported by the kernel/hardware. > Detect the case when KVM doesn't support the requested features and skip > the test rather than failing it. > > Cc: Andrew Jones <drjones@redhat.com> > Signed-off-by: Oliver Upton <oupton@google.com> > --- > Applies to 5.14-rc6. Tested by running all selftests on an Ampere Mt. > Jade system. > > .../testing/selftests/kvm/lib/aarch64/processor.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c > index 632b74d6b3ca..b1064a0c5e62 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c > @@ -216,6 +216,7 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini > { > struct kvm_vcpu_init default_init = { .target = -1, }; > uint64_t sctlr_el1, tcr_el1; > + int ret; > > if (!init) > init = &default_init; > @@ -226,7 +227,19 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini > init->target = preferred.target; > } > > - vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, init); > + ret = _vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, init); > + > + /* > + * Missing kernel feature support should result in skipping the test, > + * not failing it. > + */ > + if (ret && errno == ENOENT) { > + print_skip("requested vCPU features not supported; skipping test."); > + exit(KSFT_SKIP); > + } > + > + TEST_ASSERT(!ret, "KVM_ARM_VCPU_INIT failed, rc: %i errno: %i (%s)", > + ret, errno, strerror(errno)); > > /* > * Enable FP/ASIMD to avoid trapping when accessing Q0-Q15 > Queued, thanks. Paolo
On Tue, Sep 21, 2021 at 08:00:02PM +0200, Paolo Bonzini wrote: > On 18/08/21 23:29, Oliver Upton wrote: > > An error of ENOENT for the KVM_ARM_VCPU_INIT ioctl indicates that one of > > the requested feature flags is not supported by the kernel/hardware. > > Detect the case when KVM doesn't support the requested features and skip > > the test rather than failing it. > > > > Cc: Andrew Jones <drjones@redhat.com> > > Signed-off-by: Oliver Upton <oupton@google.com> > > --- > > Applies to 5.14-rc6. Tested by running all selftests on an Ampere Mt. > > Jade system. > > > > .../testing/selftests/kvm/lib/aarch64/processor.c | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c > > index 632b74d6b3ca..b1064a0c5e62 100644 > > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c > > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c > > @@ -216,6 +216,7 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini > > { > > struct kvm_vcpu_init default_init = { .target = -1, }; > > uint64_t sctlr_el1, tcr_el1; > > + int ret; > > if (!init) > > init = &default_init; > > @@ -226,7 +227,19 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini > > init->target = preferred.target; > > } > > - vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, init); > > + ret = _vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, init); > > + > > + /* > > + * Missing kernel feature support should result in skipping the test, > > + * not failing it. > > + */ > > + if (ret && errno == ENOENT) { > > + print_skip("requested vCPU features not supported; skipping test."); > > + exit(KSFT_SKIP); > > + } > > + > > + TEST_ASSERT(!ret, "KVM_ARM_VCPU_INIT failed, rc: %i errno: %i (%s)", > > + ret, errno, strerror(errno)); > > /* > > * Enable FP/ASIMD to avoid trapping when accessing Q0-Q15 > > > > Queued, thanks. > I'd rather we don't queue this. It'd be better, IMO, for the unit test to probe for features and then skip the test, if that's what it wants to do, when they're not present. I'd rather not have test skipping decisions made in the library code, which may not be what the unit test developer expects. Anyway, the 'skipping test' substring would be printed twice with this patch. Thanks, drew
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c index 632b74d6b3ca..b1064a0c5e62 100644 --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c @@ -216,6 +216,7 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini { struct kvm_vcpu_init default_init = { .target = -1, }; uint64_t sctlr_el1, tcr_el1; + int ret; if (!init) init = &default_init; @@ -226,7 +227,19 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini init->target = preferred.target; } - vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, init); + ret = _vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, init); + + /* + * Missing kernel feature support should result in skipping the test, + * not failing it. + */ + if (ret && errno == ENOENT) { + print_skip("requested vCPU features not supported; skipping test."); + exit(KSFT_SKIP); + } + + TEST_ASSERT(!ret, "KVM_ARM_VCPU_INIT failed, rc: %i errno: %i (%s)", + ret, errno, strerror(errno)); /* * Enable FP/ASIMD to avoid trapping when accessing Q0-Q15
An error of ENOENT for the KVM_ARM_VCPU_INIT ioctl indicates that one of the requested feature flags is not supported by the kernel/hardware. Detect the case when KVM doesn't support the requested features and skip the test rather than failing it. Cc: Andrew Jones <drjones@redhat.com> Signed-off-by: Oliver Upton <oupton@google.com> --- Applies to 5.14-rc6. Tested by running all selftests on an Ampere Mt. Jade system. .../testing/selftests/kvm/lib/aarch64/processor.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)