[PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST

[PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST

[Emanuele Giuseppe Esposito]

Extend the kvm_get_feature_msr function to cover also
KVM_GET_MSR_FEATURE_INDEX_LIST.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tools/testing/selftests/kvm/lib/x86_64/processor.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index a8906e60a108..3eaa6b0172a9 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -688,13 +688,20 @@ uint64_t kvm_get_feature_msr(uint64_t msr_index)
 		struct kvm_msr_entry entry;
 	} buffer = {};
 	int r, kvm_fd;
+	struct kvm_msr_list features_list;
 
 	buffer.header.nmsrs = 1;
 	buffer.entry.index = msr_index;
+	features_list.nmsrs = 1;
+
 	kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
 	if (kvm_fd < 0)
 		exit(KSFT_SKIP);
 
+	r = ioctl(kvm_fd, KVM_GET_MSR_FEATURE_INDEX_LIST, &features_list);
+	TEST_ASSERT(r < 0 && r != -E2BIG, "KVM_GET_MSR_FEATURE_INDEX_LIST IOCTL failed,\n"
+		"  rc: %i errno: %i", r, errno);
+
 	r = ioctl(kvm_fd, KVM_GET_MSRS, &buffer.header);
 	TEST_ASSERT(r == 1, "KVM_GET_MSRS IOCTL failed,\n"
 		"  rc: %i errno: %i", r, errno);
-- 
2.29.2

Re: [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST

[Paolo Bonzini]

On 17/03/21 08:45, Emanuele Giuseppe Esposito wrote:
> +	struct kvm_msr_list features_list;
>   
>   	buffer.header.nmsrs = 1;
>   	buffer.entry.index = msr_index;
> +	features_list.nmsrs = 1;
> +
>   	kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
>   	if (kvm_fd < 0)
>   		exit(KSFT_SKIP);
>   
> +	r = ioctl(kvm_fd, KVM_GET_MSR_FEATURE_INDEX_LIST, &features_list);
> +	TEST_ASSERT(r < 0 && r != -E2BIG, "KVM_GET_MSR_FEATURE_INDEX_LIST IOCTL failed,\n"
> +		"  rc: %i errno: %i", r, errno);

Careful: because this has nsmrs == 1, you are overwriting an u32 of the 
stack after struct kvm_msr_list.  You need to use your own struct 
similar to what is done with "buffer.header" and "buffer.entry".

>   	r = ioctl(kvm_fd, KVM_GET_MSRS, &buffer.header);
>   	TEST_ASSERT(r == 1, "KVM_GET_MSRS IOCTL failed,\n"
>   		"  rc: %i errno: %i", r, errno);
> 

More in general, this is not a test, but rather a library function used 
to read a single MSR.

If you would like to add a test for KVM_GET_MSR_FEATURE_INDEX_LIST that 
would be very welcome.  That would be a new executable.  Looking at the 
logic for the ioctl, the main purpose of the test should be:

- check that if features_list.nmsrs is too small it will set the nmsrs 
field and return -E2BIG.

- check that all MSRs returned by KVM_GET_MSR_FEATURE_INDEX_LIST can be 
accessed with KVM_GET_MSRS

So something like this:

   set nmsrs to 0 and try the ioctl
   check that it returns -E2BIG and has changed nmsrs
   if nmsrs != 1 {
     set nmsrs to 1 and try the ioctl again
     check that it returns -E2BIG
   }
   malloc a buffer with room for struct kvm_msr_list and nmsrs indices
   set nmsrs in the malloc-ed buffer and try the ioctl again
   for each index
     invoke kvm_get_feature_msr to read it

(The test should also be skipped if KVM does not expose the 
KVM_CAP_GET_MSR_FEATURES capability).

Thanks,

Paolo

Re: [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST

[Emanuele Giuseppe Esposito]

On 17/03/2021 11:49, Paolo Bonzini wrote:
> On 17/03/21 08:45, Emanuele Giuseppe Esposito wrote:
>> +    struct kvm_msr_list features_list;
>>       buffer.header.nmsrs = 1;
>>       buffer.entry.index = msr_index;
>> +    features_list.nmsrs = 1;
>> +
>>       kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
>>       if (kvm_fd < 0)
>>           exit(KSFT_SKIP);
>> +    r = ioctl(kvm_fd, KVM_GET_MSR_FEATURE_INDEX_LIST, &features_list);
>> +    TEST_ASSERT(r < 0 && r != -E2BIG, "KVM_GET_MSR_FEATURE_INDEX_LIST 
>> IOCTL failed,\n"
>> +        "  rc: %i errno: %i", r, errno);
> 
> Careful: because this has nsmrs == 1, you are overwriting an u32 of the 
> stack after struct kvm_msr_list.  You need to use your own struct 
> similar to what is done with "buffer.header" and "buffer.entry".
> 
>>       r = ioctl(kvm_fd, KVM_GET_MSRS, &buffer.header);
>>       TEST_ASSERT(r == 1, "KVM_GET_MSRS IOCTL failed,\n"
>>           "  rc: %i errno: %i", r, errno);
>>
> 
> More in general, this is not a test, but rather a library function used 
> to read a single MSR.
> 
> If you would like to add a test for KVM_GET_MSR_FEATURE_INDEX_LIST that 
> would be very welcome.  That would be a new executable.  Looking at the 
> logic for the ioctl, the main purpose of the test should be:
> 
> - check that if features_list.nmsrs is too small it will set the nmsrs 
> field and return -E2BIG.
> 
> - check that all MSRs returned by KVM_GET_MSR_FEATURE_INDEX_LIST can be 
> accessed with KVM_GET_MSRS
> 
> So something like this:
> 
>    set nmsrs to 0 and try the ioctl
>    check that it returns -E2BIG and has changed nmsrs
>    if nmsrs != 1 {
>      set nmsrs to 1 and try the ioctl again
>      check that it returns -E2BIG
>    }
>    malloc a buffer with room for struct kvm_msr_list and nmsrs indices
>    set nmsrs in the malloc-ed buffer and try the ioctl again
>    for each index
>      invoke kvm_get_feature_msr to read it
> 
> (The test should also be skipped if KVM does not expose the 
> KVM_CAP_GET_MSR_FEATURES capability).

Thank you for the feedback, the title is indeed a little bit misleading. 
My idea in this patch was to just add an additional check to all usages 
of KVM_GET_MSRS, since KVM_GET_MSR_FEATURE_INDEX_LIST is used only to 
probe host capabilities and processor features.
But you are right, a separate test would be better.

Thank you,
Emanuele

Re: [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST

[Andrew Jones]

On Wed, Mar 17, 2021 at 12:25:52PM +0100, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 17/03/2021 11:49, Paolo Bonzini wrote:
> > On 17/03/21 08:45, Emanuele Giuseppe Esposito wrote:
> > > +    struct kvm_msr_list features_list;
> > >       buffer.header.nmsrs = 1;
> > >       buffer.entry.index = msr_index;
> > > +    features_list.nmsrs = 1;
> > > +
> > >       kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> > >       if (kvm_fd < 0)
> > >           exit(KSFT_SKIP);
> > > +    r = ioctl(kvm_fd, KVM_GET_MSR_FEATURE_INDEX_LIST, &features_list);
> > > +    TEST_ASSERT(r < 0 && r != -E2BIG,
> > > "KVM_GET_MSR_FEATURE_INDEX_LIST IOCTL failed,\n"
> > > +        "  rc: %i errno: %i", r, errno);
> > 
> > Careful: because this has nsmrs == 1, you are overwriting an u32 of the
> > stack after struct kvm_msr_list.  You need to use your own struct
> > similar to what is done with "buffer.header" and "buffer.entry".
> > 
> > >       r = ioctl(kvm_fd, KVM_GET_MSRS, &buffer.header);
> > >       TEST_ASSERT(r == 1, "KVM_GET_MSRS IOCTL failed,\n"
> > >           "  rc: %i errno: %i", r, errno);
> > > 
> > 
> > More in general, this is not a test, but rather a library function used
> > to read a single MSR.
> > 
> > If you would like to add a test for KVM_GET_MSR_FEATURE_INDEX_LIST that
> > would be very welcome.  That would be a new executable.  Looking at the
> > logic for the ioctl, the main purpose of the test should be:
> > 
> > - check that if features_list.nmsrs is too small it will set the nmsrs
> > field and return -E2BIG.
> > 
> > - check that all MSRs returned by KVM_GET_MSR_FEATURE_INDEX_LIST can be
> > accessed with KVM_GET_MSRS
> > 
> > So something like this:
> > 
> >    set nmsrs to 0 and try the ioctl
> >    check that it returns -E2BIG and has changed nmsrs
> >    if nmsrs != 1 {
> >      set nmsrs to 1 and try the ioctl again
> >      check that it returns -E2BIG
> >    }
> >    malloc a buffer with room for struct kvm_msr_list and nmsrs indices
> >    set nmsrs in the malloc-ed buffer and try the ioctl again
> >    for each index
> >      invoke kvm_get_feature_msr to read it
> > 
> > (The test should also be skipped if KVM does not expose the
> > KVM_CAP_GET_MSR_FEATURES capability).
> 
> Thank you for the feedback, the title is indeed a little bit misleading. My
> idea in this patch was to just add an additional check to all usages of
> KVM_GET_MSRS, since KVM_GET_MSR_FEATURE_INDEX_LIST is used only to probe
> host capabilities and processor features.
> But you are right, a separate test would be better.
>

Hi Emanuele,

You might be able to get some inspiration from the aarch64/get-reg-list.c
test. The list of MSRs varies with KVM version and host processor, but
there may be a set of MSRs that does not vary with host processor and
should not be removed in later KVM versions. If that's the case, then
the !missing_regs assert concept of aarch64/get-reg-list.c may also
apply to this new test. Based on Paolo's comment, I presume at least the
!failed_get should apply. Finally, the test should do the E2BIG checks,
as Paolo states, but you may also want to create a lib function for
KVM_GET_MSR_FEATURE_INDEX_LIST, similar to vcpu_get_reg_list(), if you
think it may be of use to other tests.

Thanks,
drew