From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v9 05/20] intel/VPMU: Clean up Intel VPMU code Date: Mon, 11 Aug 2014 12:01:23 -0400 Message-ID: <53E8E8D3.2090305@oracle.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53E8E510020000780002B32A@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/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. -boris