[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation
>>> On 05.09.13 at 04:57, Yang Zhang <yang.z.zhang@xxxxxxxxx> 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |