Messages in this thread | | | Subject | Re: [PATCH] selftests/kvm: add test for KVM_GET_MSR_FEATURE_INDEX_LIST | From | Emanuele Giuseppe Esposito <> | Date | Wed, 17 Mar 2021 12:25:52 +0100 |
| |
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
| |