From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v9 13/20] x86/VPMU: When handling MSR accesses, leave fault injection to callers Date: Tue, 12 Aug 2014 11:47:40 -0400 Message-ID: <53EA371C.4040806@oracle.com> References: <1407516946-17833-1-git-send-email-boris.ostrovsky@oracle.com> <1407516946-17833-14-git-send-email-boris.ostrovsky@oracle.com> <53EA2873020000780002B9CD@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53EA2873020000780002B9CD@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: kevin.tian@intel.com, keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, tim@xen.org, xen-devel@lists.xen.org, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org On 08/12/2014 08:45 AM, Jan Beulich wrote: >>>> On 08.08.14 at 18:55, wrote: >> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c >> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c >> @@ -458,13 +458,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_WARNING, >> + "Debug Store is not supported on this cpu\n"); >> + return 1; > The code here (and in particular the #GP injection) was wrong anyway; > see the small series I sent earlier today. Please base your on top of that. > >> @@ -476,18 +476,17 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content) >> { >> case MSR_CORE_PERF_GLOBAL_OVF_CTRL: >> core2_vpmu_cxt->global_status &= ~msr_content; >> - return 1; >> + return 0; >> case MSR_CORE_PERF_GLOBAL_STATUS: >> gdprintk(XENLOG_INFO, "Can not write readonly MSR: " >> "MSR_PERF_GLOBAL_STATUS(0x38E)!\n"); >> - hvm_inject_hw_exception(TRAP_gp_fault, 0); >> return 1; > Suspicious: Most return values get flipped, but this one (and at least > one more below) doesn't. Any such inconsistencies that are being > corrected (assuming this is intentional) as you go should be spelled > out in the description. And then the question of course is whether > it's really necessary to flip the meaning of this and some similar SVM > function's return values anyway. The return value of 1 of vpmu_do_msr() will now indicate that the routine encountered a fault as opposed to indicating whether the MSR access was to a VPMU register. I will make a comment to that effect. > >> @@ -541,45 +539,43 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content) >> } >> } >> >> - if ( (global_ctrl & *enabled_cntrs) || (core2_vpmu_cxt->ds_area != 0) ) >> - vpmu_set(vpmu, VPMU_RUNNING); >> - else >> - vpmu_reset(vpmu, VPMU_RUNNING); > The moving off this code is - again without at least a brief comment - > also not obviously correct; at least to me it looks like this slightly > alters behavior (perhaps towards the better, but anyway). Right. Because we don't want to change VPMU state if a fault is encountered (which is possible with current code). Come think of it, this is still not enough --- we may leave registers updated when fault happens. I should fix that too. -boris > >> @@ -607,19 +603,14 @@ static int core2_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content) >> rdmsrl(msr, *msr_content); >> } >> } >> - else >> + else if ( msr == MSR_IA32_MISC_ENABLE ) >> { >> /* Extension for BTS */ >> - if ( msr == MSR_IA32_MISC_ENABLE ) >> - { >> - if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) >> - *msr_content &= ~MSR_IA32_MISC_ENABLE_BTS_UNAVAIL; >> - } >> - else >> - return 0; > Again a case where the return value stays the same even if in > general it appears to get flipped in this function. > >> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) >> + *msr_content &= ~MSR_IA32_MISC_ENABLE_BTS_UNAVAIL; >> } >> >> - return 1; >> + return 0; >> } > Jan >