| Message ID | 20210317074552.8550-1-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST | expand |
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
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
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
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);
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(+)