[PATCH 1/2] perf: arm_pmuv3: Remove cyclical dependency with kvm_host.h

[PATCH 1/2] perf: arm_pmuv3: Remove cyclical dependency with kvm_host.h

[Colton Lewis]

asm/kvm_host.h includes asm/arm_pmu.h which includes perf/arm_pmuv3.h
which includes asm/arm_pmuv3.h which includes asm/kvm_host.h This
causes confusing compilation problems when trying to use anything in
the chain.

Break the cycle by taking asm/kvm_host.h out of asm/arm_pmuv3.h
because asm/kvm_host.h is huge and we only need a few functions from
it. Move the required declarations to asm/arm_pmuv3.h.

Signed-off-by: Colton Lewis <coltonlewis@google.com>
---
 arch/arm64/include/asm/arm_pmuv3.h | 24 ++++++++++++++++++++++--
 arch/arm64/include/asm/kvm_host.h  | 18 ------------------
 include/kvm/arm_pmu.h              |  1 -
 3 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/include/asm/arm_pmuv3.h b/arch/arm64/include/asm/arm_pmuv3.h
index 8a777dec8d88..89fd6abb7da6 100644
--- a/arch/arm64/include/asm/arm_pmuv3.h
+++ b/arch/arm64/include/asm/arm_pmuv3.h
@@ -6,11 +6,31 @@
 #ifndef __ASM_PMUV3_H
 #define __ASM_PMUV3_H
 
-#include <asm/kvm_host.h>
-
 #include <asm/cpufeature.h>
 #include <asm/sysreg.h>
 
+#include <linux/perf_event.h>
+
+#ifdef CONFIG_KVM
+void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr);
+void kvm_clr_pmu_events(u64 clr);
+bool kvm_set_pmuserenr(u64 val);
+#else
+static inline void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr) {}
+static inline void kvm_clr_pmu_events(u64 clr) {}
+static inline bool kvm_set_pmuserenr(u64 val)
+{
+	return false;
+}
+#endif
+
+static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr)
+{
+	return (!has_vhe() && attr->exclude_host);
+}
+
+void kvm_vcpu_pmu_resync_el0(void);
+
 #define RETURN_READ_PMEVCNTRN(n) \
 	return read_sysreg(pmevcntr##n##_el0)
 static inline unsigned long read_pmevcntrn(int n)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e18e9244d17a..3eeb762944c9 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1362,28 +1362,10 @@ void kvm_arch_vcpu_ctxflush_fp(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu);
 
-static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr)
-{
-	return (!has_vhe() && attr->exclude_host);
-}
-
 /* Flags for host debug state */
 void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
 
-#ifdef CONFIG_KVM
-void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr);
-void kvm_clr_pmu_events(u64 clr);
-bool kvm_set_pmuserenr(u64 val);
-#else
-static inline void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr) {}
-static inline void kvm_clr_pmu_events(u64 clr) {}
-static inline bool kvm_set_pmuserenr(u64 val)
-{
-	return false;
-}
-#endif
-
 void kvm_vcpu_load_vhe(struct kvm_vcpu *vcpu);
 void kvm_vcpu_put_vhe(struct kvm_vcpu *vcpu);
 
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 147bd3ee4f7b..2c78b1b1a9bb 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -74,7 +74,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu);
 struct kvm_pmu_events *kvm_get_pmu_events(void);
 void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu);
 void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
-void kvm_vcpu_pmu_resync_el0(void);
 
 #define kvm_vcpu_has_pmu(vcpu)					\
 	(vcpu_has_feature(vcpu, KVM_ARM_VCPU_PMU_V3))

base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532
-- 
2.48.1.362.g079036d154-goog

[PATCH 2/2] perf: arm_pmuv3: Uninvert dependency between {asm,perf}/arm_pmuv3.h

[Colton Lewis]

perf/arm_pmuv3.h includes asm/arm_pmuv3.h at the bottom of the
file. This counterintiutive decision was presumably made so
asm/arm_pmuv3.h would be included everywhere perf/arm_pmuv3.h was even
though the actual dependency relationship goes the other way because
asm/arm_pmuv3.h depends on the PMEVN_SWITCH macro that was presumably
put there to avoid duplicating it in the asm files for arm and arm64.

