* [PATCH 0/3] Nested VMX: fix bugs during reading VMX MSRs.
@ 2013-09-05 2:57 Yang Zhang
2013-09-05 2:57 ` [PATCH 1/3] Nested VMX: Check VMX capability before read VMX related MSRs Yang Zhang
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Yang Zhang @ 2013-09-05 2:57 UTC (permalink / raw)
To: xen-devel; +Cc: Yang Zhang, eddie.dong, jun.nakajima, JBeulich
From: Yang Zhang <yang.z.zhang@Intel.com>
The following patches fix the issues which existing in current vmx msr reading logic.
Yang Zhang (3):
Nested VMX: Check VMX capability before read VMX related MSRs.
Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR
Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation
xen/arch/x86/hvm/vmx/vvmx.c | 54 ++++++++++++++++++++++++++++++++++++++++--
1 files changed, 51 insertions(+), 3 deletions(-)
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] Nested VMX: Check VMX capability before read VMX related MSRs. 2013-09-05 2:57 [PATCH 0/3] Nested VMX: fix bugs during reading VMX MSRs Yang Zhang @ 2013-09-05 2:57 ` Yang Zhang 2013-09-05 8:35 ` Jan Beulich 2013-09-05 9:49 ` Andrew Cooper 2013-09-05 2:57 ` [PATCH 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR Yang Zhang 2013-09-05 2:57 ` [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation Yang Zhang 2 siblings, 2 replies; 12+ messages in thread From: Yang Zhang @ 2013-09-05 2:57 UTC (permalink / raw) To: xen-devel; +Cc: Yang Zhang, eddie.dong, jun.nakajima, JBeulich From: Yang Zhang <yang.z.zhang@Intel.com> VMX MSRs only available when the CPU support the VMX feature. In addition, VMX_TRUE* MSRs only available when bitt 55 of VMX_BASIC MSR is set. Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> --- xen/arch/x86/hvm/vmx/vvmx.c | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-) diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index cecc72f..2e0b7f7 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1820,6 +1820,24 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) if ( !nestedhvm_enabled(v->domain) ) return 0; + /* + * VMX capablitys MSRs available only when guest + * support VMX. + */ + hvm_cpuid(0x1, &eax, &ebx, &ecx, &edx); + if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) ) + return 0; + + /* + * Those MSRs available only when bit 55 of + * MSR_IA32_VMX_BASIC is set. + */ + rdmsrl(MSR_IA32_VMX_BASIC, data); + if ( msr >= MSR_IA32_VMX_TRUE_PINBASED_CTLS && + msr <= MSR_IA32_VMX_TRUE_ENTRY_CTLS && + !(data & VMX_BASIC_DEFAULT1_ZERO) ) + return 0; + rdmsrl(msr, host_data); /* -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] Nested VMX: Check VMX capability before read VMX related MSRs. 2013-09-05 2:57 ` [PATCH 1/3] Nested VMX: Check VMX capability before read VMX related MSRs Yang Zhang @ 2013-09-05 8:35 ` Jan Beulich 2013-09-05 9:49 ` Andrew Cooper 1 sibling, 0 replies; 12+ messages in thread From: Jan Beulich @ 2013-09-05 8:35 UTC (permalink / raw) To: Yang Zhang; +Cc: xen-devel, eddie.dong, jun.nakajima >>> On 05.09.13 at 04:57, Yang Zhang <yang.z.zhang@intel.com> wrote: > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1820,6 +1820,24 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 > *msr_content) > if ( !nestedhvm_enabled(v->domain) ) > return 0; > > + /* > + * VMX capablitys MSRs available only when guest > + * support VMX. > + */ > + hvm_cpuid(0x1, &eax, &ebx, &ecx, &edx); > + if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) ) > + return 0; > + > + /* > + * Those MSRs available only when bit 55 of > + * MSR_IA32_VMX_BASIC is set. > + */ > + rdmsrl(MSR_IA32_VMX_BASIC, data); > + if ( msr >= MSR_IA32_VMX_TRUE_PINBASED_CTLS && > + msr <= MSR_IA32_VMX_TRUE_ENTRY_CTLS && > + !(data & VMX_BASIC_DEFAULT1_ZERO) ) Indentation. Jan > + return 0; > + > rdmsrl(msr, host_data); > > /* ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] Nested VMX: Check VMX capability before read VMX related MSRs. 2013-09-05 2:57 ` [PATCH 1/3] Nested VMX: Check VMX capability before read VMX related MSRs Yang Zhang 2013-09-05 8:35 ` Jan Beulich @ 2013-09-05 9:49 ` Andrew Cooper 1 sibling, 0 replies; 12+ messages in thread From: Andrew Cooper @ 2013-09-05 9:49 UTC (permalink / raw) To: Yang Zhang; +Cc: xen-devel, eddie.dong, JBeulich, jun.nakajima On 05/09/13 03:57, Yang Zhang wrote: > From: Yang Zhang <yang.z.zhang@Intel.com> > > VMX MSRs only available when the CPU support the VMX feature. In addition, > VMX_TRUE* MSRs only available when bitt 55 of VMX_BASIC MSR is set. As you already have to respin the patch, "bit" rather than "bitt" ~Andrew > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > --- > xen/arch/x86/hvm/vmx/vvmx.c | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index cecc72f..2e0b7f7 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1820,6 +1820,24 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) > if ( !nestedhvm_enabled(v->domain) ) > return 0; > > + /* > + * VMX capablitys MSRs available only when guest > + * support VMX. > + */ > + hvm_cpuid(0x1, &eax, &ebx, &ecx, &edx); > + if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) ) > + return 0; > + > + /* > + * Those MSRs available only when bit 55 of > + * MSR_IA32_VMX_BASIC is set. > + */ > + rdmsrl(MSR_IA32_VMX_BASIC, data); > + if ( msr >= MSR_IA32_VMX_TRUE_PINBASED_CTLS && > + msr <= MSR_IA32_VMX_TRUE_ENTRY_CTLS && > + !(data & VMX_BASIC_DEFAULT1_ZERO) ) > + return 0; > + > rdmsrl(msr, host_data); > > /* ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR 2013-09-05 2:57 [PATCH 0/3] Nested VMX: fix bugs during reading VMX MSRs Yang Zhang 2013-09-05 2:57 ` [PATCH 1/3] Nested VMX: Check VMX capability before read VMX related MSRs Yang Zhang @ 2013-09-05 2:57 ` Yang Zhang 2013-09-05 10:12 ` Andrew Cooper 2013-09-05 2:57 ` [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation Yang Zhang 2 siblings, 1 reply; 12+ messages in thread From: Yang Zhang @ 2013-09-05 2:57 UTC (permalink / raw) To: xen-devel; +Cc: Yang Zhang, eddie.dong, jun.nakajima, JBeulich From: Yang Zhang <yang.z.zhang@Intel.com> The bit 31 of revision_id will set to 1 if vmcs shadowing enabled. And according intel SDM, the bit 31 of IA32_VMX_BASIC MSR is always 0. So we cannot set low 32 bit of IA32_VMX_BASIC to revision_id directly. Must clear the bit 31 to 0. Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> --- xen/arch/x86/hvm/vmx/vvmx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index 2e0b7f7..8571002 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1846,7 +1846,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) switch (msr) { case MSR_IA32_VMX_BASIC: data = (host_data & (~0ul << 32)) | - ((v->arch.hvm_vmx.vmcs)->vmcs_revision_id); + ((v->arch.hvm_vmx.vmcs)->vmcs_revision_id & ~(1ul << 31)); break; case MSR_IA32_VMX_PINBASED_CTLS: case MSR_IA32_VMX_TRUE_PINBASED_CTLS: -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR 2013-09-05 2:57 ` [PATCH 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR Yang Zhang @ 2013-09-05 10:12 ` Andrew Cooper 2013-09-05 10:16 ` Zhang, Yang Z 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2013-09-05 10:12 UTC (permalink / raw) To: Yang Zhang; +Cc: xen-devel, eddie.dong, JBeulich, jun.nakajima On 05/09/13 03:57, Yang Zhang wrote: > From: Yang Zhang <yang.z.zhang@Intel.com> > > The bit 31 of revision_id will set to 1 if vmcs shadowing enabled. And > according intel SDM, the bit 31 of IA32_VMX_BASIC MSR is always 0. So we > cannot set low 32 bit of IA32_VMX_BASIC to revision_id directly. Must clear > the bit 31 to 0. > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > --- > xen/arch/x86/hvm/vmx/vvmx.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index 2e0b7f7..8571002 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1846,7 +1846,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) > switch (msr) { > case MSR_IA32_VMX_BASIC: > data = (host_data & (~0ul << 32)) | > - ((v->arch.hvm_vmx.vmcs)->vmcs_revision_id); > + ((v->arch.hvm_vmx.vmcs)->vmcs_revision_id & ~(1ul << 31)); What are the chances of vmcs_revision_id extending beyond 32 bits? The SDM states that the bottom 31 bits of IA32_VMX_BASIC shall be the bottom 31 bits of the revision id, so (v->arch.hvm_vmx.vmcs->vmcs_revision_id & 0x7fffffff); would seem more obvious. Also, the brackets were superfluous. ~Andrew > break; > case MSR_IA32_VMX_PINBASED_CTLS: > case MSR_IA32_VMX_TRUE_PINBASED_CTLS: ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR 2013-09-05 10:12 ` Andrew Cooper @ 2013-09-05 10:16 ` Zhang, Yang Z 0 siblings, 0 replies; 12+ messages in thread From: Zhang, Yang Z @ 2013-09-05 10:16 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel@lists.xensource.com, Dong, Eddie, JBeulich@suse.com, Nakajima, Jun Andrew Cooper wrote on 2013-09-05: > On 05/09/13 03:57, Yang Zhang wrote: >> From: Yang Zhang <yang.z.zhang@Intel.com> >> >> The bit 31 of revision_id will set to 1 if vmcs shadowing enabled. >> And according intel SDM, the bit 31 of IA32_VMX_BASIC MSR is always >> 0. So we cannot set low 32 bit of IA32_VMX_BASIC to revision_id directly. >> Must clear the bit 31 to 0. >> >> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> >> --- >> xen/arch/x86/hvm/vmx/vvmx.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c >> b/xen/arch/x86/hvm/vmx/vvmx.c index 2e0b7f7..8571002 100644 >> --- a/xen/arch/x86/hvm/vmx/vvmx.c >> +++ b/xen/arch/x86/hvm/vmx/vvmx.c >> @@ -1846,7 +1846,7 @@ int nvmx_msr_read_intercept(unsigned int msr, > u64 *msr_content) >> switch (msr) { >> case MSR_IA32_VMX_BASIC: >> data = (host_data & (~0ul << 32)) | >> - ((v->arch.hvm_vmx.vmcs)->vmcs_revision_id); >> + ((v->arch.hvm_vmx.vmcs)->vmcs_revision_id & ~(1ul << >> + 31)); > > What are the chances of vmcs_revision_id extending beyond 32 bits? > > The SDM states that the bottom 31 bits of IA32_VMX_BASIC shall be the > bottom 31 bits of the revision id, so > > (v->arch.hvm_vmx.vmcs->vmcs_revision_id & 0x7fffffff); > > would seem more obvious. Also, the brackets were superfluous. Right! > > ~Andrew > >> break; >> case MSR_IA32_VMX_PINBASED_CTLS: >> case MSR_IA32_VMX_TRUE_PINBASED_CTLS: Best regards, Yang ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation 2013-09-05 2:57 [PATCH 0/3] Nested VMX: fix bugs during reading VMX MSRs Yang Zhang 2013-09-05 2:57 ` [PATCH 1/3] Nested VMX: Check VMX capability before read VMX related MSRs Yang Zhang 2013-09-05 2:57 ` [PATCH 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR Yang Zhang @ 2013-09-05 2:57 ` Yang Zhang 2013-09-05 8:46 ` Jan Beulich 2 siblings, 1 reply; 12+ messages in thread From: Yang Zhang @ 2013-09-05 2:57 UTC (permalink / raw) To: xen-devel; +Cc: Yang Zhang, eddie.dong, jun.nakajima, JBeulich From: Yang Zhang <yang.z.zhang@Intel.com> Currently, it use hardcode value for IA32_VMX_CR4_FIXED1. This is wrong. We should check guest's cpuid to know which bits are writeable in CR4 by guest and allow the guest to set the corresponding bit only when guest has the feature. Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> --- xen/arch/x86/hvm/vmx/vvmx.c | 34 ++++++++++++++++++++++++++++++++-- 1 files changed, 32 insertions(+), 2 deletions(-) diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index 8571002..8e53beb 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1815,6 +1815,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) { struct vcpu *v = current; u64 data = 0, host_data = 0; + unsigned int eax, ebx, ecx, edx; int r = 1; if ( !nestedhvm_enabled(v->domain) ) @@ -1943,8 +1944,37 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) data = X86_CR4_VMXE; break; case MSR_IA32_VMX_CR4_FIXED1: - /* allow 0-settings except SMXE */ - data = 0x267ff & ~X86_CR4_SMXE; + data |= (edx & cpufeat_mask(X86_FEATURE_VME) ? + (X86_CR4_VME | X86_CR4_PVI) : 0) | + (edx & cpufeat_mask(X86_FEATURE_TSC) ? X86_CR4_TSD : 0) | + (edx & cpufeat_mask(X86_FEATURE_DE) ? X86_CR4_DE : 0) | + (edx & cpufeat_mask(X86_FEATURE_PSE) ? X86_CR4_PSE : 0) | + (edx & cpufeat_mask(X86_FEATURE_PAE) ? X86_CR4_PAE : 0) | + (edx & cpufeat_mask(X86_FEATURE_MCE) ? X86_CR4_MCE : 0) | + (edx & cpufeat_mask(X86_FEATURE_PGE) ? X86_CR4_PGE : 0) | + (edx & cpufeat_mask(X86_FEATURE_FXSR) ? X86_CR4_OSFXSR : 0) | + (edx & cpufeat_mask(X86_FEATURE_XMM) ? X86_CR4_OSXMMEXCPT : 0) | + (ecx & cpufeat_mask(X86_FEATURE_VMXE) ? X86_CR4_VMXE : 0) | + (ecx & cpufeat_mask(X86_FEATURE_SMXE) ? X86_CR4_SMXE : 0) | + (ecx & cpufeat_mask(X86_FEATURE_PCID) ? X86_CR4_PCIDE : 0) | + (ecx & cpufeat_mask(X86_FEATURE_XSAVE) ? X86_CR4_OSXSAVE : 0); + + hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx); + if ( eax >= 0xa ) + { + unsigned int temp_eax; + + hvm_cpuid(0xa, &temp_eax, &ebx, &ecx, &edx); + /* Check whether guest has the perf monitor feature. */ + if ( (temp_eax & 0xff) && (temp_eax & 0xff00) ) + data |= X86_CR4_PCE; + } else if ( eax >= 0x7 ) + { + hvm_cpuid(0x7, &eax, &ebx, &ecx, &edx); + data |= (ebx & cpufeat_mask(X86_FEATURE_SMEP) ? X86_CR4_SMEP : 0) | + (ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) ? + X86_CR4_FSGSBASE : 0); + } break; case MSR_IA32_VMX_MISC: /* Do not support CR3-target feature now */ -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation 2013-09-05 2:57 ` [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation Yang Zhang @ 2013-09-05 8:46 ` Jan Beulich 2013-09-05 9:05 ` Zhang, Yang Z 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2013-09-05 8:46 UTC (permalink / raw) To: Yang Zhang; +Cc: xen-devel, eddie.dong, jun.nakajima >>> On 05.09.13 at 04:57, Yang Zhang <yang.z.zhang@intel.com> wrote: > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1815,6 +1815,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) > { > struct vcpu *v = current; > u64 data = 0, host_data = 0; > + unsigned int eax, ebx, ecx, edx; > int r = 1; > > if ( !nestedhvm_enabled(v->domain) ) > @@ -1943,8 +1944,37 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) > data = X86_CR4_VMXE; > break; > case MSR_IA32_VMX_CR4_FIXED1: > - /* allow 0-settings except SMXE */ > - data = 0x267ff & ~X86_CR4_SMXE; > + data |= (edx & cpufeat_mask(X86_FEATURE_VME) ? Did you perhaps send a stale patch? I can't see how this would even have compiled: edx is uninitialized at this point afaict. > + (X86_CR4_VME | X86_CR4_PVI) : 0) | > + (edx & cpufeat_mask(X86_FEATURE_TSC) ? X86_CR4_TSD : 0) | > + (edx & cpufeat_mask(X86_FEATURE_DE) ? X86_CR4_DE : 0) | > + (edx & cpufeat_mask(X86_FEATURE_PSE) ? X86_CR4_PSE : 0) | > + (edx & cpufeat_mask(X86_FEATURE_PAE) ? X86_CR4_PAE : 0) | > + (edx & cpufeat_mask(X86_FEATURE_MCE) ? X86_CR4_MCE : 0) | > + (edx & cpufeat_mask(X86_FEATURE_PGE) ? X86_CR4_PGE : 0) | > + (edx & cpufeat_mask(X86_FEATURE_FXSR) ? X86_CR4_OSFXSR : 0) | > + (edx & cpufeat_mask(X86_FEATURE_XMM) ? X86_CR4_OSXMMEXCPT : 0) | > + (ecx & cpufeat_mask(X86_FEATURE_VMXE) ? X86_CR4_VMXE : 0) | > + (ecx & cpufeat_mask(X86_FEATURE_SMXE) ? X86_CR4_SMXE : 0) | > + (ecx & cpufeat_mask(X86_FEATURE_PCID) ? X86_CR4_PCIDE : 0) | > + (ecx & cpufeat_mask(X86_FEATURE_XSAVE) ? X86_CR4_OSXSAVE : 0); I think this would be more legible if you used a series of "if() data |=". Or at least suitably pad the lines so the similar parts align nicely. > + > + hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx); > + if ( eax >= 0xa ) > + { > + unsigned int temp_eax; Why is this needed? You don't need eax anymore below. > + > + hvm_cpuid(0xa, &temp_eax, &ebx, &ecx, &edx); > + /* Check whether guest has the perf monitor feature. */ > + if ( (temp_eax & 0xff) && (temp_eax & 0xff00) ) > + data |= X86_CR4_PCE; > + } else if ( eax >= 0x7 ) Coding style. Also, is this really "else if"? If not, _that_ would explain the (apparent; can be avoided nevertheless) need for temp_eax above... > + { > + hvm_cpuid(0x7, &eax, &ebx, &ecx, &edx); > + data |= (ebx & cpufeat_mask(X86_FEATURE_SMEP) ? X86_CR4_SMEP : 0) | > + (ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) ? > + X86_CR4_FSGSBASE : 0); Same as above. I'd also like to see SMAP added here right away, even if for now hvm_cpuid() [hopefully] always returns the respective bit clear. Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation 2013-09-05 8:46 ` Jan Beulich @ 2013-09-05 9:05 ` Zhang, Yang Z 2013-09-05 9:36 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Zhang, Yang Z @ 2013-09-05 9:05 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Dong, Eddie, Nakajima, Jun Jan Beulich wrote on 2013-09-05: >>>> On 05.09.13 at 04:57, Yang Zhang <yang.z.zhang@intel.com> wrote: >> --- a/xen/arch/x86/hvm/vmx/vvmx.c >> +++ b/xen/arch/x86/hvm/vmx/vvmx.c >> @@ -1815,6 +1815,7 @@ int nvmx_msr_read_intercept(unsigned int msr, >> u64 *msr_content) { >> struct vcpu *v = current; u64 data = 0, host_data = 0; + >> unsigned int eax, ebx, ecx, edx; int r = 1; >> >> if ( !nestedhvm_enabled(v->domain) ) @@ -1943,8 +1944,37 @@ int >> nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) >> data = X86_CR4_VMXE; >> break; >> case MSR_IA32_VMX_CR4_FIXED1: >> - /* allow 0-settings except SMXE */ >> - data = 0x267ff & ~X86_CR4_SMXE; >> + data |= (edx & cpufeat_mask(X86_FEATURE_VME) ? > > Did you perhaps send a stale patch? I can't see how this would even have > compiled: edx is uninitialized at this point afaict. It is already filled. See the first patch. > >> + (X86_CR4_VME | X86_CR4_PVI) : >> 0) | + (edx & cpufeat_mask(X86_FEATURE_TSC) ? >> X86_CR4_TSD : 0) | + (edx & cpufeat_mask(X86_FEATURE_DE) >> ? X86_CR4_DE : 0) | + (edx & >> cpufeat_mask(X86_FEATURE_PSE) ? X86_CR4_PSE : 0) | + >> (edx & cpufeat_mask(X86_FEATURE_PAE) ? X86_CR4_PAE : 0) | + >> (edx & cpufeat_mask(X86_FEATURE_MCE) ? X86_CR4_MCE : 0) | + >> (edx & cpufeat_mask(X86_FEATURE_PGE) ? X86_CR4_PGE : 0) | + >> (edx & cpufeat_mask(X86_FEATURE_FXSR) ? X86_CR4_OSFXSR : 0) | >> + (edx & cpufeat_mask(X86_FEATURE_XMM) ? >> X86_CR4_OSXMMEXCPT : 0) | + (ecx & >> cpufeat_mask(X86_FEATURE_VMXE) ? X86_CR4_VMXE : 0) | + >> (ecx & cpufeat_mask(X86_FEATURE_SMXE) ? X86_CR4_SMXE : 0) | + >> (ecx & cpufeat_mask(X86_FEATURE_PCID) ? X86_CR4_PCIDE : 0) | + >> (ecx & cpufeat_mask(X86_FEATURE_XSAVE) ? + X86_CR4_OSXSAVE : >> 0); > > I think this would be more legible if you used a series of "if() data |=". > Or at least suitably pad the lines so the similar parts align nicely. if() should be better. Since the line will exceed the 80 characters if using pad. > >> + >> + hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx); >> + if ( eax >= 0xa ) >> + { >> + unsigned int temp_eax; > > Why is this needed? You don't need eax anymore below. Good point! > >> + >> + hvm_cpuid(0xa, &temp_eax, &ebx, &ecx, &edx); >> + /* Check whether guest has the perf monitor feature. */ >> + if ( (temp_eax & 0xff) && (temp_eax & 0xff00) ) >> + data |= X86_CR4_PCE; >> + } else if ( eax >= 0x7 ) > > Coding style. Also, is this really "else if"? If not, _that_ would > explain the (apparent; can be avoided nevertheless) need for temp_eax above... The different coding style between upstream Linux and Xen really defeats me. :( > >> + { + hvm_cpuid(0x7, &eax, &ebx, &ecx, &edx); + >> data |= (ebx & cpufeat_mask(X86_FEATURE_SMEP) ? X86_CR4_SMEP : 0) >> | + (ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) ? + > X86_CR4_FSGSBASE : 0); > > Same as above. I'd also like to see SMAP added here right away, even > if for now > hvm_cpuid() [hopefully] always returns the respective bit clear. Sure. > > Jan Best regards, Yang ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation 2013-09-05 9:05 ` Zhang, Yang Z @ 2013-09-05 9:36 ` Jan Beulich 2013-09-05 10:28 ` Zhang, Yang Z 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2013-09-05 9:36 UTC (permalink / raw) To: Yang Z Zhang; +Cc: xen-devel, Eddie Dong, Jun Nakajima >>> On 05.09.13 at 11:05, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: > Jan Beulich wrote on 2013-09-05: >>>>> On 05.09.13 at 04:57, Yang Zhang <yang.z.zhang@intel.com> wrote: >>> case MSR_IA32_VMX_CR4_FIXED1: >>> - /* allow 0-settings except SMXE */ >>> - data = 0x267ff & ~X86_CR4_SMXE; >>> + data |= (edx & cpufeat_mask(X86_FEATURE_VME) ? >> >> Did you perhaps send a stale patch? I can't see how this would even have >> compiled: edx is uninitialized at this point afaict. > > It is already filled. See the first patch. Then the first patch fails to declare all four variables, and hence won't build. It just caught my eye that you declare the variables here and don't initialize them between declaration and use. Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation 2013-09-05 9:36 ` Jan Beulich @ 2013-09-05 10:28 ` Zhang, Yang Z 0 siblings, 0 replies; 12+ messages in thread From: Zhang, Yang Z @ 2013-09-05 10:28 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Dong, Eddie, Nakajima, Jun Jan Beulich wrote on 2013-09-05: >>>> On 05.09.13 at 11:05, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: >> Jan Beulich wrote on 2013-09-05: >>>>>> On 05.09.13 at 04:57, Yang Zhang <yang.z.zhang@intel.com> wrote: >>>> case MSR_IA32_VMX_CR4_FIXED1: >>>> - /* allow 0-settings except SMXE */ >>>> - data = 0x267ff & ~X86_CR4_SMXE; >>>> + data |= (edx & cpufeat_mask(X86_FEATURE_VME) ? >>> >>> Did you perhaps send a stale patch? I can't see how this would even >>> have >>> compiled: edx is uninitialized at this point afaict. >> >> It is already filled. See the first patch. > > Then the first patch fails to declare all four variables, and hence > won't build. It just caught my eye that you declare the variables here > and don't initialize them between declaration and use. You are right. I adjust the order of patches but forget tweaking the code accordingly. Best regards, Yang ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-09-05 10:29 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-05 2:57 [PATCH 0/3] Nested VMX: fix bugs during reading VMX MSRs Yang Zhang 2013-09-05 2:57 ` [PATCH 1/3] Nested VMX: Check VMX capability before read VMX related MSRs Yang Zhang 2013-09-05 8:35 ` Jan Beulich 2013-09-05 9:49 ` Andrew Cooper 2013-09-05 2:57 ` [PATCH 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR Yang Zhang 2013-09-05 10:12 ` Andrew Cooper 2013-09-05 10:16 ` Zhang, Yang Z 2013-09-05 2:57 ` [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation Yang Zhang 2013-09-05 8:46 ` Jan Beulich 2013-09-05 9:05 ` Zhang, Yang Z 2013-09-05 9:36 ` Jan Beulich 2013-09-05 10:28 ` Zhang, Yang Z
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.