From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH RFC V8 3/5] xen, libxc: Force-enable relevant MSR events Date: Thu, 28 Aug 2014 10:40:29 +0300 Message-ID: <53FEDCED.1070904@bitdefender.com> References: <1409148101-11703-1-git-send-email-rcojocaru@bitdefender.com> <1409148101-11703-3-git-send-email-rcojocaru@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XMuJd-0002Yw-RV for xen-devel@lists.xenproject.org; Thu, 28 Aug 2014 07:40:18 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Tian, Kevin" , "xen-devel@lists.xenproject.org" Cc: "Dong, Eddie" , "stefano.stabellini@eu.citrix.com" , "Nakajima, Jun" , "andrew.cooper3@citrix.com" , "tim@xen.org" , "JBeulich@suse.com" , "ian.jackson@eu.citrix.com" , "ian.campbell@citrix.com" List-Id: xen-devel@lists.xenproject.org On 08/27/2014 11:25 PM, Tian, Kevin wrote: >> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com] >> Sent: Wednesday, August 27, 2014 7:02 AM >> >> Vmx_disable_intercept_for_msr() will now refuse to disable interception of >> MSRs needed for memory introspection. It is not possible to gate this on >> mem_access being active for the domain, since by the time mem_access does >> become active the interception for the interesting MSRs has already been >> disabled (vmx_disable_intercept_for_msr() runs very early on). >> >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >> index 4a4f4e1..44aca73 100644 >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -39,6 +39,7 @@ >> #include >> #include >> #include >> +#include >> >> static bool_t __read_mostly opt_vpid_enabled = 1; >> boolean_param("vpid", opt_vpid_enabled); >> @@ -71,6 +72,18 @@ u32 vmx_vmexit_control __read_mostly; >> u32 vmx_vmentry_control __read_mostly; >> u64 vmx_ept_vpid_cap __read_mostly; >> >> +const u32 vmx_msrs_exit_array[] = { >> + MSR_IA32_SYSENTER_EIP, >> + MSR_IA32_SYSENTER_ESP, >> + MSR_IA32_SYSENTER_CS, >> + MSR_IA32_MC0_CTL, >> + MSR_STAR, >> + MSR_LSTAR >> +}; >> + >> +const unsigned int vmx_msrs_exit_array_size = >> + ARRAY_SIZE(vmx_msrs_exit_array); >> + > > prefer a name including 'force' purpose or at least have a comment > to describe the intention, so its meaning is explained as early as possible. How would vmx_force_enabled_msrs_array (with a comment added) sound to you and Jan? >> /* >> * See Intel PRM Vol. 3, 20.6.9 (MSR-Bitmap Address). Early manuals >> * have the write-low and read-high bitmap offsets the wrong way >> round. >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >> index dc18a72..40e6c2c 100644 >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -1682,6 +1682,17 @@ void vmx_hypervisor_cpuid_leaf(uint32_t sub_idx, >> *eax |= XEN_HVM_CPUID_X2APIC_VIRT; >> } >> >> +static void vmx_enable_msr_exit_interception(struct domain *d) >> +{ >> + struct vcpu *v; >> + unsigned int i; >> + >> + /* Enable interception for MSRs needed for memory introspection. */ >> + for_each_vcpu ( d, v ) >> + for ( i = 0; i < vmx_msrs_exit_array_size; i++ ) >> + vmx_enable_intercept_for_msr(v, vmx_msrs_exit_array[i], >> MSR_TYPE_W); >> +} >> + > > so you need same conditional check as you added for vmx_disable_intercept_for_msr The check part is in this code: diff --git a/xen/arch/x86/mm/mem_event.c b/xen/arch/x86/mm/mem_event.c index ba7e71e..fdd5ff6 100644 --- a/xen/arch/x86/mm/mem_event.c +++ b/xen/arch/x86/mm/mem_event.c @@ -587,6 +587,7 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec, switch( mec->op ) { case XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE: + case XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE_INTROSPECTION: { rc = -ENODEV; /* Only HAP is supported */ @@ -600,13 +601,23 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec, rc = mem_event_enable(d, mec, med, _VPF_mem_access, HVM_PARAM_ACCESS_RING_PFN, mem_access_notification); + + if ( mec->op != XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE && + rc == 0 && hvm_funcs.enable_msr_exit_interception ) + { + d->arch.hvm_domain.introspection_enabled = 1; + hvm_funcs.enable_msr_exit_interception(d); + } } break; case XEN_DOMCTL_MEM_EVENT_OP_ACCESS_DISABLE: { if ( med->ring_page ) + { rc = mem_event_disable(d, med); + d->arch.hvm_domain.introspection_enabled = 0; + } } break; So if we do a domctl with XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE_INTROSPECTION, then we must make sure that those MSRs are being intercepted (so only then do we call hvm_funcs.enable_msr_exit_interception(d)). The previous check (the one you've mentioned) only makes sure that if d->arch.hvm_domain.introspection_enabled is true, interception for the interesting MSRs cannot be disabled. Thanks, Razvan Cojocaru