Extract the relevant macro to its own file to avoid this unusual
structure so it may be included in the asm headers without worrying
about ordering issues.

Signed-off-by: Colton Lewis <coltonlewis@google.com>
---
 arch/arm/include/asm/arm_pmuv3.h   |  2 ++
 arch/arm64/include/asm/arm_pmuv3.h |  1 +
 include/linux/perf/arm_pmuv3.h     | 49 ++-------------------------
 include/linux/perf/pmevn_switch.h  | 54 ++++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+), 47 deletions(-)
 create mode 100644 include/linux/perf/pmevn_switch.h

diff --git a/arch/arm/include/asm/arm_pmuv3.h b/arch/arm/include/asm/arm_pmuv3.h
index 2ec0e5e83fc9..a39277b6a365 100644
--- a/arch/arm/include/asm/arm_pmuv3.h
+++ b/arch/arm/include/asm/arm_pmuv3.h
@@ -9,6 +9,8 @@
 #include <asm/cp15.h>
 #include <asm/cputype.h>
 
+#include <linux/perf/pmevn_switch.h>
+
 #define PMCCNTR			__ACCESS_CP15_64(0, c9)
 
 #define PMCR			__ACCESS_CP15(c9,  0, c12, 0)
diff --git a/arch/arm64/include/asm/arm_pmuv3.h b/arch/arm64/include/asm/arm_pmuv3.h
index 89fd6abb7da6..e22fb26b6169 100644
--- a/arch/arm64/include/asm/arm_pmuv3.h
+++ b/arch/arm64/include/asm/arm_pmuv3.h
@@ -10,6 +10,7 @@
 #include <asm/sysreg.h>
 
 #include <linux/perf_event.h>
+#include <linux/perf/pmevn_switch.h>
 
 #ifdef CONFIG_KVM
 void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr);
diff --git a/include/linux/perf/arm_pmuv3.h b/include/linux/perf/arm_pmuv3.h
index d698efba28a2..00623b69cdcc 100644
--- a/include/linux/perf/arm_pmuv3.h
+++ b/include/linux/perf/arm_pmuv3.h
@@ -6,6 +6,8 @@
 #ifndef __PERF_ARM_PMUV3_H
 #define __PERF_ARM_PMUV3_H
 
+#include <asm/arm_pmuv3.h>
+
 #define ARMV8_PMU_MAX_GENERAL_COUNTERS	31
 #define ARMV8_PMU_CYCLE_IDX		31
 #define ARMV8_PMU_INSTR_IDX		32 /* Not accessible from AArch32 */
@@ -268,51 +270,4 @@
 #define ARMV8_PMU_BUS_WIDTH	GENMASK(19, 16)
 #define ARMV8_PMU_THWIDTH	GENMASK(23, 20)
 
-/*
- * This code is really good
- */
-
-#define PMEVN_CASE(n, case_macro) \
-	case n: case_macro(n); break
-
-#define PMEVN_SWITCH(x, case_macro)				\
-	do {							\
-		switch (x) {					\
-		PMEVN_CASE(0,  case_macro);			\
-		PMEVN_CASE(1,  case_macro);			\
-		PMEVN_CASE(2,  case_macro);			\
-		PMEVN_CASE(3,  case_macro);			\
-		PMEVN_CASE(4,  case_macro);			\
-		PMEVN_CASE(5,  case_macro);			\
-		PMEVN_CASE(6,  case_macro);			\
-		PMEVN_CASE(7,  case_macro);			\
-		PMEVN_CASE(8,  case_macro);			\
-		PMEVN_CASE(9,  case_macro);			\
-		PMEVN_CASE(10, case_macro);			\
-		PMEVN_CASE(11, case_macro);			\
-		PMEVN_CASE(12, case_macro);			\
-		PMEVN_CASE(13, case_macro);			\
-		PMEVN_CASE(14, case_macro);			\
-		PMEVN_CASE(15, case_macro);			\
-		PMEVN_CASE(16, case_macro);			\
-		PMEVN_CASE(17, case_macro);			\
-		PMEVN_CASE(18, case_macro);			\
-		PMEVN_CASE(19, case_macro);			\
-		PMEVN_CASE(20, case_macro);			\
-		PMEVN_CASE(21, case_macro);			\
-		PMEVN_CASE(22, case_macro);			\
-		PMEVN_CASE(23, case_macro);			\
-		PMEVN_CASE(24, case_macro);			\
-		PMEVN_CASE(25, case_macro);			\
-		PMEVN_CASE(26, case_macro);			\
-		PMEVN_CASE(27, case_macro);			\
-		PMEVN_CASE(28, case_macro);			\
-		PMEVN_CASE(29, case_macro);			\
-		PMEVN_CASE(30, case_macro);			\
-		default: WARN(1, "Invalid PMEV* index\n");	\
-		}						\
-	} while (0)
-
-#include <asm/arm_pmuv3.h>
-
 #endif
