* [PATCH v2] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3 @ 2022-01-26 13:53 Mark Brown 2022-01-26 14:17 ` Andrew Jones 0 siblings, 1 reply; 4+ messages in thread From: Mark Brown @ 2022-01-26 13:53 UTC (permalink / raw) To: Marc Zyngier, Shuah Khan Cc: Mark Brown, linux-kselftest, kvmarm, linux-arm-kernel 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"); + 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, -- 2.30.2 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3 2022-01-26 13:53 [PATCH v2] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3 Mark Brown @ 2022-01-26 14:17 ` Andrew Jones 2022-01-26 14:29 ` Mark Brown 0 siblings, 1 reply; 4+ messages in thread From: Andrew Jones @ 2022-01-26 14:17 UTC (permalink / raw) To: Mark Brown Cc: linux-arm-kernel, Marc Zyngier, Shuah Khan, kvmarm, linux-kselftest 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 > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3 2022-01-26 14:17 ` Andrew Jones @ 2022-01-26 14:29 ` Mark Brown 2022-01-26 14:50 ` Andrew Jones 0 siblings, 1 reply; 4+ messages in thread From: Mark Brown @ 2022-01-26 14:29 UTC (permalink / raw) To: Andrew Jones Cc: linux-arm-kernel, Marc Zyngier, Shuah Khan, kvmarm, linux-kselftest [-- Attachment #1.1: Type: text/plain, Size: 1406 bytes --] 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... > > - /* 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. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 151 bytes --] _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3 2022-01-26 14:29 ` Mark Brown @ 2022-01-26 14:50 ` Andrew Jones 0 siblings, 0 replies; 4+ messages in thread From: Andrew Jones @ 2022-01-26 14:50 UTC (permalink / raw) To: Mark Brown Cc: linux-arm-kernel, Marc Zyngier, Shuah Khan, kvmarm, linux-kselftest 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 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-26 14:50 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-26 13:53 [PATCH v2] kselftest: kvm/arm64: Skip tests if we can't create a vgic-v3 Mark Brown 2022-01-26 14:17 ` Andrew Jones 2022-01-26 14:29 ` Mark Brown 2022-01-26 14:50 ` Andrew Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).