From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v2 1/2] xenoprof: fix up ability to disable
	it
Date: Tue, 9 Feb 2016 10:05:23 +0000
Message-ID: <56B9B9E3.9000700@citrix.com>
References: <1454991321-399-1-git-send-email-cardoe@cardoe.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1454991321-399-1-git-send-email-cardoe@cardoe.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: Doug Goldstein <cardoe@cardoe.com>, xen-devel@lists.xen.org
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>, Jan Beulich <jbeulich@suse.com>, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>, Jun Nakajima <jun.nakajima@intel.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
List-Id: xen-devel@lists.xenproject.org

On 09/02/16 04:15, Doug Goldstein wrote:
> diff --git a/xen/include/asm-x86/vpmu.h b/xen/include/asm-x86/vpmu.h
> index 67e73dc..4750a1f 100644
> --- a/xen/include/asm-x86/vpmu.h
> +++ b/xen/include/asm-x86/vpmu.h
> @@ -89,10 +89,14 @@ static inline void vpmu_clear(struct vpmu_struct *vpmu)
>  {
>      vpmu->flags = 0;
>  }
> +#ifdef CONFIG_XENOPROF
>  static inline bool_t vpmu_is_set(const struct vpmu_struct *vpmu, const u32 mask)
>  {
>      return !!(vpmu->flags & mask);
>  }
> +#else
> +#define vpmu_is_set(x, y) (!!0)

Why vpmu_is_set()?  this wasn't guarded in v1.

> +#endif
>  static inline bool_t vpmu_are_all_set(const struct vpmu_struct *vpmu,
>                                        const u32 mask)
>  {
> @@ -121,8 +125,13 @@ static inline int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content)
>      return vpmu_do_msr(msr, msr_content, 0, 0);
>  }
>  
> +#ifdef CONFIG_XENOPROF
>  extern int acquire_pmu_ownership(int pmu_ownership);
>  extern void release_pmu_ownership(int pmu_ownership);
> +#else
> +#define acquire_pmu_ownership(x) (1)
> +#define release_pmu_ownership(x)

Yikes - we have two externs of acquire_pmu_ownership().  This is plain
wrong and needs fixing as a separate patch.

These are xenoprof functions, so the externs in xenoprof.h are correct. 
These ones in vpmu.h should be deleted, and code should include
xenoprof.h instead.

> +#endif
>  
>  extern unsigned int vpmu_mode;
>  extern unsigned int vpmu_features;
> diff --git a/xen/include/asm-x86/xenoprof.h b/xen/include/asm-x86/xenoprof.h
> index b006ddc..9574f33 100644
> --- a/xen/include/asm-x86/xenoprof.h
> +++ b/xen/include/asm-x86/xenoprof.h
> @@ -67,9 +67,16 @@ void xenoprof_backtrace(struct vcpu *, const struct cpu_user_regs *,
>                   "xenoprof/x86 with autotranslated mode enabled"    \
>                   "isn't supported yet\n");                          \
>      } while (0)
> +
> +#ifdef CONFIG_XENOPROF
>  int passive_domain_do_rdmsr(unsigned int msr, uint64_t *msr_content);
>  int passive_domain_do_wrmsr(unsigned int msr, uint64_t msr_content);
>  void passive_domain_destroy(struct vcpu *v);
> +#else
> +#define passive_domain_do_rdmsr(x, y) (0)
> +#define passive_domain_do_wrmsr(x, y) (0)
> +#define passive_domain_destroy(x)
> +#endif

This looks much neater - Thanks.

However, please make static inline functions rather than macros, so that
the arguments still get typechecked and evaluated even if !CONFIG_XENOPROF

passive_domain_destroy() is particular is problematic if one were to
have a construct such as "if ( passive_domain_destroy() )" (although
that specific one is not a good example in this case).

~Andrew