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

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

[Doug Goldstein]

Allow Xenoprof to be fully disabled when toggling the option off.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>

change since v3:
- drop (void)var; from static inlines
- fix typo that broke build (must have forgotten to do XEN_CONFIG_EXPERT=y make
change since v2:
- move all functions in xenoprof.h inside CONFIG_XENOPROF as suggested by
  Andrew Cooper
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/xenoprof.h     | 19 +++++++++++++++++++
 xen/include/xen/xenoprof.h         | 25 +++++++++++++++++++------
 7 files changed, 49 insertions(+), 8 deletions(-)

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/xenoprof.h b/xen/include/asm-x86/xenoprof.h
index b006ddc..7044084 100644
--- a/xen/include/asm-x86/xenoprof.h
+++ b/xen/include/asm-x86/xenoprof.h
@@ -67,9 +67,28 @@ 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
+static inline int passive_domain_do_rdmsr(unsigned int msr,
+                                          uint64_t *msr_content)
+{
+    return 0;
+}
+
+static inline int passive_domain_do_wrmsr(unsigned int msr,
+                                          uint64_t msr_content)
+{
+    return 0;
+}
+
+static inline void passive_domain_destroy(struct vcpu *v)
+{
+}
+#endif
 
 #endif /* __ASM_X86_XENOPROF_H__ */
 
diff --git a/xen/include/xen/xenoprof.h b/xen/include/xen/xenoprof.h
index 9b9ef56..8148c01 100644
--- a/xen/include/xen/xenoprof.h
+++ b/xen/include/xen/xenoprof.h
@@ -64,19 +64,32 @@ struct xenoprof {
 #endif
 
 struct domain;
-int is_active(struct domain *d);
-int is_passive(struct domain *d);
-void free_xenoprof_pages(struct domain *d);
-
-int xenoprof_add_trace(struct vcpu *, uint64_t pc, int mode);
-
 #define PMU_OWNER_NONE          0
 #define PMU_OWNER_XENOPROF      1
 #define PMU_OWNER_HVM           2
+
+#ifdef CONFIG_XENOPROF
 int acquire_pmu_ownership(int pmu_ownership);
 void release_pmu_ownership(int pmu_ownership);
 
+int is_active(struct domain *d);
+int is_passive(struct domain *d);
+void free_xenoprof_pages(struct domain *d);
+
+int xenoprof_add_trace(struct vcpu *, uint64_t pc, int mode);
+
 void xenoprof_log_event(struct vcpu *, const struct cpu_user_regs *,
                         uint64_t pc, int mode, int event);
 
+#else
+static inline int acquire_pmu_ownership(int pmu_ownership)
+{
+    return 1;
+}
+
+static inline void release_pmu_ownership(int pmu_ownership)
+{
+}
+#endif
+
 #endif  /* __XEN__XENOPROF_H__ */
-- 
2.4.10

[PATCH v4 2/2] build: convert xenoprof to Kconfig

[Doug Goldstein]

Convert the xenoprof x86 build time option to Kconfig.

CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>

change since v3:
- move xenoprof entry to the main sources list
- combine 'default' and 'bool' into 'def_bool'
change since v2:
- require EXPERT for XENOPROF as suggested by Jan Beulich
change since v1:
- fix name of Kconfig entry as suggested by Andrew Cooper
---
 xen/arch/x86/Makefile |  2 +-
 xen/arch/x86/Rules.mk |  3 ---
 xen/common/Kconfig    | 13 +++++++++++++
 xen/common/Makefile   |  2 +-
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 434d985..1bcb08b 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-$(xenoprof) += oprofile
+subdir-$(CONFIG_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 94e4efd..14519e3 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -3,15 +3,12 @@
 
 HAS_NUMA := y
 HAS_CORE_PARKING := y
-xenoprof := y
 
 CFLAGS += -I$(BASEDIR)/include 
 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/common/Kconfig b/xen/common/Kconfig
index 6f404b4..49de790 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -84,6 +84,19 @@ config LATE_HWDOM
 
 	  If unsure, say N.
 
+# Adds support for Xenoprof
+config XENOPROF
+	def_bool y
+	prompt "Xen Oprofile Support" if EXPERT = "y"
+	depends on X86
+	---help---
+	  Xen OProfile (Xenoprof) is a system-wide profiler for Xen virtual
+	  machine environments, capable of profiling the Xen virtual machine
+	  monitor, multiple Linux guest operating systems, and applications
+	  running on them.
+
+	  If unsure, say Y.
+
 # Enable/Disable XSM support
 config XSM
 	bool "Xen Security Modules support"
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 0d76efe..57f4ed7 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -57,13 +57,13 @@ obj-y += vm_event.o
 obj-y += vmap.o
 obj-y += vsprintf.o
 obj-y += wait.o
+obj-$(CONFIG_XENOPROF) += xenoprof.o
 obj-y += xmalloc_tlsf.o
 
 obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
 
 obj-$(perfc)       += perfc.o
 obj-$(crash_debug) += gdbstub.o
-obj-$(xenoprof)    += xenoprof.o
 
 obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall.o tmem_xen.o xlat.o)
 
-- 
2.4.10

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

[Andrew Cooper]

On 17/02/16 18:02, Doug Goldstein wrote:
> Allow Xenoprof to be fully disabled when toggling the option off.
>
> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
> ---
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>
> change since v3:
> - drop (void)var; from static inlines
> - fix typo that broke build (must have forgotten to do XEN_CONFIG_EXPERT=y make
> change since v2:
> - move all functions in xenoprof.h inside CONFIG_XENOPROF as suggested by
>   Andrew Cooper
> 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/xenoprof.h     | 19 +++++++++++++++++++
>  xen/include/xen/xenoprof.h         | 25 +++++++++++++++++++------
>  7 files changed, 49 insertions(+), 8 deletions(-)
>
> 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/xenoprof.h b/xen/include/asm-x86/xenoprof.h
> index b006ddc..7044084 100644
> --- a/xen/include/asm-x86/xenoprof.h
> +++ b/xen/include/asm-x86/xenoprof.h
> @@ -67,9 +67,28 @@ 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

Sorry for not commenting before you resubmitted.

I would suggest having this #ifdef immediately under this files
inclusion guard.  That way you hide all symbols and declarations in the
!CONFIG_XENOPROF case.  Given that this patch compiles, I don't expect
you will encounter any issues from moving it.

>  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

As a matter of style, please put newlines around here to space it out a
bit, and /* CONFIG_XENOPROF */ after the else, as the original #if is a
long way away.

> +static inline int passive_domain_do_rdmsr(unsigned int msr,
> +                                          uint64_t *msr_content)
> +{
> +    return 0;
> +}
> +
> +static inline int passive_domain_do_wrmsr(unsigned int msr,
> +                                          uint64_t msr_content)
> +{
> +    return 0;
> +}
> +
> +static inline void passive_domain_destroy(struct vcpu *v)

For brevity, "static inline void passive_domain_destroy(struct vcpu *v)
{}" is perfectly fine here.

> +{
> +}
> +#endif
>  
>  #endif /* __ASM_X86_XENOPROF_H__ */
>  
> diff --git a/xen/include/xen/xenoprof.h b/xen/include/xen/xenoprof.h
> index 9b9ef56..8148c01 100644
> --- a/xen/include/xen/xenoprof.h
> +++ b/xen/include/xen/xenoprof.h
> @@ -64,19 +64,32 @@ struct xenoprof {
>  #endif
>  
>  struct domain;
> -int is_active(struct domain *d);
> -int is_passive(struct domain *d);
> -void free_xenoprof_pages(struct domain *d);
> -
> -int xenoprof_add_trace(struct vcpu *, uint64_t pc, int mode);
> -
>  #define PMU_OWNER_NONE          0
>  #define PMU_OWNER_XENOPROF      1
>  #define PMU_OWNER_HVM           2
> +
> +#ifdef CONFIG_XENOPROF

Same comment concerning the position of this #ifdef.  I can't see any
symbols which will cause an issue.

With these issues addressed, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>