| Message ID | 20220126135319.1918802-1-broonie@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3 | expand |
On Wed, Jan 26, 2022 at 01:53:19PM +0000, Mark Brown wrote: > The arch_timer and vgic_irq kselftests assume that they can create a > vgic-v3, using the library function vgic_v3_setup() which aborts with a > test failure if it is not possible to do so. Since vgic-v3 can only be > instantiated on systems where the host has GICv3 this leads to false > positives on older systems where that is not the case. > > Fix this by changing vgic_v3_setup() to return an error if the vgic can't > be instantiated and have the callers skip if this happens. We could also > exit flagging a skip in vgic_v3_setup() but this would prevent future test > cases conditionally deciding which GIC to use or generally doing more > complex output. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > > v2: > - The test for being able to create the GIC doesn't actually > instantiate it, add a call doing so in that case. > > tools/testing/selftests/kvm/aarch64/arch_timer.c | 7 ++++++- > tools/testing/selftests/kvm/aarch64/vgic_irq.c | 4 ++++ > tools/testing/selftests/kvm/lib/aarch64/vgic.c | 5 ++++- > 3 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c > index 9ad38bd360a4..791d38404652 100644 > --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c > +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c > @@ -366,6 +366,7 @@ static struct kvm_vm *test_vm_create(void) > { > struct kvm_vm *vm; > unsigned int i; > + int ret; > int nr_vcpus = test_args.nr_vcpus; > > vm = vm_create_default_with_vcpus(nr_vcpus, 0, 0, guest_code, NULL); > @@ -382,7 +383,11 @@ static struct kvm_vm *test_vm_create(void) > > ucall_init(vm, NULL); > test_init_timer_irq(vm); > - vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA); > + ret = vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA); > + if (ret < 0) { > + pr_info("Failed to create vgic-v3, skipping\n"); Please use 'print_skip', which appends ", skipping test" to keep the skip messages consistent. Also, print_skip can't be disabled with -DQUIET like pr_info. > + exit(KSFT_SKIP); > + } > > /* Make all the test's cmdline args visible to the guest */ > sync_global_to_guest(vm, test_args); > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c > index e6c7d7f8fbd1..b127a261fd29 100644 > --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c > +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c > @@ -761,6 +761,10 @@ static void test_vgic(uint32_t nr_irqs, bool level_sensitive, bool eoi_split) > > gic_fd = vgic_v3_setup(vm, 1, nr_irqs, > GICD_BASE_GPA, GICR_BASE_GPA); > + if (gic_fd < 0) { > + pr_info("Failed to create vgic-v3, skipping\n"); print_skip > + exit(KSFT_SKIP); > + } > > vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, > guest_irq_handlers[args.eoi_split][args.level_sensitive]); > diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c > index b3a0fca0d780..4ea65e119bdd 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c > +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c > @@ -51,7 +51,10 @@ int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus, uint32_t nr_irqs, > "Number of vCPUs requested (%u) doesn't match with the ones created for the VM (%u)\n", > nr_vcpus, nr_vcpus_created); > > - /* Distributor setup */ > + /* Distributor setup - test if it's possible then actually do it */ > + gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, true); > + if (gic_fd != 0) > + return -1; > gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false); kvm selftests generally asserts on failure with the nonunderscore prefixed KVM ioctl wrapper functions, which is why you appear to be forced to do this nasty dance. However, kvm selftests usually always also offers an underscore prefixed version of the KVM ioctl wrapper function too for cases like these. So we can just do if (_kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false, &gic_fd) != 0) return -1; Thanks, drew > > kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, > -- > 2.30.2 > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm >
On Wed, Jan 26, 2022 at 02:29:14PM +0000, Mark Brown wrote: > On Wed, Jan 26, 2022 at 03:17:41PM +0100, Andrew Jones wrote: > > On Wed, Jan 26, 2022 at 01:53:19PM +0000, Mark Brown wrote: > > > > - vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA); > > > + ret = vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA); > > > + if (ret < 0) { > > > + pr_info("Failed to create vgic-v3, skipping\n"); > > > Please use 'print_skip', which appends ", skipping test" to keep the skip > > messages consistent. Also, print_skip can't be disabled with -DQUIET like > > pr_info. > > I see. It might be nice to convert these tests to use the ksft_ > stuff... Indeed. I'll add that to my TODO. > > > > - /* Distributor setup */ > > > + /* Distributor setup - test if it's possible then actually do it */ > > > + gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, true); > > > + if (gic_fd != 0) > > > + return -1; > > > gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false); > > > kvm selftests generally asserts on failure with the nonunderscore > > prefixed KVM ioctl wrapper functions, which is why you appear to > > be forced to do this nasty dance. However, kvm selftests usually > > always also offers an underscore prefixed version of the KVM ioctl > > wrapper function too for cases like these. So we can just do > > > if (_kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false, &gic_fd) != 0) > > return -1; > > And the _ version is OK to use in the vgic code? The _ makes it look > like it's internal only. It's extra OK. Anything in lib/* or lib/*/* is internal to the lib. However, it's even OK for a unit test to use the _ prefixed functions. The _ isn't so much about being internal as it is about being a raw version of the ioctl wrapper, which returns error codes, vs. the asserting version of the wrapper which only returns results on success. Thanks, drew
diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c index 9ad38bd360a4..791d38404652 100644 --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c @@ -366,6 +366,7 @@ static struct kvm_vm *test_vm_create(void) { struct kvm_vm *vm; unsigned int i; + int ret; int nr_vcpus = test_args.nr_vcpus; vm = vm_create_default_with_vcpus(nr_vcpus, 0, 0, guest_code, NULL); @@ -382,7 +383,11 @@ static struct kvm_vm *test_vm_create(void) ucall_init(vm, NULL); test_init_timer_irq(vm); - vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA); + ret = vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA); + if (ret < 0) { + pr_info("Failed to create vgic-v3, skipping\n"); + exit(KSFT_SKIP); + } /* Make all the test's cmdline args visible to the guest */ sync_global_to_guest(vm, test_args); diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c index e6c7d7f8fbd1..b127a261fd29 100644 --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c @@ -761,6 +761,10 @@ static void test_vgic(uint32_t nr_irqs, bool level_sensitive, bool eoi_split) gic_fd = vgic_v3_setup(vm, 1, nr_irqs, GICD_BASE_GPA, GICR_BASE_GPA); + if (gic_fd < 0) { + pr_info("Failed to create vgic-v3, skipping\n"); + exit(KSFT_SKIP); + } vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handlers[args.eoi_split][args.level_sensitive]); diff --git a/tools/testing/selftests/kvm/lib/aarch64/vgic.c b/tools/testing/selftests/kvm/lib/aarch64/vgic.c index b3a0fca0d780..4ea65e119bdd 100644 --- a/tools/testing/selftests/kvm/lib/aarch64/vgic.c +++ b/tools/testing/selftests/kvm/lib/aarch64/vgic.c @@ -51,7 +51,10 @@ int vgic_v3_setup(struct kvm_vm *vm, unsigned int nr_vcpus, uint32_t nr_irqs, "Number of vCPUs requested (%u) doesn't match with the ones created for the VM (%u)\n", nr_vcpus, nr_vcpus_created); - /* Distributor setup */ + /* Distributor setup - test if it's possible then actually do it */ + gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, true); + if (gic_fd != 0) + return -1; gic_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false); kvm_device_access(gic_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
The arch_timer and vgic_irq kselftests assume that they can create a vgic-v3, using the library function vgic_v3_setup() which aborts with a test failure if it is not possible to do so. Since vgic-v3 can only be instantiated on systems where the host has GICv3 this leads to false positives on older systems where that is not the case. Fix this by changing vgic_v3_setup() to return an error if the vgic can't be instantiated and have the callers skip if this happens. We could also exit flagging a skip in vgic_v3_setup() but this would prevent future test cases conditionally deciding which GIC to use or generally doing more complex output. Signed-off-by: Mark Brown <broonie@kernel.org> --- v2: - The test for being able to create the GIC doesn't actually instantiate it, add a call doing so in that case. tools/testing/selftests/kvm/aarch64/arch_timer.c | 7 ++++++- tools/testing/selftests/kvm/aarch64/vgic_irq.c | 4 ++++ tools/testing/selftests/kvm/lib/aarch64/vgic.c | 5 ++++- 3 files changed, 14 insertions(+), 2 deletions(-)