From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v8 12/19] x86/VPMU: When handling MSR accesses, leave fault injection to callers Date: Mon, 28 Jul 2014 17:26:34 +0100 Message-ID: <53D695DA0200007800026DA6@mail.emea.novell.com> References: <1404225480-2664-1-git-send-email-boris.ostrovsky@oracle.com> <1404225480-2664-13-git-send-email-boris.ostrovsky@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1404225480-2664-13-git-send-email-boris.ostrovsky@oracle.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Boris Ostrovsky Cc: kevin.tian@intel.com, keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org >>> On 01.07.14 at 16:37, wrote: > @@ -2079,11 +2079,18 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content) > MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL; > /* Perhaps vpmu will change some bits. */ > if ( vpmu_do_rdmsr(msr, msr_content) ) > - goto done; > + goto gp_fault; > break; > - default: > + case MSR_P6_PERFCTR0...MSR_P6_PERFCTR1: > + case MSR_P6_EVNTSEL0...MSR_P6_EVNTSEL1: > + case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2: > + case MSR_CORE_PERF_FIXED_CTR_CTRL...MSR_CORE_PERF_GLOBAL_OVF_CTRL: > + case MSR_IA32_PEBS_ENABLE: > + case MSR_IA32_DS_AREA: > if ( vpmu_do_rdmsr(msr, msr_content) ) > - break; > + goto gp_fault; > + break; > + default: If you really want to switch to a white listing approach, I think you'd be better off having the previous case fall through into the PMU ones, thus reducing code duplication. > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c > @@ -461,13 +461,13 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content) > if ( cpu_has(¤t_cpu_data, X86_FEATURE_DSCPL) ) > supported |= IA32_DEBUGCTLMSR_BTS_OFF_OS | > IA32_DEBUGCTLMSR_BTS_OFF_USR; > - if ( msr_content & supported ) > + > + if ( !(msr_content & supported ) || > + !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) > { > - if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) > - return 1; > - gdprintk(XENLOG_WARNING, "Debug Store is not supported on this cpu\n"); > - hvm_inject_hw_exception(TRAP_gp_fault, 0); > - return 0; > + gdprintk(XENLOG_DEBUG, > + "Debug Store is not supported on this cpu\n"); > + return 1; I think these warning level adjustments should be in their own patch, with (unless there is proof of the contrary) the wording in the commit message adjusted so that one doesn't get the false impression of you wanting to slip in a security fix this way: Messages at guest level are (by default) intentionally always rate limited. Jan