From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v23 11/15] VPMU/AMD: Check MSR values before writing to hardware Date: Fri, 05 Jun 2015 12:32:55 -0400 Message-ID: <5571CF37.80803@oracle.com> References: <1432924980-24281-1-git-send-email-boris.ostrovsky@oracle.com> <1432924980-24281-12-git-send-email-boris.ostrovsky@oracle.com> <5571E464020000780008184C@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: <5571E464020000780008184C@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/05/2015 12:03 PM, Jan Beulich wrote: >>>> On 29.05.15 at 20:42, wrote: >> @@ -289,19 +302,24 @@ static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, >> { >> struct vcpu *v = current; >> struct vpmu_struct *vpmu = vcpu_vpmu(v); >> + unsigned int idx = 0; >> + int type = get_pmu_reg_type(msr, &idx); >> >> ASSERT(!supported); >> >> /* For all counters, enable guest only mode for HVM guest */ >> - if ( has_hvm_container_vcpu(v) && >> - (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) && >> + if ( has_hvm_container_vcpu(v) && (type == MSR_TYPE_CTRL) && >> !is_guest_mode(msr_content) ) >> { >> set_guest_mode(msr_content); >> } >> >> + if ( (type == MSR_TYPE_CTRL ) && >> + ((msr_content & CTRL_RSVD_MASK) != ctrl_rsvd[idx]) ) >> + return 1; > I think the check should go before the use of the value for anything, > i.e. including the set_guest_mode() above. > > Also I'm pretty sure I asked about the meaning of 1 as a return > value of a function returning int: If this is a boolean, the function > should return bool_t (and probably use 1 as success indicator, > while the case at hand appears to be a failure one). If this is an > error indicator, -E... values should be used. Of course, if for some > reason this is meant to represent success, considering the function > being that way even before your change, I'd not insist on you > re-working the return value aspects of it. One means error (which is the reverse of what it is now), this is described in patch 8. We did talk about this a while ago (not during latest round) when IIRC you requested me to clarify this in the commit message. I can replace these 1s with -EINVAL (or -EFAULT). -boris