From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v7 05/19] intel/VPMU: Clean up Intel VPMU
 code
Date: Fri, 06 Jun 2014 15:43:47 -0400
Message-ID: <539219F3.5030502@oracle.com>
References: <1402076415-26475-1-git-send-email-boris.ostrovsky@oracle.com>
	<1402076415-26475-6-git-send-email-boris.ostrovsky@oracle.com>
	<539209AC.4070607@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <539209AC.4070607@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: kevin.tian@intel.com, keir@xen.org, JBeulich@suse.com, jun.nakajima@intel.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, suravee.suthikulpanit@amd.com
List-Id: xen-devel@lists.xenproject.org

On 06/06/2014 02:34 PM, Andrew Cooper wrote:
>
> +static int core2_get_arch_pmc_count(void)
> +{
> +    u32 eax;
>   
> -static const struct pmumsr core2_ctrls = {
> -    VPMU_CORE2_NUM_CTRLS,
> -    core2_ctrls_msr
> -};
> -static int arch_pmc_cnt;
> +    eax = cpuid_eax(0xa);
> +    return ( (eax & PMU_GENERAL_NR_MASK) >> PMU_GENERAL_NR_SHIFT );
> +}
> This (and later) can be made much simpler.  The style guidelines easily
> permit:
>
> static int core2_get_arch_pmc_count(void)
> {
>      return (cpuid_eax(0xa) & PMU_GENERAL_NR_MASK) >> PMU_GENERAL_NR_SHIFT;
> }
>
> Unrelated to this code itself, I wonder whether Xen should gain some
> mnemonics for cpuid leaves.

Not sure what you mean here.

>   
>   static void core2_vpmu_destroy(struct vcpu *v)
>   {
>       struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct core2_vpmu_context *core2_vpmu_cxt = vpmu->context;
>   
>       if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>           return;
> -    xfree(core2_vpmu_cxt->pmu_enable);
> +
>       xfree(vpmu->context);
> Is it really right to bail if !VPMU_CONTEXT_ALLOCATED ?  A stray
> vpmu_clear() would result in a memory leak.
>
> i.e. can VPMU_CONTEXT_ALLOCATED be deemed from "vpmu->context != NULL"
> instead?

I think so.

But the flag is used throughout VPMU code to indicate availability of 
context so for consistency I think we should continue using it here. And 
I don't want to drop it as a flag completely since checking for it is a 
tiny bit faster than for NULL pointer: we often check multiple flags at 
once.

-boris