Re: Addback capability check for non-initial features

[Keir Fraser <keir.xen@gmail.com>]

On 10/06/2011 06:50, "Dong, Eddie" <eddie.dong@intel.com> wrote:

> 
> add back missing capability check of MSR_IA32_VMX_PROCBASED_CTLS.
> 
> Besides initial configuration, adjust_vmx_controls is responsible for
> hardware capibility check as well. This patch add back the check.

I suppose the CPU_BASED_VIRTUAL_INTR_PENDING addition is correct, for what
it's worth (surely every VMX-capable CPU ever has and will support that).

The change to CR8 detection looks mad and incorrect. You've inverted it so
that CR8 exits get enabled when TPR_SHADOW is available, rather than when it
isn't, surely? And that can't be correct. I don't see how the CR8-exit
detection and enabling is wrong, as it is already.

 -- Keir

> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
> 
> diff -r 43a06a43e60b xen/arch/x86/hvm/vmx/vmcs.c
> --- a/xen/arch/x86/hvm/vmx/vmcs.c Thu Jun 09 16:30:34 2011 +0800
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c Fri Jun 10 13:28:49 2011 +0800
> @@ -148,6 +148,11 @@ static int vmx_init_vmcs_config(void)
>          MSR_IA32_VMX_PINBASED_CTLS, &mismatch);
>  
>      min = (CPU_BASED_HLT_EXITING |
> +           CPU_BASED_VIRTUAL_INTR_PENDING |
> +#ifdef __x86_64__
> +           CPU_BASED_CR8_LOAD_EXITING |
> +           CPU_BASED_CR8_STORE_EXITING |
> +#endif
>             CPU_BASED_INVLPG_EXITING |
>             CPU_BASED_CR3_LOAD_EXITING |
>             CPU_BASED_CR3_STORE_EXITING |
> @@ -164,15 +169,12 @@ static int vmx_init_vmcs_config(void)
>      _vmx_cpu_based_exec_control = adjust_vmx_controls(
>          "CPU-Based Exec Control", min, opt,
>          MSR_IA32_VMX_PROCBASED_CTLS, &mismatch);
> -    _vmx_cpu_based_exec_control &= ~CPU_BASED_RDTSC_EXITING;
> +    _vmx_cpu_based_exec_control &= ~(CPU_BASED_RDTSC_EXITING |
> +                                     CPU_BASED_VIRTUAL_INTR_PENDING);
>  #ifdef __x86_64__
>      if ( !(_vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW) )
> -    {
> -        min |= CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING;
> -        _vmx_cpu_based_exec_control = adjust_vmx_controls(
> -            "CPU-Based Exec Control", min, opt,
> -            MSR_IA32_VMX_PROCBASED_CTLS, &mismatch);
> -    }
> +        _vmx_cpu_based_exec_control &= ~(CPU_BASED_CR8_LOAD_EXITING |
> +                                         CPU_BASED_CR8_STORE_EXITING);
>  #endif
>  
>      if ( _vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS
> )