Re: [PATCH v2] hvm/vpmu: Prevent dump handlers from incorrectly mutating state

[Suravee Suthikulanit <suravee.suthikulpanit@amd.com>]

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 <andrew.cooper3@citrix.com>
>
> 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 <jbeulich@suse.com>
>
> --- 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 <suravee.suthikulpanit@amd.com>