Re: [PATCH v4 5/5] selftests: kvm/x86: Test the flags in MSR filtering and MSR exiting

From: Sean Christopherson <seanjc@google.com>
To: Aaron Lewis <aaronlewis@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, jmattson@google.com
Subject: Re: [PATCH v4 5/5] selftests: kvm/x86: Test the flags in MSR filtering and MSR exiting
Date: Fri, 7 Oct 2022 22:24:19 +0000	[thread overview]
Message-ID: <Y0CnE4iqo60j2wdT@google.com> (raw)
In-Reply-To: <20220921151525.904162-6-aaronlewis@google.com>

On Wed, Sep 21, 2022, Aaron Lewis wrote:
> +#define test_user_exit_msr_ioctl(vm, cmd, arg, flag, valid_mask)	\

There's nothing specific to userspace MSR exiting in this macro.  To keep the
name short, and to potentially allow moving it to common code in the future, how
about test_ioctl_flags()?

> +({									\
> +	int r = __vm_ioctl(vm, cmd, arg);				\
> +									\
> +	if (flag & valid_mask)						\
> +		TEST_ASSERT(!r, __KVM_IOCTL_ERROR(#cmd, r));		\
> +	else								\
> +		TEST_ASSERT(r == -1 && errno == EINVAL,			\
> +			    "Wanted EINVAL for %s with flag = 0x%llx, got  rc: %i errno: %i (%s)", \
> +			    #cmd, flag, r, errno,  strerror(errno));	\
> +})
> +
> +static void run_user_space_msr_flag_test(struct kvm_vm *vm)
> +{
> +	struct kvm_enable_cap cap = { .cap = KVM_CAP_X86_USER_SPACE_MSR };
> +	int nflags = sizeof(cap.args[0]) * BITS_PER_BYTE;
> +	int rc;
> +	int i;

These declarations can go on a single line.

> +
> +	rc = kvm_check_cap(KVM_CAP_X86_USER_SPACE_MSR);
> +	TEST_ASSERT(rc, "KVM_CAP_X86_USER_SPACE_MSR is available");
> +
> +	for (i = 0; i < nflags; i++) {
> +		cap.args[0] = BIT_ULL(i);
> +		test_user_exit_msr_ioctl(vm, KVM_ENABLE_CAP, &cap,
> +			   BIT_ULL(i), KVM_MSR_EXIT_REASON_VALID_MASK);

Align params.  With a shorter macro name, that's easy to do without creating
massively long lines.

And pass in the actual flags, e.g. cap.args[0] here, so that it's slightly more
obvious what is being tested, and to minimize the risk of mixups.

E.g.

		test_ioctl_flags(vm, KVM_ENABLE_CAP, &cap, cap.args[0],
				 KVM_MSR_EXIT_REASON_VALID_MASK);

> +	}
> +}
> +
> +static void run_msr_filter_flag_test(struct kvm_vm *vm)
> +{
> +	u64 deny_bits = 0;
> +	struct kvm_msr_filter filter = {
> +		.flags = KVM_MSR_FILTER_DEFAULT_ALLOW,
> +		.ranges = {
> +			{
> +				.flags = KVM_MSR_FILTER_READ,
> +				.nmsrs = 1,
> +				.base = 0,
> +				.bitmap = (uint8_t *)&deny_bits,
> +			},
> +		},
> +	};
> +	int nflags;
> +	int rc;
> +	int i;

	int nflags, rc, i;

> +
> +	rc = kvm_check_cap(KVM_CAP_X86_MSR_FILTER);
> +	TEST_ASSERT(rc, "KVM_CAP_X86_MSR_FILTER is available");
> +
> +	nflags = sizeof(filter.flags) * BITS_PER_BYTE;
> +	for (i = 0; i < nflags; i++) {
> +		filter.flags = BIT_ULL(i);
> +		test_user_exit_msr_ioctl(vm, KVM_X86_SET_MSR_FILTER, &filter,
> +			   BIT_ULL(i), KVM_MSR_FILTER_VALID_MASK);

		test_ioctl_flags(vm, KVM_X86_SET_MSR_FILTER, &filter,
				 filter.flags, KVM_MSR_FILTER_VALID_MASK);

> +	}
> +
> +	filter.flags = KVM_MSR_FILTER_DEFAULT_ALLOW;
> +	nflags = sizeof(filter.ranges[0].flags) * BITS_PER_BYTE;
> +	for (i = 0; i < nflags; i++) {
> +		filter.ranges[0].flags = BIT_ULL(i);
> +		test_user_exit_msr_ioctl(vm, KVM_X86_SET_MSR_FILTER, &filter,
> +			   BIT_ULL(i), KVM_MSR_FILTER_RANGE_VALID_MASK);

		test_ioctl_flags(vm, KVM_X86_SET_MSR_FILTER, &filter,
				 filter.ranges[0].flags,
				 KVM_MSR_FILTER_RANGE_VALID_MASK);

> +	}
> +}

Nits aside, nice test!