From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: Addback capability check for non-initial features Date: Fri, 10 Jun 2011 07:05:34 +0100 Message-ID: References: <1A42CE6F5F474C41B63392A5F80372B256260B2A@shsmsx501.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1A42CE6F5F474C41B63392A5F80372B256260B2A@shsmsx501.ccr.corp.intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "Dong, Eddie" Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 10/06/2011 06:50, "Dong, Eddie" 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 > > 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 > )