| Message ID | 1454991321-399-1-git-send-email-cardoe@cardoe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
On 02/09/2016 05:05 AM, 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. This looks wrong to me. vpmu_is_set() is almost exclusively used by VPMU code. > >> +#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) Hmm... acquire_pmu_ownship() vs acquire_pmu_ownership() and release_pmu_ownship() vs. release_pmu_ownership(). Since you are working with this code, can you also clean that up? It looks to me that at some point names got messed up. -boris
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 8e6e901..434d985 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -3,7 +3,7 @@ subdir-y += cpu subdir-y += genapic subdir-y += hvm subdir-y += mm -subdir-y += oprofile +subdir-$(xenoprof) += oprofile subdir-y += x86_64 obj-bin-y += alternative.init.o diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk index a1cdae0..94e4efd 100644 --- a/xen/arch/x86/Rules.mk +++ b/xen/arch/x86/Rules.mk @@ -10,6 +10,8 @@ CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default CFLAGS += '-D__OBJECT_LABEL__=$(subst /,$$,$(subst -,_,$(subst $(BASEDIR)/,,$(CURDIR))/$@))' +CFLAGS-$(xenoprof) += -DCONFIG_XENOPROF + # Prevent floating-point variables from creeping into Xen. CFLAGS += -msoft-float diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S index 3088aa7..6424ed0 100644 --- a/xen/arch/x86/x86_64/compat/entry.S +++ b/xen/arch/x86/x86_64/compat/entry.S @@ -394,6 +394,10 @@ compat_crash_page_fault: #define compat_kexec_op do_ni_hypercall #endif +#ifndef CONFIG_XENOPROF +#define compat_xenoprof_op do_ni_hypercall +#endif + ENTRY(compat_hypercall_table) .quad compat_set_trap_table /* 0 */ .quad do_mmu_update diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index 94a54aa..0a73878 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -727,6 +727,10 @@ ENTRY(exception_table) #define do_kexec_op do_ni_hypercall #endif +#ifndef CONFIG_XENOPROF +#define do_xenoprof_op do_ni_hypercall +#endif + ENTRY(hypercall_table) .quad do_set_trap_table /* 0 */ .quad do_mmu_update diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h index d97877d..a45d3ee 100644 --- a/xen/include/asm-x86/config.h +++ b/xen/include/asm-x86/config.h @@ -47,7 +47,6 @@ #define CONFIG_VGA 1 #define CONFIG_VIDEO 1 -#define CONFIG_XENOPROF 1 #define CONFIG_WATCHDOG 1 #define CONFIG_MULTIBOOT 1 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) +#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) +#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 #endif /* __ASM_X86_XENOPROF_H__ */ diff --git a/xen/include/xen/xenoprof.h b/xen/include/xen/xenoprof.h index 7804e69..2dd084a 100644 --- a/xen/include/xen/xenoprof.h +++ b/xen/include/xen/xenoprof.h @@ -74,7 +74,11 @@ int xenoprof_add_trace(struct vcpu *, uint64_t pc, int mode); #define PMU_OWNER_XENOPROF 1 #define PMU_OWNER_HVM 2 int acquire_pmu_ownship(int pmu_ownership); +#ifdef CONFIG_XENOPROF void release_pmu_ownship(int pmu_ownership); +#else +#define release_pmu_ownship(x) +#endif void xenoprof_log_event(struct vcpu *, const struct cpu_user_regs *, uint64_t pc, int mode, int event);
Allow Xenoprof to be fully disabled when toggling the option off. CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> Signed-off-by: Doug Goldstein <cardoe@cardoe.com> --- change since v1: - switch to #define empty 'functions' as suggested by Andrew Cooper --- xen/arch/x86/Makefile | 2 +- xen/arch/x86/Rules.mk | 2 ++ xen/arch/x86/x86_64/compat/entry.S | 4 ++++ xen/arch/x86/x86_64/entry.S | 4 ++++ xen/include/asm-x86/config.h | 1 - xen/include/asm-x86/vpmu.h | 9 +++++++++ xen/include/asm-x86/xenoprof.h | 7 +++++++ xen/include/xen/xenoprof.h | 4 ++++ 8 files changed, 31 insertions(+), 2 deletions(-)