Re: [RFC PATCH v2 3/3] selftests: kvm/x86: Test the flags in MSR filtering / exiting

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 21, 2022, Aaron Lewis wrote:
> > > --- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> > > @@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void)
> > >       kvm_vm_free(vm);
> > >  }
> > >
> > > +static void test_results(int rc, const char *scmd, bool expected_success)
> >
> > Rather than pass in "success expected", pass in the actual value and the valid
> > mask.  Then you can spit out the problematic value in the assert and be kind to
> > future debuggers.
> >
> > And similarly, make the __vm_ioctl() call here instead of in the "caller" and name
> > this __test_ioctl() (rename as necessary, see below) to show it's relationship with
> > the macro.
> 
> The other comments look good.  I'll update.
> 
> This one is a bit tricky though.  I did originally have __vm_ioctl()
> in test_results() (or whatever name it will end up with), but the
> static assert in kvm_do_ioctl() gave me problems.  Unless I make
> test_results() a macro, I have to force cmd to a uint64_t or something
> other than a literal, then I get this:
> 
> include/kvm_util_base.h:190:39: error: expression in static assertion
> is not constant
> 190 |         static_assert(!_IOC_SIZE(cmd) || sizeof(*arg) ==
> _IOC_SIZE(cmd), "");   \
>        |                                       ^
> include/kvm_util_base.h:213:9: note: in expansion of macro ‘kvm_do_ioctl’
> 213 |         kvm_do_ioctl((vm)->fd, cmd, arg);                       \
> 
> That's not the only problem.  In order to pass 'arg' in I would have
> to pass it as a void *, making sizeof(*arg) wrong.
> 
> Being that the ioctl call was the first thing I did in that function I
> opted to make it a part of test_ioctl() rather than making
> test_results() a macro.
> 
> If only C had templates :)

Eh, what C++ can do with templates, C can usually do with macros :-)

Sans backslashes, I think this can simply be:

#define test_user_exit_msr_ioctl(vm, cmd, arg, val, valid_mask)
({
	int r = __vm_ioctl(vm, cmd, arg);

	if (val & valid_mask)
		TEST_ASSERT(!r, KVM_IOCTL_ERROR(cmd, r));
	else
		TEST_ASSERT(r == -1 && errno == EINVAL,
			    "Wanted EINVAL with val = 0x%llx, got  rc: %i errno: %i (%s)",
			    val, r, errno,  strerror(errno))
})


It doesn't print "val" when success is expected, but I'm ok with that.  Though I
suspect that adding a common macro to print additional info on an unexpected
ioctl() result would be useful for other tests.