Re: [PATCH v3 2/3] selftests: KVM: AMD Nested test infrastructure

[Wei Huang]

On 2/7/20 3:53 AM, Auger Eric wrote:
[snip]
>>> +
>>> +#define	SVM_EXITINTINFO_TYPE_INTR SVM_EVTINJ_TYPE_INTR
>>> +#define	SVM_EXITINTINFO_TYPE_NMI SVM_EVTINJ_TYPE_NMI
>>> +#define	SVM_EXITINTINFO_TYPE_EXEPT SVM_EVTINJ_TYPE_EXEPT
>>> +#define	SVM_EXITINTINFO_TYPE_SOFT SVM_EVTINJ_TYPE_SOFT
>>           ^^^^^^
>> TAB instead of SPACE
> 
> as written in the history log (but I think I will add this to the commit
> msg too), this file is an exact copy of arch/x86/include/asm/svm.h
> (except the header includer #ifdef + uapi/asm/svm.h header inclusion. So
> it inherits the style issue of its parent ;-)
>>
>>> +
>>> +#define SVM_EXITINTINFO_VALID SVM_EVTINJ_VALID
>>> +#define SVM_EXITINTINFO_VALID_ERR SVM_EVTINJ_VALID_ERR
>>> +
>>> +#define SVM_EXITINFOSHIFT_TS_REASON_IRET 36
>>> +#define SVM_EXITINFOSHIFT_TS_REASON_JMP 38
>>> +#define SVM_EXITINFOSHIFT_TS_HAS_ERROR_CODE 44
>>> +
>>> +#define SVM_EXITINFO_REG_MASK 0x0F
>>> +
>>> +#define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
>>> +
>>> +#endif /* SELFTEST_KVM_SVM_H */
>>> diff --git a/tools/testing/selftests/kvm/include/x86_64/svm_util.h b/tools/testing/selftests/kvm/include/x86_64/svm_util.h
>>> new file mode 100644
>>> index 000000000000..6a67a89c5d06
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/kvm/include/x86_64/svm_util.h
>>> @@ -0,0 +1,36 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * tools/testing/selftests/kvm/include/x86_64/svm_utils.h
>>> + * Header for nested SVM testing
>>> + *
>>> + * Copyright (C) 2020, Red Hat, Inc.
>>> + */
>>> +
>>> +#ifndef SELFTEST_KVM_SVM_UTILS_H
>>> +#define SELFTEST_KVM_SVM_UTILS_H
>>> +
>>> +#include <stdint.h>
>>> +#include "svm.h"
>>> +#include "processor.h"
>>> +
>>> +#define CPUID_SVM_BIT		2
>>> +#define CPUID_SVM		BIT_ULL(CPUID_SVM_BIT)
>>> +
>>> +#define SVM_EXIT_VMMCALL	0x081
>>
>> SVM_EXIT_VMMCALL is better to relocate to svm.h file as it is an
>> architecture definition.
> For the same reason I am tempted to leave this definition here for now.
> Maybe at some point if we introduce some additional ones, this will
> indeed deserve to be moved to the parent? Is it ok?
> 

I figured out this was your intention when I compared arch/x86/include/asm/svm.h with tools/testing/selftests/kvm/include/x86_64/svm.h. However I also noticed that vmx.h in tools/testing/selftests/kvm/include/x86_64/ is not identical as arch/x86/include/asm/vmx.h. So being the same isn't a hard requirement. I am OK with either way.

-Wei