| Message ID | 20161216134348.16236-3-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/12/16 13:43, Haozhong Zhang wrote: > diff --git a/tests/vvmx/msr.c b/tests/vvmx/msr.c > new file mode 100644 > index 0000000..100491d > --- /dev/null > +++ b/tests/vvmx/msr.c > @@ -0,0 +1,67 @@ > +#include <xtf.h> > + > +#include <arch/x86/msr-index.h> > + > +/* > + * NB. Guest MSR_IA32_FEATURE_CONTROL is set by Xen hypervisor instead > + * by guest firmware or hvmloader, so this test checks whether bits in > + * MSR_IA32_FEATURE_CONTROL are set correctly and does not require they > + * are all zero. > + * > + * TODO: If the behavior in above NB is changed in future, remember to > + * adjust this test. What are the purposes of these lock bits on real hardware? I presume it is to allow a user choice in the BIOS, but to force the choice to be immutable once the OS loads? If so, I don't expect Xen's behaviour to ever change. We'd prefer to do all configuration like that at the toolstack/hypervisor level, rather than the in-guest firmware (if any). > + */ > +static bool test_msr_feature_control(void) > +{ > + bool passed = true; > + uint32_t cpuid_feat_ecx = cpuid_ecx(1); > + uint64_t msr_feat_ctrl; > + > + if ( rdmsr_safe(MSR_IA32_FEATURE_CONTROL, &msr_feat_ctrl) ) > + { > + xtf_failure("Fail: fault when rdmsr MSR_IA32_FEATURE_CONTROL\n"); "Fail: Fault when reading MSR_IA32_FEATURE_CONTROL\n" would be slightly clearer. > + passed = false; The status functions including xtf_failure and xtf_success are sticky, worst-takes-priority. Therefore, you don't need the xtf_failure(NULL) case in test_main(). You can also even leave via the xtf_success(NULL) case and the overall report will be FAILURE. > + goto out; In the past, I have found it very useful to proceed with as many checks as reasonable, even in the face of a failure. Sometime, a subtle change in Xen can have a cascade failure for the guest, and seeing all the failures at once can help identify which area went wrong. > + } > + > + if ( (msr_feat_ctrl & IA32_FEATURE_CONTROL_ENABLE_VMXON_INSIDE_SMX) && > + !(cpuid_feat_ecx & X86_FEATURE_SMX) ) cpu_has_smx please. ~Andrew > + { > + xtf_failure("Fail: MSR_IA32_FEATURE_CONTROL[1] is set " > + "but SMX is not supported\n"); > + passed = false; > + } > + > + if ( !(msr_feat_ctrl & IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX) ) > + { > + xtf_failure("Fail: MSR_IA32_FEATURE_CONTROL[2] is not set\n"); > + passed = false; > + } > + > + if ( !(msr_feat_ctrl & IA32_FEATURE_CONTROL_LOCK) ) > + { > + xtf_failure("Fail: MSR_IA32_FEATURE_CONTROL[0] is not set\n"); > + passed = false; > + } > + > +out: > + return passed; > +} > + > +bool test_msr_vmx(void) > +{ > + if ( !test_msr_feature_control() ) > + return false; > + > + return true; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */
On 12/16/16 16:17 +0000, Andrew Cooper wrote: >On 16/12/16 13:43, Haozhong Zhang wrote: >> diff --git a/tests/vvmx/msr.c b/tests/vvmx/msr.c >> new file mode 100644 >> index 0000000..100491d >> --- /dev/null >> +++ b/tests/vvmx/msr.c >> @@ -0,0 +1,67 @@ >> +#include <xtf.h> >> + >> +#include <arch/x86/msr-index.h> >> + >> +/* >> + * NB. Guest MSR_IA32_FEATURE_CONTROL is set by Xen hypervisor instead >> + * by guest firmware or hvmloader, so this test checks whether bits in >> + * MSR_IA32_FEATURE_CONTROL are set correctly and does not require they >> + * are all zero. >> + * >> + * TODO: If the behavior in above NB is changed in future, remember to >> + * adjust this test. > >What are the purposes of these lock bits on real hardware? I presume it >is to allow a user choice in the BIOS, but to force the choice to be >immutable once the OS loads? > same as my understanding >If so, I don't expect Xen's behaviour to ever change. We'd prefer to do >all configuration like that at the toolstack/hypervisor level, rather >than the in-guest firmware (if any). > OK, then TODO is not necessary at all. My concern was that Intel SDM says MSR_IA32_FEATURE_CONTROL is cleared to zero when a logical processor is reset that is not consistent to what Xen currently presents in HVM domains. >> + */ >> +static bool test_msr_feature_control(void) >> +{ >> + bool passed = true; >> + uint32_t cpuid_feat_ecx = cpuid_ecx(1); >> + uint64_t msr_feat_ctrl; >> + >> + if ( rdmsr_safe(MSR_IA32_FEATURE_CONTROL, &msr_feat_ctrl) ) >> + { >> + xtf_failure("Fail: fault when rdmsr MSR_IA32_FEATURE_CONTROL\n"); > >"Fail: Fault when reading MSR_IA32_FEATURE_CONTROL\n" would be slightly >clearer. > will change >> + passed = false; > >The status functions including xtf_failure and xtf_success are sticky, >worst-takes-priority. Therefore, you don't need the xtf_failure(NULL) >case in test_main(). You can also even leave via the xtf_success(NULL) >case and the overall report will be FAILURE. > Thanks for reminding me of this which I didn't notice. >> + goto out; > >In the past, I have found it very useful to proceed with as many checks >as reasonable, even in the face of a failure. > >Sometime, a subtle change in Xen can have a cascade failure for the >guest, and seeing all the failures at once can help identify which area >went wrong. > I'll add case to test vmxon w/ VMX is turned off by hypervisor. >> + } >> + >> + if ( (msr_feat_ctrl & IA32_FEATURE_CONTROL_ENABLE_VMXON_INSIDE_SMX) && >> + !(cpuid_feat_ecx & X86_FEATURE_SMX) ) > >cpu_has_smx please. > Will change. Thanks, Haozhong
On 19/12/16 02:20, Haozhong Zhang wrote: > On 12/16/16 16:17 +0000, Andrew Cooper wrote: >> If so, I don't expect Xen's behaviour to ever change. We'd prefer to do >> all configuration like that at the toolstack/hypervisor level, rather >> than the in-guest firmware (if any). >> > > OK, then TODO is not necessary at all. My concern was that Intel SDM > says MSR_IA32_FEATURE_CONTROL is cleared to zero when a logical > processor is reset that is not consistent to what Xen currently > presents in HVM domains. The firmware line is somewhat blurred in virtualisation. Even with reset, I'd still expect Xen to handle this part of the state, in accordance with the toolstack settings. ~Andrew
diff --git a/include/arch/x86/msr-index.h b/include/arch/x86/msr-index.h index d479239..f9867d5 100644 --- a/include/arch/x86/msr-index.h +++ b/include/arch/x86/msr-index.h @@ -3,6 +3,11 @@ #include <xtf/numbers.h> +#define MSR_IA32_FEATURE_CONTROL 0x0000003a +#define IA32_FEATURE_CONTROL_LOCK 0x0001 +#define IA32_FEATURE_CONTROL_ENABLE_VMXON_INSIDE_SMX 0x0002 +#define IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX 0x0004 + #define MSR_INTEL_PLATFORM_INFO 0x000000ce #define _MSR_PLATFORM_INFO_CPUID_FAULTING 31 #define MSR_PLATFORM_INFO_CPUID_FAULTING (1ULL << _MSR_PLATFORM_INFO_CPUID_FAULTING) diff --git a/tests/vvmx/Makefile b/tests/vvmx/Makefile index 80a6629..54769fa 100644 --- a/tests/vvmx/Makefile +++ b/tests/vvmx/Makefile @@ -6,6 +6,6 @@ TEST-ENVS := hvm64 TEST-EXTRA-CFG := extra.cfg.in -obj-perenv += main.o cpuid.o +obj-perenv += main.o cpuid.o msr.o include $(ROOT)/build/gen.mk diff --git a/tests/vvmx/main.c b/tests/vvmx/main.c index 8506b7b..bd36f10 100644 --- a/tests/vvmx/main.c +++ b/tests/vvmx/main.c @@ -14,6 +14,7 @@ const char test_title[] = "Test vvmx"; extern bool test_cpuid_vmx_feat(void); +extern bool test_msr_vmx(void); void test_main(void) { @@ -26,6 +27,9 @@ void test_main(void) if ( !test_cpuid_vmx_feat() ) goto fail; + if ( !test_msr_vmx() ) + goto fail; + xtf_success(NULL); return; diff --git a/tests/vvmx/msr.c b/tests/vvmx/msr.c new file mode 100644 index 0000000..100491d --- /dev/null +++ b/tests/vvmx/msr.c @@ -0,0 +1,67 @@ +#include <xtf.h> + +#include <arch/x86/msr-index.h> + +/* + * NB. Guest MSR_IA32_FEATURE_CONTROL is set by Xen hypervisor instead + * by guest firmware or hvmloader, so this test checks whether bits in + * MSR_IA32_FEATURE_CONTROL are set correctly and does not require they + * are all zero. + * + * TODO: If the behavior in above NB is changed in future, remember to + * adjust this test. + */ +static bool test_msr_feature_control(void) +{ + bool passed = true; + uint32_t cpuid_feat_ecx = cpuid_ecx(1); + uint64_t msr_feat_ctrl; + + if ( rdmsr_safe(MSR_IA32_FEATURE_CONTROL, &msr_feat_ctrl) ) + { + xtf_failure("Fail: fault when rdmsr MSR_IA32_FEATURE_CONTROL\n"); + passed = false; + goto out; + } + + if ( (msr_feat_ctrl & IA32_FEATURE_CONTROL_ENABLE_VMXON_INSIDE_SMX) && + !(cpuid_feat_ecx & X86_FEATURE_SMX) ) + { + xtf_failure("Fail: MSR_IA32_FEATURE_CONTROL[1] is set " + "but SMX is not supported\n"); + passed = false; + } + + if ( !(msr_feat_ctrl & IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX) ) + { + xtf_failure("Fail: MSR_IA32_FEATURE_CONTROL[2] is not set\n"); + passed = false; + } + + if ( !(msr_feat_ctrl & IA32_FEATURE_CONTROL_LOCK) ) + { + xtf_failure("Fail: MSR_IA32_FEATURE_CONTROL[0] is not set\n"); + passed = false; + } + +out: + return passed; +} + +bool test_msr_vmx(void) +{ + if ( !test_msr_feature_control() ) + return false; + + return true; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */
Guest MSR_IA32_FEATURE_CONTROL is set by Xen hypervisor instead by guest firmware or hvmloader, so this test instead checks whether bits in MSR_IA32_FEATURE_CONTROL are set correctly, rather than requiring they are all zeroed. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- include/arch/x86/msr-index.h | 5 ++++ tests/vvmx/Makefile | 2 +- tests/vvmx/main.c | 4 +++ tests/vvmx/msr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 tests/vvmx/msr.c