diff --git a/include/linux/perf/pmevn_switch.h b/include/linux/perf/pmevn_switch.h
new file mode 100644
index 000000000000..1f211468d8bf
--- /dev/null
+++ b/include/linux/perf/pmevn_switch.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __PMEVN_SWITCH_H
+#define __PMEVN_SWITCH_H
+
+#include <asm-generic/bug.h>
+
+/*
+ * This code is really good
+ */
+
+#define PMEVN_CASE(n, case_macro) \
+	case n: case_macro(n); break
+
+#define PMEVN_SWITCH(x, case_macro)				\
+	do {							\
+		switch (x) {					\
+		PMEVN_CASE(0,  case_macro);			\
+		PMEVN_CASE(1,  case_macro);			\
+		PMEVN_CASE(2,  case_macro);			\
+		PMEVN_CASE(3,  case_macro);			\
+		PMEVN_CASE(4,  case_macro);			\
+		PMEVN_CASE(5,  case_macro);			\
+		PMEVN_CASE(6,  case_macro);			\
+		PMEVN_CASE(7,  case_macro);			\
+		PMEVN_CASE(8,  case_macro);			\
+		PMEVN_CASE(9,  case_macro);			\
+		PMEVN_CASE(10, case_macro);			\
+		PMEVN_CASE(11, case_macro);			\
+		PMEVN_CASE(12, case_macro);			\
+		PMEVN_CASE(13, case_macro);			\
+		PMEVN_CASE(14, case_macro);			\
+		PMEVN_CASE(15, case_macro);			\
+		PMEVN_CASE(16, case_macro);			\
+		PMEVN_CASE(17, case_macro);			\
+		PMEVN_CASE(18, case_macro);			\
+		PMEVN_CASE(19, case_macro);			\
+		PMEVN_CASE(20, case_macro);			\
+		PMEVN_CASE(21, case_macro);			\
+		PMEVN_CASE(22, case_macro);			\
+		PMEVN_CASE(23, case_macro);			\
+		PMEVN_CASE(24, case_macro);			\
+		PMEVN_CASE(25, case_macro);			\
+		PMEVN_CASE(26, case_macro);			\
+		PMEVN_CASE(27, case_macro);			\
+		PMEVN_CASE(28, case_macro);			\
+		PMEVN_CASE(29, case_macro);			\
+		PMEVN_CASE(30, case_macro);			\
+		default: WARN(1, "Invalid PMEV* index\n");	\
+		}						\
+	} while (0)
+
+
+#endif
-- 
2.48.1.362.g079036d154-goog

Re: [PATCH 1/2] perf: arm_pmuv3: Remove cyclical dependency with kvm_host.h

[Oliver Upton]

Hi Colton,

On Tue, Feb 04, 2025 at 07:57:07PM +0000, Colton Lewis wrote:
> asm/kvm_host.h includes asm/arm_pmu.h which includes perf/arm_pmuv3.h
> which includes asm/arm_pmuv3.h which includes asm/kvm_host.h This
> causes confusing compilation problems when trying to use anything in
> the chain.
> 
> Break the cycle by taking asm/kvm_host.h out of asm/arm_pmuv3.h
> because asm/kvm_host.h is huge and we only need a few functions from
> it. Move the required declarations to asm/arm_pmuv3.h.
> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>

