Re: [Xen-devel] [PATCH 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

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