* [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST
@ 2021-03-17 7:45 Emanuele Giuseppe Esposito
2021-03-17 10:49 ` Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: Emanuele Giuseppe Esposito @ 2021-03-17 7:45 UTC (permalink / raw)
To: linux-kselftest
Cc: Paolo Bonzini, Shuah Khan, Vitaly Kuznetsov, Andrew Jones,
linux-kernel, kvm
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST
2021-03-17 7:45 [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST Emanuele Giuseppe Esposito
@ 2021-03-17 10:49 ` Paolo Bonzini
2021-03-17 11:25 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2021-03-17 10:49 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, linux-kselftest
Cc: Shuah Khan, Vitaly Kuznetsov, Andrew Jones, linux-kernel, kvm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST
2021-03-17 10:49 ` Paolo Bonzini
@ 2021-03-17 11:25 ` Emanuele Giuseppe Esposito
2021-03-17 12:07 ` Andrew Jones
0 siblings, 1 reply; 4+ messages in thread
From: Emanuele Giuseppe Esposito @ 2021-03-17 11:25 UTC (permalink / raw)
To: Paolo Bonzini, linux-kselftest
Cc: Shuah Khan, Vitaly Kuznetsov, Andrew Jones, linux-kernel, kvm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST
2021-03-17 11:25 ` Emanuele Giuseppe Esposito
@ 2021-03-17 12:07 ` Andrew Jones
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Jones @ 2021-03-17 12:07 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Paolo Bonzini, linux-kselftest, Shuah Khan, Vitaly Kuznetsov,
linux-kernel, kvm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-17 12:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-17 7:45 [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST Emanuele Giuseppe Esposito
2021-03-17 10:49 ` Paolo Bonzini
2021-03-17 11:25 ` Emanuele Giuseppe Esposito
2021-03-17 12:07 ` Andrew Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).