From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v9 05/20] intel/VPMU: Clean up Intel VPMU code Date: Mon, 11 Aug 2014 17:13:33 +0100 Message-ID: <53E907CD020000780002B508@mail.emea.novell.com> References: <1407516946-17833-1-git-send-email-boris.ostrovsky@oracle.com> <1407516946-17833-6-git-send-email-boris.ostrovsky@oracle.com> <53E8E510020000780002B32A@mail.emea.novell.com> <53E8E8D3.2090305@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53E8E8D3.2090305@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, xen-devel@lists.xen.org, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org >>> On 11.08.14 at 18:01, wrote: > On 08/11/2014 09:45 AM, Jan Beulich wrote: >>>>> On 08.08.14 at 18:55, wrote: >>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>> @@ -1208,6 +1208,32 @@ int vmx_add_guest_msr(u32 msr) >>> return 0; >>> } >>> >>> +void vmx_rm_guest_msr(u32 msr) >>> +{ >>> + struct vcpu *curr = current; >>> + unsigned int idx, msr_count = curr->arch.hvm_vmx.msr_count; >>> + struct vmx_msr_entry *msr_area = curr->arch.hvm_vmx.msr_area; >>> + >>> + if ( msr_area == NULL ) >>> + return; >>> + >>> + for ( idx = 0; idx < msr_count; idx++ ) >>> + if ( msr_area[idx].index == msr ) >>> + break; >>> + >>> + if ( idx == msr_count ) >>> + return; >> This absence check (producing success) together with the presence >> check in the corresponding "add" function (also producing success) is >> certainly bogus without reference counting: Two independent callers >> of "add" would each validly assume they ought to call "rm" when done >> with their job, taking away the control over the MSR from the other. >> Yet if you don't think of independent callers, these presence/absence >> checks don't make sense at all. > > > Hmm, yes, that's a problem. Refcounting would require separate data > structures which would need to be dynamically grown (we don't know how > many MSR we will be tracking and most often the number is very small). > > So I wonder whether I should drop support for remove altogether since > this is only used in error path and I am not sure whether adding more > complexity is worth it. Probably not - see e.g. the debugctl and lbr handling, which also only ever adds MSRs to that list. That's not optimal, but sufficient for now - if we ever learn this presents a (performance) problem, we could then start thinking about adding remove support. Jan