Re: [PATCH v2 1/2] xenoprof: fix up ability to disable it

[Andrew Cooper <andrew.cooper3@citrix.com>]

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