Please do not move KVM namespaced functions into non-KVM headers. Having
a separate header for KVM<->PMUv3 driver interfaces is probably the
right thing to do, especially since you're going to be adding more with
partitioned PMU support.

-- 
Thanks,
Oliver

Re: [PATCH 2/2] perf: arm_pmuv3: Uninvert dependency between {asm,perf}/arm_pmuv3.h

[Oliver Upton]

On Tue, Feb 04, 2025 at 07:57:08PM +0000, Colton Lewis wrote:
> perf/arm_pmuv3.h includes asm/arm_pmuv3.h at the bottom of the
> file. This counterintiutive decision was presumably made so
> asm/arm_pmuv3.h would be included everywhere perf/arm_pmuv3.h was even
> though the actual dependency relationship goes the other way because
> asm/arm_pmuv3.h depends on the PMEVN_SWITCH macro that was presumably
> put there to avoid duplicating it in the asm files for arm and arm64.
> 
> Extract the relevant macro to its own file to avoid this unusual
> structure so it may be included in the asm headers without worrying
> about ordering issues.
> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>

Is the intention of this change to allow asm/arm_pmuv3.h to be directly
included? If yes, what's the issue with using perf/arm_pmuv3.h?

We already use definitions from the non-arch header in KVM anyway...

-- 
Thanks,
Oliver

Re: [PATCH 1/2] perf: arm_pmuv3: Remove cyclical dependency with kvm_host.h

[Colton Lewis]

Oliver Upton <oliver.upton@linux.dev> writes:

> Hi Colton,

> On Tue, Feb 04, 2025 at 07:57:07PM +0000, Colton Lewis wrote:
>> asm/kvm_host.h includes asm/arm_pmu.h which includes perf/arm_pmuv3.h
>> which includes asm/arm_pmuv3.h which includes asm/kvm_host.h This
>> causes confusing compilation problems when trying to use anything in
>> the chain.

>> Break the cycle by taking asm/kvm_host.h out of asm/arm_pmuv3.h
>> because asm/kvm_host.h is huge and we only need a few functions from
>> it. Move the required declarations to asm/arm_pmuv3.h.

>> Signed-off-by: Colton Lewis <coltonlewis@google.com>

> Please do not move KVM namespaced functions into non-KVM headers. Having
> a separate header for KVM<->PMUv3 driver interfaces is probably the
> right thing to do, especially since you're going to be adding more with
> partitioned PMU support.

That seems like a good idea to me.

Re: [PATCH 2/2] perf: arm_pmuv3: Uninvert dependency between {asm,perf}/arm_pmuv3.h

[Colton Lewis]

Oliver Upton <oliver.upton@linux.dev> writes:

> On Tue, Feb 04, 2025 at 07:57:08PM +0000, Colton Lewis wrote:
>> perf/arm_pmuv3.h includes asm/arm_pmuv3.h at the bottom of the
>> file. This counterintiutive decision was presumably made so
>> asm/arm_pmuv3.h would be included everywhere perf/arm_pmuv3.h was even
>> though the actual dependency relationship goes the other way because
>> asm/arm_pmuv3.h depends on the PMEVN_SWITCH macro that was presumably
>> put there to avoid duplicating it in the asm files for arm and arm64.

>> Extract the relevant macro to its own file to avoid this unusual
>> structure so it may be included in the asm headers without worrying
>> about ordering issues.

>> Signed-off-by: Colton Lewis <coltonlewis@google.com>

> Is the intention of this change to allow asm/arm_pmuv3.h to be directly
> included? If yes, what's the issue with using perf/arm_pmuv3.h?

That isn't the primary intent, but it's a good side effect. Headers that
can't be directly included violate people's expectations.

> We already use definitions from the non-arch header in KVM anyway...

My intention here was just reorganizing a counterintuitive use of
headers. The arch header depends on definitions in the non-arch header
even though the inclusion relationship is the other way around.

But this patch doesn't matter as much as fixing the cyclical dependency.