| Message ID | 20161216134348.16236-4-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/12/16 13:43, Haozhong Zhang wrote: > It tests whether bit 31 and bit 48 are 0, and VMCS size is in the > range (0, 4096]. > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > --- > include/arch/x86/msr-index.h | 4 ++++ > tests/vvmx/msr.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 51 insertions(+) > > diff --git a/include/arch/x86/msr-index.h b/include/arch/x86/msr-index.h > index f9867d5..b7aeef0 100644 > --- a/include/arch/x86/msr-index.h > +++ b/include/arch/x86/msr-index.h > @@ -16,6 +16,10 @@ > #define _MSR_MISC_FEATURES_CPUID_FAULTING 0 > #define MSR_MISC_FEATURES_CPUID_FAULTING (1ULL << _MSR_MISC_FEATURES_CPUID_FAULTING) > > +#define MSR_IA32_VMX_BASIC 0x00000480 > +#define VMX_BASIC_VMCS_SIZE_MASK (0x1fffULL << 32) > +#define VMX_BASIC_32BIT_ADDRESSES (1ULL << 48) > + > #define MSR_EFER 0xc0000080 /* Extended Feature register. */ > #define _EFER_SCE 0 /* SYSCALL Enable. */ > #define EFER_SCE (_AC(1, L) << _EFER_SCE) > diff --git a/tests/vvmx/msr.c b/tests/vvmx/msr.c > index 100491d..ad01f26 100644 > --- a/tests/vvmx/msr.c > +++ b/tests/vvmx/msr.c > @@ -48,11 +48,58 @@ static bool test_msr_feature_control(void) > return passed; > } > > +static bool test_msr_vmx_basic(void) > +{ > + bool passed = true; > + uint64_t vmx_basic; > + uint64_t vmcs_size; > + > + if ( rdmsr_safe(MSR_IA32_VMX_BASIC, &vmx_basic) ) > + { > + xtf_failure("Fail: fault when rdmsr MSR_IA32_VMX_BASIC\n"); > + passed = false; > + goto out; > + } > + > + if ( vmx_basic & (1ULL << 31) ) > + { > + xtf_failure("Fail: MSR_IA32_VMX_BASIC[31] is not 0\n"); Out of interest, what is the reason for requiring this bit to be 0? I can see that the manual insists that it is, but not why. > + passed = false; > + } > + > + vmcs_size = (vmx_basic & VMX_BASIC_VMCS_SIZE_MASK) >> 32; > + if ( vmcs_size > PAGE_SIZE ) > + { > + xtf_failure("Fail: " > + "VMCS size (%"PRIu64") in MSR_IA32_VMX_BASIC is > %ld\n", > + vmcs_size, PAGE_SIZE); > + passed = false; > + } > + else if ( vmcs_size == 0 ) > + { > + xtf_failure("Fail: VMCS size in MSR_IA32_VMX_BASIC cannot be 0\n"); > + passed = false; > + } > + > + /* test is running on hvm64, so this bit should be 0 */ > + if ( vmx_basic & VMX_BASIC_32BIT_ADDRESSES ) There is nothing in principle wrong with Xen setting this bit. It would be odd certainly, but not erroneous. ~Andrew > + { > + xtf_failure("Fail: MSR_IA32_VMX_BASIC[48] is not 0\n"); > + passed = false; > + } > + > +out: > + return passed; > +} > + > bool test_msr_vmx(void) > { > if ( !test_msr_feature_control() ) > return false; > > + if ( !test_msr_vmx_basic() ) > + return false; > + > return true; > } >
On 12/16/16 17:19 +0000, Andrew Cooper wrote: >On 16/12/16 13:43, Haozhong Zhang wrote: >> It tests whether bit 31 and bit 48 are 0, and VMCS size is in the >> range (0, 4096]. >> >> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> >> --- >> include/arch/x86/msr-index.h | 4 ++++ >> tests/vvmx/msr.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 51 insertions(+) >> >> diff --git a/include/arch/x86/msr-index.h b/include/arch/x86/msr-index.h >> index f9867d5..b7aeef0 100644 >> --- a/include/arch/x86/msr-index.h >> +++ b/include/arch/x86/msr-index.h >> @@ -16,6 +16,10 @@ >> #define _MSR_MISC_FEATURES_CPUID_FAULTING 0 >> #define MSR_MISC_FEATURES_CPUID_FAULTING (1ULL << _MSR_MISC_FEATURES_CPUID_FAULTING) >> >> +#define MSR_IA32_VMX_BASIC 0x00000480 >> +#define VMX_BASIC_VMCS_SIZE_MASK (0x1fffULL << 32) >> +#define VMX_BASIC_32BIT_ADDRESSES (1ULL << 48) >> + >> #define MSR_EFER 0xc0000080 /* Extended Feature register. */ >> #define _EFER_SCE 0 /* SYSCALL Enable. */ >> #define EFER_SCE (_AC(1, L) << _EFER_SCE) >> diff --git a/tests/vvmx/msr.c b/tests/vvmx/msr.c >> index 100491d..ad01f26 100644 >> --- a/tests/vvmx/msr.c >> +++ b/tests/vvmx/msr.c >> @@ -48,11 +48,58 @@ static bool test_msr_feature_control(void) >> return passed; >> } >> >> +static bool test_msr_vmx_basic(void) >> +{ >> + bool passed = true; >> + uint64_t vmx_basic; >> + uint64_t vmcs_size; >> + >> + if ( rdmsr_safe(MSR_IA32_VMX_BASIC, &vmx_basic) ) >> + { >> + xtf_failure("Fail: fault when rdmsr MSR_IA32_VMX_BASIC\n"); >> + passed = false; >> + goto out; >> + } >> + >> + if ( vmx_basic & (1ULL << 31) ) >> + { >> + xtf_failure("Fail: MSR_IA32_VMX_BASIC[31] is not 0\n"); > >Out of interest, what is the reason for requiring this bit to be 0? I >can see that the manual insists that it is, but not why. > I'm not sure the exact reason, but guess it's because bit 31 of VMCS revision id is reserved to indicate whether it's a shadow one. >> + passed = false; >> + } >> + >> + vmcs_size = (vmx_basic & VMX_BASIC_VMCS_SIZE_MASK) >> 32; >> + if ( vmcs_size > PAGE_SIZE ) >> + { >> + xtf_failure("Fail: " >> + "VMCS size (%"PRIu64") in MSR_IA32_VMX_BASIC is > %ld\n", >> + vmcs_size, PAGE_SIZE); >> + passed = false; >> + } >> + else if ( vmcs_size == 0 ) >> + { >> + xtf_failure("Fail: VMCS size in MSR_IA32_VMX_BASIC cannot be 0\n"); >> + passed = false; >> + } >> + >> + /* test is running on hvm64, so this bit should be 0 */ >> + if ( vmx_basic & VMX_BASIC_32BIT_ADDRESSES ) > >There is nothing in principle wrong with Xen setting this bit. It would >be odd certainly, but not erroneous. > The reason I added this test is because Intel SDM Vol 3, Appendix A.1 Basic VMX Information says "This bit (bit 48) is always 0 for processors that support Intel 64 architecture." I don't know whether there is SW in real world that depends on the consistency between this bit and Intel SDM, but if there is any, it might be confused by an odd configuration. So I think it's still worth to keep such test. Thanks, Haozhong >~Andrew > >> + { >> + xtf_failure("Fail: MSR_IA32_VMX_BASIC[48] is not 0\n"); >> + passed = false; >> + } >> + >> +out: >> + return passed; >> +} >> + >> bool test_msr_vmx(void) >> { >> if ( !test_msr_feature_control() ) >> return false; >> >> + if ( !test_msr_vmx_basic() ) >> + return false; >> + >> return true; >> } >> >
On 19/12/16 02:44, Haozhong Zhang wrote: >>> + passed = false; >>> + } >>> + >>> + vmcs_size = (vmx_basic & VMX_BASIC_VMCS_SIZE_MASK) >> 32; >>> + if ( vmcs_size > PAGE_SIZE ) >>> + { >>> + xtf_failure("Fail: " >>> + "VMCS size (%"PRIu64") in MSR_IA32_VMX_BASIC is >>> > %ld\n", >>> + vmcs_size, PAGE_SIZE); >>> + passed = false; >>> + } >>> + else if ( vmcs_size == 0 ) >>> + { >>> + xtf_failure("Fail: VMCS size in MSR_IA32_VMX_BASIC cannot >>> be 0\n"); >>> + passed = false; >>> + } >>> + >>> + /* test is running on hvm64, so this bit should be 0 */ >>> + if ( vmx_basic & VMX_BASIC_32BIT_ADDRESSES ) >> >> There is nothing in principle wrong with Xen setting this bit. It would >> be odd certainly, but not erroneous. >> > > The reason I added this test is because Intel SDM Vol 3, Appendix A.1 > Basic VMX Information says "This bit (bit 48) is always 0 for > processors that support Intel 64 architecture." Right, and Xen, being 64bit only, can expect to find this bit always clear. > > I don't know whether there is SW in real world that depends on the > consistency between this bit and Intel SDM, but if there is any, it > might be confused by an odd configuration. So I think it's still worth > to keep such test. It is a valid administrator choice to restrict a VM to 32bit-only by hiding the long mode bit. Under those circumstances, it would be logical to expect this bit to be set in the virtual environment, despite not being required at the physical level. ~Andrew
On 12/19/16 15:41 +0000, Andrew Cooper wrote: >On 19/12/16 02:44, Haozhong Zhang wrote: >>>> + passed = false; >>>> + } >>>> + >>>> + vmcs_size = (vmx_basic & VMX_BASIC_VMCS_SIZE_MASK) >> 32; >>>> + if ( vmcs_size > PAGE_SIZE ) >>>> + { >>>> + xtf_failure("Fail: " >>>> + "VMCS size (%"PRIu64") in MSR_IA32_VMX_BASIC is >>>> > %ld\n", >>>> + vmcs_size, PAGE_SIZE); >>>> + passed = false; >>>> + } >>>> + else if ( vmcs_size == 0 ) >>>> + { >>>> + xtf_failure("Fail: VMCS size in MSR_IA32_VMX_BASIC cannot >>>> be 0\n"); >>>> + passed = false; >>>> + } >>>> + >>>> + /* test is running on hvm64, so this bit should be 0 */ >>>> + if ( vmx_basic & VMX_BASIC_32BIT_ADDRESSES ) >>> >>> There is nothing in principle wrong with Xen setting this bit. It would >>> be odd certainly, but not erroneous. >>> >> >> The reason I added this test is because Intel SDM Vol 3, Appendix A.1 >> Basic VMX Information says "This bit (bit 48) is always 0 for >> processors that support Intel 64 architecture." > >Right, and Xen, being 64bit only, can expect to find this bit always clear. > >> >> I don't know whether there is SW in real world that depends on the >> consistency between this bit and Intel SDM, but if there is any, it >> might be confused by an odd configuration. So I think it's still worth >> to keep such test. > >It is a valid administrator choice to restrict a VM to 32bit-only by >hiding the long mode bit. Under those circumstances, it would be >logical to expect this bit to be set in the virtual environment, despite >not being required at the physical level. > If we want to limit the L1 hypervisor to only use 32-bit VMCS/VMXON address, I think the right way is to limit the L1 vcpu's physical address width to 32, instead of introducing spec inconsistency, though it also limits the physical address space that can be used by L1 hypervisor. Thanks, Haozhong
diff --git a/include/arch/x86/msr-index.h b/include/arch/x86/msr-index.h index f9867d5..b7aeef0 100644 --- a/include/arch/x86/msr-index.h +++ b/include/arch/x86/msr-index.h @@ -16,6 +16,10 @@ #define _MSR_MISC_FEATURES_CPUID_FAULTING 0 #define MSR_MISC_FEATURES_CPUID_FAULTING (1ULL << _MSR_MISC_FEATURES_CPUID_FAULTING) +#define MSR_IA32_VMX_BASIC 0x00000480 +#define VMX_BASIC_VMCS_SIZE_MASK (0x1fffULL << 32) +#define VMX_BASIC_32BIT_ADDRESSES (1ULL << 48) + #define MSR_EFER 0xc0000080 /* Extended Feature register. */ #define _EFER_SCE 0 /* SYSCALL Enable. */ #define EFER_SCE (_AC(1, L) << _EFER_SCE) diff --git a/tests/vvmx/msr.c b/tests/vvmx/msr.c index 100491d..ad01f26 100644 --- a/tests/vvmx/msr.c +++ b/tests/vvmx/msr.c @@ -48,11 +48,58 @@ static bool test_msr_feature_control(void) return passed; } +static bool test_msr_vmx_basic(void) +{ + bool passed = true; + uint64_t vmx_basic; + uint64_t vmcs_size; + + if ( rdmsr_safe(MSR_IA32_VMX_BASIC, &vmx_basic) ) + { + xtf_failure("Fail: fault when rdmsr MSR_IA32_VMX_BASIC\n"); + passed = false; + goto out; + } + + if ( vmx_basic & (1ULL << 31) ) + { + xtf_failure("Fail: MSR_IA32_VMX_BASIC[31] is not 0\n"); + passed = false; + } + + vmcs_size = (vmx_basic & VMX_BASIC_VMCS_SIZE_MASK) >> 32; + if ( vmcs_size > PAGE_SIZE ) + { + xtf_failure("Fail: " + "VMCS size (%"PRIu64") in MSR_IA32_VMX_BASIC is > %ld\n", + vmcs_size, PAGE_SIZE); + passed = false; + } + else if ( vmcs_size == 0 ) + { + xtf_failure("Fail: VMCS size in MSR_IA32_VMX_BASIC cannot be 0\n"); + passed = false; + } + + /* test is running on hvm64, so this bit should be 0 */ + if ( vmx_basic & VMX_BASIC_32BIT_ADDRESSES ) + { + xtf_failure("Fail: MSR_IA32_VMX_BASIC[48] is not 0\n"); + passed = false; + } + +out: + return passed; +} + bool test_msr_vmx(void) { if ( !test_msr_feature_control() ) return false; + if ( !test_msr_vmx_basic() ) + return false; + return true; }
It tests whether bit 31 and bit 48 are 0, and VMCS size is in the range (0, 4096]. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- include/arch/x86/msr-index.h | 4 ++++ tests/vvmx/msr.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+)