linux-kernel - Re: [PATCH] selftests/kvm: add get_msr_index

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52d09cdf-3819-0cd8-5812-387febaa1ab3@redhat.com>
Date:   Fri, 19 Mar 2021 09:07:07 +0100
From:   Emanuele Giuseppe Esposito <eesposit@...hat.com>
To:     Andrew Jones <drjones@...hat.com>
Cc:     linux-kselftest@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Shuah Khan <shuah@...nel.org>,
        Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] selftests/kvm: add get_msr_index_features

Hi Andrew,

Thank you for the feedback (also in v1).

On 18/03/2021 18:03, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 03:56:29PM +0100, Emanuele Giuseppe Esposito wrote:
>> Test the KVM_GET_MSR_FEATURE_INDEX_LIST
>> and KVM_GET_MSR_INDEX_LIST ioctls.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@...hat.com>
>> ---
>>   tools/testing/selftests/kvm/.gitignore        |   1 +
>>   tools/testing/selftests/kvm/Makefile          |   1 +
>>   .../kvm/x86_64/get_msr_index_features.c       | 124 ++++++++++++++++++
>>   3 files changed, 126 insertions(+)
>>   create mode 100644 tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
>>
>> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
>> index 32b87cc77c8e..d99f3969d371 100644
>> --- a/tools/testing/selftests/kvm/.gitignore
>> +++ b/tools/testing/selftests/kvm/.gitignore
>> @@ -5,6 +5,7 @@
>>   /s390x/resets
>>   /s390x/sync_regs_test
>>   /x86_64/cr4_cpuid_sync_test
>> +/x86_64/get_msr_index_features
>>   /x86_64/debug_regs
>>   /x86_64/evmcs_test
>>   /x86_64/get_cpuid_test
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index a6d61f451f88..c748b9650e28 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -39,6 +39,7 @@ LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
>>   LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>>   
>>   TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
>> +TEST_GEN_PROGS_x86_64 += x86_64/get_msr_index_features
> 
> Maybe we should give up trying to keep an alphabetic order.

My bad, I did not notice that it was in alphabetic order.

> 
>>   TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
>>   TEST_GEN_PROGS_x86_64 += x86_64/get_cpuid_test
>>   TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
>> diff --git a/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c b/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
>> new file mode 100644
>> index 000000000000..ad9972d99dfa
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
>> @@ -0,0 +1,124 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Test that KVM_GET_MSR_INDEX_LIST and
>> + * KVM_GET_MSR_FEATURE_INDEX_LIST work as intended
>> + *
>> + * Copyright (C) 2020, Red Hat, Inc.
>> + */
>> +#include <fcntl.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <sys/ioctl.h>
>> +
>> +#include "test_util.h"
>> +#include "kvm_util.h"
>> +#include "processor.h"
>> +#include "../lib/kvm_util_internal.h"
> 
> I'm not sure why the original kvm selftests authors decided to do this
> internal stuff, but we should either kill that or avoid doing stuff like
> this.

I need this include because of the KVM_DEV_PATH macro, to get the kvm_fd.
No other reason for including it in this test.
>> +
>> +static int kvm_num_feature_msrs(int kvm_fd, int nmsrs)
>> +{
>> +	struct kvm_msr_list *list;
>> +	int r;
>> +
>> +	list = malloc(sizeof(*list) + nmsrs * sizeof(list->indices[0]));
>> +	list->nmsrs = nmsrs;
>> +	r = ioctl(kvm_fd, KVM_GET_MSR_FEATURE_INDEX_LIST, list);
>> +	TEST_ASSERT(r == -1 && errno == E2BIG,
>> +		"Unexpected result from KVM_GET_MSR_FEATURE_INDEX_LIST probe, r: %i",
>> +				r);
> 
> Weird indentation. I'd just leave it up on the last line. We don't care
> about long lines.

Ok. I wanted avoid warnings from the checkpatch script.

Paolo, do you want me to send v2 with fixed indentation or you already 
took care of it? I'll be happy to do so.

Emanuele