On Tue, Jul 19, 2022, Aaron Lewis wrote:
> When using the flags in KVM_X86_SET_MSR_FILTER and
> KVM_CAP_X86_USER_SPACE_MSR it is expected that an attempt to write to
> any of the unused bits will fail. Add testing to walk over every bit
> in each of the flag fields in MSR filtering / exiting to verify that
> happens.
>
> Signed-off-by: Aaron Lewis <aaronlewis@xxxxxxxxxx>
> ---
> .../kvm/x86_64/userspace_msr_exit_test.c | 95 +++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> index f84dc37426f5..3b4ad16cc982 100644
> --- 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.
> +{
> + int expected_rc;
> +
> + expected_rc = expected_success ? 0 : -1;
> + TEST_ASSERT(rc == expected_rc,
> + "Unexpected result from '%s', rc: %d, expected rc: %d.",
> + scmd, rc, expected_rc);
> + TEST_ASSERT(!rc || (rc == -1 && errno == EINVAL),
> + "Failures are expected to have rc == -1 && errno == EINVAL(%d),\n"
> + " got rc: %d, errno: %d",
> + EINVAL, rc, errno);
> +}
> +
> +#define test_ioctl(vm, cmd, arg, expected_success) \
As above, just do e.g.
#define test_ioctl(vm, cmd, arg, val, valid_mask) \
__test_ioctl(vm, cmd, arg, #cmd, val, valid_mask)
Though it might be worth using a more verbose name? E.g. test_msr_filtering_ioctl()?
Hmm, but I guess KVM_CAP_X86_USER_SPACE_MSR isn't technically filtering.
test_user_exit_msr_ioctl()? Not a big deal if that's too wordy.
> +({ \
> + int rc = __vm_ioctl(vm, cmd, arg); \
> + \
> + test_results(rc, #cmd, expected_success); \
> +})
> +#define FLAG (1ul << i)
No. :-)
First, silently consuming local variables is Evil with a capital E.
Second, just use BIT() or BIT_ULL().
> +/* Test that attempts to write to the unused bits in a flag fails. */
> +static void test_flags(void)
For this one, definitely use a more verbose name, even if it seems stupidly
redundant. test_user_msr_exit_flags()?
> +{
> + struct kvm_vcpu *vcpu;
> + struct kvm_vm *vm;
> +
> + vm = vm_create_with_one_vcpu(&vcpu, NULL);
> +
> + /* Test flags for KVM_CAP_X86_USER_SPACE_MSR. */
> + run_user_space_msr_flag_test(vm);
> +
> + /* Test flags and range flags for KVM_X86_SET_MSR_FILTER. */
> + run_msr_filter_flag_test(vm);
> +
> + kvm_vm_free(vm);
> +}
> +
> int main(int argc, char *argv[])
> {
> /* Tell stdout not to buffer its content */
> @@ -745,5 +838,7 @@ int main(int argc, char *argv[])
>
> test_msr_permission_bitmap();
>
> + test_flags();
> +
> return 0;
> }
> --
> 2.37.1.359.gd136c6c3e2-goog
>
