From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH v2] hvm/vpmu: Prevent dump handlers from incorrectly mutating state Date: Thu, 12 Sep 2013 18:28:19 -0500 Message-ID: <52324E13.4080907@amd.com> References: <1378918711-21543-1-git-send-email-andrew.cooper3@citrix.com> <52318D1F02000078000F29E7@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VKGJG-0003eZ-HC for xen-devel@lists.xenproject.org; Thu, 12 Sep 2013 23:28:26 +0000 In-Reply-To: <52318D1F02000078000F29E7@nat28.tlf.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: Keir Fraser , Eddie Dong , Andrew Cooper , Jacob Shin , Jun Nakajima , xen-devel , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On 9/12/2013 2:45 AM, Jan Beulich wrote: > Discovered by Coverity, CID 1055181 > > core2_vpmu_dump() was incorrectly setting VPMU_CONTEXT_LOADED when it > was intending to check for it. > > Signed-off-by: Andrew Cooper > > This would have been avoided if the dump function declared all its > pointers "const" - doing this now (also in SVM). > > Also fixing some indentation issues at once. > > Signed-off-by: Jan Beulich > > --- a/xen/arch/x86/hvm/svm/vpmu.c > +++ b/xen/arch/x86/hvm/svm/vpmu.c > @@ -415,10 +415,10 @@ static void amd_vpmu_destroy(struct vcpu > } > > /* VPMU part of the 'q' keyhandler */ > -static void amd_vpmu_dump(struct vcpu *v) > +static void amd_vpmu_dump(const struct vcpu *v) > { > - struct vpmu_struct *vpmu = vcpu_vpmu(v); > - struct amd_vpmu_context *ctxt = vpmu->context; > + const struct vpmu_struct *vpmu = vcpu_vpmu(v); > + const struct amd_vpmu_context *ctxt = vpmu->context; > unsigned int i; > > printk(" VPMU state: 0x%x ", vpmu->flags); > @@ -447,9 +447,9 @@ static void amd_vpmu_dump(struct vcpu *v > > rdmsrl(ctrls[i], ctrl); > rdmsrl(counters[i], cntr); > - printk(" 0x%08x: 0x%lx (0x%lx in HW) 0x%08x: 0x%lx (0x%lx in HW)\n", > - ctrls[i], ctxt->ctrls[i], ctrl, > - counters[i], ctxt->counters[i], cntr); > + printk(" %#x: %#lx (%#lx in HW) %#x: %#lx (%#lx in HW)\n", > + ctrls[i], ctxt->ctrls[i], ctrl, > + counters[i], ctxt->counters[i], cntr); > } > } > > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c > @@ -678,11 +678,11 @@ static void core2_vpmu_do_cpuid(unsigned > } > > /* Dump vpmu info on console, called in the context of keyhandler 'q'. */ > -static void core2_vpmu_dump(struct vcpu *v) > +static void core2_vpmu_dump(const struct vcpu *v) > { > - struct vpmu_struct *vpmu = vcpu_vpmu(v); > + const struct vpmu_struct *vpmu = vcpu_vpmu(v); > int i, num; > - struct core2_vpmu_context *core2_vpmu_cxt = NULL; > + const struct core2_vpmu_context *core2_vpmu_cxt = NULL; > u64 val; > > if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) > @@ -690,7 +690,7 @@ static void core2_vpmu_dump(struct vcpu > > if ( !vpmu_is_set(vpmu, VPMU_RUNNING) ) > { > - if ( vpmu_set(vpmu, VPMU_CONTEXT_LOADED) ) > + if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) ) > printk(" vPMU loaded\n"); > else > printk(" vPMU allocated\n"); > @@ -703,10 +703,11 @@ static void core2_vpmu_dump(struct vcpu > /* Print the contents of the counter and its configuration msr. */ > for ( i = 0; i < num; i++ ) > { > - struct arch_msr_pair* msr_pair = core2_vpmu_cxt->arch_msr_pair; > + const struct arch_msr_pair *msr_pair = core2_vpmu_cxt->arch_msr_pair; > + > if ( core2_vpmu_cxt->pmu_enable->arch_pmc_enable[i] ) > printk(" general_%d: 0x%016lx ctrl: 0x%016lx\n", > - i, msr_pair[i].counter, msr_pair[i].control); > + i, msr_pair[i].counter, msr_pair[i].control); > } > /* > * The configuration of the fixed counter is 4 bits each in the > @@ -716,9 +717,9 @@ static void core2_vpmu_dump(struct vcpu > for ( i = 0; i < core2_fix_counters.num; i++ ) > { > if ( core2_vpmu_cxt->pmu_enable->fixed_ctr_enable[i] ) > - printk(" fixed_%d: 0x%016lx ctrl: 0x%lx\n", > - i, core2_vpmu_cxt->fix_counters[i], > - val & FIXED_CTR_CTRL_MASK); > + printk(" fixed_%d: 0x%016lx ctrl: %#lx\n", > + i, core2_vpmu_cxt->fix_counters[i], > + val & FIXED_CTR_CTRL_MASK); > val >>= FIXED_CTR_CTRL_BITS; > } > } > --- a/xen/include/asm-x86/hvm/vpmu.h > +++ b/xen/include/asm-x86/hvm/vpmu.h > @@ -54,7 +54,7 @@ struct arch_vpmu_ops { > void (*arch_vpmu_destroy)(struct vcpu *v); > int (*arch_vpmu_save)(struct vcpu *v); > void (*arch_vpmu_load)(struct vcpu *v); > - void (*arch_vpmu_dump)(struct vcpu *v); > + void (*arch_vpmu_dump)(const struct vcpu *); > }; > > int vmx_vpmu_initialise(struct vcpu *, unsigned int flags); > > Acked-by: Suravee Suthikulpanit