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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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