[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <61d11f32-a2da-b593-1c62-bbadc6408215@redhat.com>
Date: Wed, 17 Mar 2021 12:25:52 +0100
From: Emanuele Giuseppe Esposito <eesposit@...hat.com>
To: Paolo Bonzini <pbonzini@...hat.com>,
linux-kselftest@...r.kernel.org
Cc: Shuah Khan <shuah@...nel.org>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Andrew Jones <drjones@...hat.com>,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH] selftests/kvm: add test for
KVM_GET_MSR_FEATURE_INDEX_LIST
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
Powered by blists - more mailing lists