From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v24 08/15] x86/VPMU: When handling MSR accesses, leave fault injection to callers Date: Mon, 15 Jun 2015 12:23:05 -0400 Message-ID: <557EFBE9.40100@oracle.com> References: <1433948671-29504-1-git-send-email-boris.ostrovsky@oracle.com> <1433948671-29504-9-git-send-email-boris.ostrovsky@oracle.com> <557F06020200007800084FDC@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: <557F06020200007800084FDC@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, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, Aravind.Gopalakrishnan@amd.com, jun.nakajima@intel.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On 06/15/2015 11:06 AM, Jan Beulich wrote: >>>> On 10.06.15 at 17:04, wrote: >> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c >> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c >> @@ -454,36 +454,41 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, >> IA32_DEBUGCTLMSR_BTS_OFF_USR; >> if ( !(msr_content & ~supported) && >> vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) >> - return 1; >> + return 0; >> if ( (msr_content & supported) && >> !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) >> printk(XENLOG_G_WARNING >> "%pv: Debug Store unsupported on this CPU\n", >> current); >> } >> - return 0; >> + return -EINVAL; >> } >> >> ASSERT(!supported); >> >> + if ( type == MSR_TYPE_COUNTER && >> + (msr_content & >> + ~((1ull << core2_get_bitwidth_fix_count()) - 1)) ) >> + /* Writing unsupported bits to a fixed counter */ >> + return -EINVAL; >> + >> core2_vpmu_cxt = vpmu->context; >> enabled_cntrs = vpmu->priv_context; >> switch ( msr ) >> { >> 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; >> + return -EINVAL; > Is it intentional that you convert a success to a failure here? If so, > this should be mentioned (with reason) in the commit message. If > not, this should be adjusted to there's no behavioral change here. Yes, this is intentional. Until now return value indicated whether access was to a PMU register. This worked for HVM guests since they can do hvm_inject_trap() at any time. For PV guests we are called from emulate_privileged_op() and we need to know whether access was successful or not. This way emulate_privileged_op() will take care of fault injection by returning 0. -boris