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
