From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v18 05/16] x86/VPMU: Interface for setting PMU mode and flags Date: Fri, 20 Feb 2015 11:04:45 -0500 Message-ID: <54E75B1D.4030402@oracle.com> References: <1424125619-10851-1-git-send-email-boris.ostrovsky@oracle.com> <1424125619-10851-6-git-send-email-boris.ostrovsky@oracle.com> <54E74BEC020000780006208F@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: <54E74BEC020000780006208F@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 02/20/2015 08:59 AM, Jan Beulich wrote: >>>> On 16.02.15 at 23:26, wrote: >> --- a/xen/arch/x86/hvm/svm/vpmu.c >> +++ b/xen/arch/x86/hvm/svm/vpmu.c >> @@ -253,6 +253,26 @@ static int amd_vpmu_save(struct vpmu_struct *vpmu) >> return 1; >> } >> >> +static void amd_vpmu_unload(struct vpmu_struct *vpmu) >> +{ >> + struct vcpu *v; >> + >> + if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED | VPMU_FROZEN) ) >> + { >> + unsigned int i; >> + >> + for ( i = 0; i < num_counters; i++ ) >> + wrmsrl(ctrls[i], 0); >> + context_save(vpmu); > This assumes vpmu_vcpu(vpmu) == current, yet it looks like you're > also calling it under different circumstances now. This doesn't make this assumption. PMU MSRs may be loaded with values that belong to a VCPU that is not currently running if, for example, current VCPU is not using PMU. And flags test guarantees that we won't stomp on someone else's registers. I could make a test here and write zeroes to the MSRs only if vpmu is current but I don't like leaving these MSRs with what is essentially garbage values. This routine is executed very rarely so overhead is negligible. > >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -1183,11 +1183,10 @@ int vmx_read_guest_msr(u32 msr, u64 *val) >> return -ESRCH; >> } >> >> -int vmx_write_guest_msr(u32 msr, u64 val) >> +int vmx_write_guest_msr_vcpu(struct vcpu *v, u32 msr, u64 val) >> { >> - struct vcpu *curr = current; >> - unsigned int i, msr_count = curr->arch.hvm_vmx.msr_count; >> - struct vmx_msr_entry *msr_area = curr->arch.hvm_vmx.msr_area; >> + unsigned int i, msr_count = v->arch.hvm_vmx.msr_count; >> + struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area; >> >> for ( i = 0; i < msr_count; i++ ) >> { > Same here - while the code itself is only accessing memory, it > remains unclear whether correct behavior results when the subject > vCPU is actually executing. Not sure what you mean here. The changes are specifically for updating MSR values (cached in VMCS) of possibly non-current VCPU. > > In both cases, if there are restrictions on the conditions under > which unload can validly be done, I think you should ASSERT() > those conditions (at once making them explicit). > >> +long do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg) >> +{ >> + int ret; >> + struct xen_pmu_params pmu_params; >> + >> + if ( vpmu_disabled ) >> + return -EINVAL; > EOPNOTSUPP perhaps? > > And I agree with Dietmar that keeping opt_vpmu_enabled instead > of introducing vpmu_disabled would seem more natural, the more > that the default is disabled. I can switch polarity back to enabled since both of you think it makes more sense but I don't think I should keep "opt_" prefix since this flag can be modified independently of what boot option says (e.g. when watchdog is on). > >> +/* Context switch */ >> +static inline void vpmu_switch_from(struct vpmu_struct *prev, >> + struct vpmu_struct *next) >> +{ >> + if ( vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV) ) >> + vpmu_save(prev); >> +} >> + >> +static inline void vpmu_switch_to(struct vpmu_struct *prev, >> + struct vpmu_struct *next) >> +{ >> + if ( vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV) ) >> + vpmu_load(next); >> + else if ( vpmu_is_set(next, VPMU_CONTEXT_LOADED | VPMU_RUNNING) ) >> + vpmu_unload(next); > I don't recall there having been an unload path here, and I don't > see this being explained anywhere either. Wouldn't that more > naturally be done in the switch-from function? I had this for a couple of revisions now. And I had a very specific reason for (1) having it and (2) having it here (it had something to do with the fact that 'next' may still be LOADED after we've changed vpmu_mode to, say, OFF) . But I can't remember what it was now. If I remember why I'll add a comment, otherwise I'll drop this. > > Apart from that it's also not clear why both prev and next get > passed to both of these functions. Iirc a later patch may make > use of that, but then that later patch should add the second > function parameter. Right, I was thinking of using both parameters at some point and left them here. Only one is needed. -boris