[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
Jan Beulich wrote on 2013-09-05: >>>> 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. 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |