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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/02/16 10:05, Andrew Cooper wrote:
> 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

One final thing to say.  Most of the header file should be inside an
#ifdef CONFIG_XENOPROF, with only the stub inlines in the else clause. 
This way none of the other internals are exposed to other Xen code if
CONFIG_XENOPROF is enabled.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel