[PATCH v2 0/4] arm64: Support perf event modifiers :G and :H

[PATCH v2 0/4] arm64: Support perf event modifiers :G and :H

[Andrew Murray]

This patchset provides support for perf event modifiers :G and :H which
allows for filtering of PMU events between host and guests when used
with KVM.

As the underlying hardware cannot distinguish between guest and host
context, the performance counters must be stopped and started upon
entry/exit to the guest. This is performed at EL2 in a way that
minimizes overhead and improves accuracy of recording events that only
occur in the requested context.

This has been tested with VHE and non-VHE kernels with a KVM guest.

Changes from v1:

 - Removed unnecessary exclusion of EL1 with exclude_guest on VHE
 - Removed unnecessary isb from existing perf_event.c driver
 - Folded perf_event.c patches together
 - Added additional information to last patch commit message

Andrew Murray (4):
  arm64: arm_pmu: remove unnecessary isb instruction
  arm64: KVM: add accessors to track guest/host only counters
  arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes
  arm64: KVM: Enable support for :G/:H perf event modifiers

 arch/arm64/include/asm/kvm_host.h | 20 +++++++++++++++++++
 arch/arm64/kernel/perf_event.c    | 42 ++++++++++++++++++++++++++++++++-------
 arch/arm64/kvm/hyp/switch.c       | 38 +++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v2 0/4] arm64: Support perf event modifiers :G and :H

[Andrew Murray]

This patchset provides support for perf event modifiers :G and :H which
allows for filtering of PMU events between host and guests when used
with KVM.

As the underlying hardware cannot distinguish between guest and host
context, the performance counters must be stopped and started upon
entry/exit to the guest. This is performed at EL2 in a way that
minimizes overhead and improves accuracy of recording events that only
occur in the requested context.

This has been tested with VHE and non-VHE kernels with a KVM guest.

Changes from v1:

 - Removed unnecessary exclusion of EL1 with exclude_guest on VHE
 - Removed unnecessary isb from existing perf_event.c driver
 - Folded perf_event.c patches together
 - Added additional information to last patch commit message

Andrew Murray (4):
  arm64: arm_pmu: remove unnecessary isb instruction
  arm64: KVM: add accessors to track guest/host only counters
  arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes
  arm64: KVM: Enable support for :G/:H perf event modifiers

 arch/arm64/include/asm/kvm_host.h | 20 +++++++++++++++++++
 arch/arm64/kernel/perf_event.c    | 42 ++++++++++++++++++++++++++++++++-------
 arch/arm64/kvm/hyp/switch.c       | 38 +++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] arm64: arm_pmu: remove unnecessary isb instruction

[Andrew Murray]

The armv8pmu_enable_event_counter function issues an isb instruction
after enabling a pair of counters - this doesn't provide any value
and is inconsistent with the armv8pmu_disable_event_counter.

In any case armv8pmu_enable_event_counter is always called with the
PMU stopped. Starting the PMU with armv8pmu_start results in an isb
instruction being issued.

Let's remove the unnecessary isb instruction.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kernel/perf_event.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 8e38d52..de564ae 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
 	armv8pmu_enable_counter(idx);
 	if (armv8pmu_event_is_chained(event))
 		armv8pmu_enable_counter(idx - 1);
-	isb();
 }
 
 static inline int armv8pmu_disable_counter(int idx)
-- 
2.7.4

[PATCH v2 1/4] arm64: arm_pmu: remove unnecessary isb instruction

[Andrew Murray]

The armv8pmu_enable_event_counter function issues an isb instruction
after enabling a pair of counters - this doesn't provide any value
and is inconsistent with the armv8pmu_disable_event_counter.

In any case armv8pmu_enable_event_counter is always called with the
PMU stopped. Starting the PMU with armv8pmu_start results in an isb
instruction being issued.

Let's remove the unnecessary isb instruction.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kernel/perf_event.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 8e38d52..de564ae 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
 	armv8pmu_enable_counter(idx);
 	if (armv8pmu_event_is_chained(event))
 		armv8pmu_enable_counter(idx - 1);
-	isb();
 }
 
 static inline int armv8pmu_disable_counter(int idx)
-- 
2.7.4

[PATCH v2 2/4] arm64: KVM: add accessors to track guest/host only counters

[Andrew Murray]

In order to effeciently enable/disable guest/host only perf counters
at guest entry/exit we add bitfields to kvm_cpu_context for guest and
host only events as well as accessors for updating them.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1550192..b6f998b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -203,6 +203,8 @@ struct kvm_cpu_context {
 	};
 
 	struct kvm_vcpu *__hyp_running_vcpu;
+	u32 events_host_only;
+	u32 events_guest_only;
 };
 
 typedef struct kvm_cpu_context kvm_cpu_context_t;
@@ -472,6 +474,24 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 {
 	return kvm_arch_vcpu_run_map_fp(vcpu);
 }
+static inline void kvm_set_clr_host_pmu_events(u32 clr, u32 set)
+{
+	kvm_cpu_context_t *ctx = this_cpu_ptr(&kvm_host_cpu_state);
+
+	ctx->events_host_only &= ~clr;
+	ctx->events_host_only |= set;
+}
+
+static inline void kvm_set_clr_guest_pmu_events(u32 clr, u32 set)
+{
+	kvm_cpu_context_t *ctx = this_cpu_ptr(&kvm_host_cpu_state);
+
+	ctx->events_guest_only &= ~clr;
+	ctx->events_guest_only |= set;
+}
+#else
+static inline void kvm_set_clr_host_pmu_events(u32 clr, u32 set) {}
+static inline void kvm_set_clr_guest_pmu_events(u32 clr, u32 set) {}
 #endif
 
 static inline void kvm_arm_vhe_guest_enter(void)
-- 
2.7.4

[PATCH v2 2/4] arm64: KVM: add accessors to track guest/host only counters

[Andrew Murray]

In order to effeciently enable/disable guest/host only perf counters
at guest entry/exit we add bitfields to kvm_cpu_context for guest and
host only events as well as accessors for updating them.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1550192..b6f998b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -203,6 +203,8 @@ struct kvm_cpu_context {
 	};
 
 	struct kvm_vcpu *__hyp_running_vcpu;
+	u32 events_host_only;
+	u32 events_guest_only;
 };
 
 typedef struct kvm_cpu_context kvm_cpu_context_t;
@@ -472,6 +474,24 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 {
 	return kvm_arch_vcpu_run_map_fp(vcpu);
 }
+static inline void kvm_set_clr_host_pmu_events(u32 clr, u32 set)
+{
+	kvm_cpu_context_t *ctx = this_cpu_ptr(&kvm_host_cpu_state);
+
+	ctx->events_host_only &= ~clr;
+	ctx->events_host_only |= set;
+}
+
+static inline void kvm_set_clr_guest_pmu_events(u32 clr, u32 set)
+{
+	kvm_cpu_context_t *ctx = this_cpu_ptr(&kvm_host_cpu_state);
+
+	ctx->events_guest_only &= ~clr;
+	ctx->events_guest_only |= set;
+}
+#else
+static inline void kvm_set_clr_host_pmu_events(u32 clr, u32 set) {}
+static inline void kvm_set_clr_guest_pmu_events(u32 clr, u32 set) {}
 #endif
 
 static inline void kvm_arm_vhe_guest_enter(void)
-- 
2.7.4

[PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

Add support for the :G and :H attributes in perf by handling the
exclude_host/exclude_guest event attributes.

We notify KVM of counters that we wish to be enabled or disabled on
guest entry/exit and thus defer from starting or stopping :G events
as per the events exclude_host attribute.

When using VHE, EL2 is unused by the guest - therefore we can filter
out these events with the PMU as per the 'exclude_host' attribute.

With both VHE and non-VHE we switch the counters between host/guest
at EL2. With non-VHE when using 'exclude_host' we filter out EL2.

These changes eliminate counters counting host events on the
boundaries of guest entry/exit when using :G. However when using :H
unless exclude_hv is set on non-VHE then there is a small blackout
window at the guest entry/exit where host events are not captured.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index de564ae..412bd80 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -26,6 +26,7 @@
 
 #include <linux/acpi.h>
 #include <linux/clocksource.h>
+#include <linux/kvm_host.h>
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
@@ -647,11 +648,23 @@ static inline int armv8pmu_enable_counter(int idx)
 
 static inline void armv8pmu_enable_event_counter(struct perf_event *event)
 {
+	struct perf_event_attr *attr = &event->attr;
 	int idx = event->hw.idx;
+	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
 
-	armv8pmu_enable_counter(idx);
 	if (armv8pmu_event_is_chained(event))
-		armv8pmu_enable_counter(idx - 1);
+		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
+
+	if (attr->exclude_host)
+		kvm_set_clr_guest_pmu_events(0, counter_bits);
+	if (attr->exclude_guest)
+		kvm_set_clr_host_pmu_events(0, counter_bits);
+
+	if (!attr->exclude_host) {
+		armv8pmu_enable_counter(idx);
+		if (armv8pmu_event_is_chained(event))
+			armv8pmu_enable_counter(idx - 1);
+	}
 }
 
 static inline int armv8pmu_disable_counter(int idx)
@@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
 static inline void armv8pmu_disable_event_counter(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
+	struct perf_event_attr *attr = &event->attr;
 	int idx = hwc->idx;
+	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
 
 	if (armv8pmu_event_is_chained(event))
-		armv8pmu_disable_counter(idx - 1);
-	armv8pmu_disable_counter(idx);
+		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
+
+	if (attr->exclude_host)
+		kvm_set_clr_guest_pmu_events(counter_bits, 0);
+	if (attr->exclude_guest)
+		kvm_set_clr_host_pmu_events(counter_bits, 0);
+
+	if (!attr->exclude_host) {
+		if (armv8pmu_event_is_chained(event))
+			armv8pmu_disable_counter(idx - 1);
+		armv8pmu_disable_counter(idx);
+	}
 }
 
 static inline int armv8pmu_enable_intens(int idx)
@@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
 	 * with other architectures (x86 and Power).
 	 */
 	if (is_kernel_in_hyp_mode()) {
-		if (!attr->exclude_kernel)
+		if (!attr->exclude_kernel && !attr->exclude_host)
 			config_base |= ARMV8_PMU_INCLUDE_EL2;
 	} else {
 		if (attr->exclude_kernel)
 			config_base |= ARMV8_PMU_EXCLUDE_EL1;
-		if (!attr->exclude_hv)
+		if (!attr->exclude_hv && !attr->exclude_host)
 			config_base |= ARMV8_PMU_INCLUDE_EL2;
 	}
 	if (attr->exclude_user)
@@ -976,6 +1001,10 @@ static void armv8pmu_reset(void *info)
 		armv8pmu_disable_intens(idx);
 	}
 
+	/* Clear the counters we flip at guest entry/exit */
+	kvm_set_clr_host_pmu_events(U32_MAX, 0);
+	kvm_set_clr_guest_pmu_events(U32_MAX, 0);
+
 	/*
 	 * Initialize & Reset PMNC. Request overflow interrupt for
 	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
-- 
2.7.4

[PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

Add support for the :G and :H attributes in perf by handling the
exclude_host/exclude_guest event attributes.

We notify KVM of counters that we wish to be enabled or disabled on
guest entry/exit and thus defer from starting or stopping :G events
as per the events exclude_host attribute.

When using VHE, EL2 is unused by the guest - therefore we can filter
out these events with the PMU as per the 'exclude_host' attribute.

With both VHE and non-VHE we switch the counters between host/guest
at EL2. With non-VHE when using 'exclude_host' we filter out EL2.

These changes eliminate counters counting host events on the
boundaries of guest entry/exit when using :G. However when using :H
unless exclude_hv is set on non-VHE then there is a small blackout
window at the guest entry/exit where host events are not captured.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index de564ae..412bd80 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -26,6 +26,7 @@
 
 #include <linux/acpi.h>
 #include <linux/clocksource.h>
+#include <linux/kvm_host.h>
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
@@ -647,11 +648,23 @@ static inline int armv8pmu_enable_counter(int idx)
 
 static inline void armv8pmu_enable_event_counter(struct perf_event *event)
 {
+	struct perf_event_attr *attr = &event->attr;
 	int idx = event->hw.idx;
+	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
 
-	armv8pmu_enable_counter(idx);
 	if (armv8pmu_event_is_chained(event))
-		armv8pmu_enable_counter(idx - 1);
+		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
+
+	if (attr->exclude_host)
+		kvm_set_clr_guest_pmu_events(0, counter_bits);
+	if (attr->exclude_guest)
+		kvm_set_clr_host_pmu_events(0, counter_bits);
+
+	if (!attr->exclude_host) {
+		armv8pmu_enable_counter(idx);
+		if (armv8pmu_event_is_chained(event))
+			armv8pmu_enable_counter(idx - 1);
+	}
 }
 
 static inline int armv8pmu_disable_counter(int idx)
@@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
 static inline void armv8pmu_disable_event_counter(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
+	struct perf_event_attr *attr = &event->attr;
 	int idx = hwc->idx;
+	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
 
 	if (armv8pmu_event_is_chained(event))
-		armv8pmu_disable_counter(idx - 1);
-	armv8pmu_disable_counter(idx);
+		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
+
+	if (attr->exclude_host)
+		kvm_set_clr_guest_pmu_events(counter_bits, 0);
+	if (attr->exclude_guest)
+		kvm_set_clr_host_pmu_events(counter_bits, 0);
+
+	if (!attr->exclude_host) {
+		if (armv8pmu_event_is_chained(event))
+			armv8pmu_disable_counter(idx - 1);
+		armv8pmu_disable_counter(idx);
+	}
 }
 
 static inline int armv8pmu_enable_intens(int idx)
@@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
 	 * with other architectures (x86 and Power).
 	 */
 	if (is_kernel_in_hyp_mode()) {
-		if (!attr->exclude_kernel)
+		if (!attr->exclude_kernel && !attr->exclude_host)
 			config_base |= ARMV8_PMU_INCLUDE_EL2;
 	} else {
 		if (attr->exclude_kernel)
 			config_base |= ARMV8_PMU_EXCLUDE_EL1;
-		if (!attr->exclude_hv)
+		if (!attr->exclude_hv && !attr->exclude_host)
 			config_base |= ARMV8_PMU_INCLUDE_EL2;
 	}
 	if (attr->exclude_user)
@@ -976,6 +1001,10 @@ static void armv8pmu_reset(void *info)
 		armv8pmu_disable_intens(idx);
 	}
 
+	/* Clear the counters we flip at guest entry/exit */
+	kvm_set_clr_host_pmu_events(U32_MAX, 0);
+	kvm_set_clr_guest_pmu_events(U32_MAX, 0);
+
 	/*
 	 * Initialize & Reset PMNC. Request overflow interrupt for
 	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
-- 
2.7.4

[PATCH v2 4/4] arm64: KVM: Enable support for :G/:H perf event modifiers

[Andrew Murray]

Enable/disable event counters as appropriate when entering and exiting
the guest to enable support for guest or host only event counting.

For both VHE and non-VHE we switch the counters between host/guest at
EL2. EL2 is filtered out by the PMU when we are using the :G modifier.

The PMU may be on when we change which counters are enabled however
we avoid adding an isb as we instead rely on existing context
synchronisation events: the isb in kvm_arm_vhe_guest_exit for VHE and
the eret from the hvc in kvm_call_hyp.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kvm/hyp/switch.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index d496ef5..ebf0aac 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -373,6 +373,32 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
 	return true;
 }
 
+static bool __hyp_text __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
+{
+	u32 host_only = host_ctxt->events_host_only;
+	u32 guest_only = host_ctxt->events_guest_only;
+
+	if (host_only)
+		write_sysreg(host_only, pmcntenclr_el0);
+
+	if (guest_only)
+		write_sysreg(guest_only, pmcntenset_el0);
+
+	return (host_only || guest_only);
+}
+
+static void __hyp_text __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt)
+{
+	u32 host_only = host_ctxt->events_host_only;
+	u32 guest_only = host_ctxt->events_guest_only;
+
+	if (guest_only)
+		write_sysreg(guest_only, pmcntenclr_el0);
+
+	if (host_only)
+		write_sysreg(host_only, pmcntenset_el0);
+}
+
 /*
  * Return true when we were able to fixup the guest exit and should return to
  * the guest, false when we should restore the host state and return to the
@@ -488,12 +514,15 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
+	bool pmu_switch_needed;
 	u64 exit_code;
 
 	host_ctxt = vcpu->arch.host_cpu_context;
 	host_ctxt->__hyp_running_vcpu = vcpu;
 	guest_ctxt = &vcpu->arch.ctxt;
 
+	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
+
 	sysreg_save_host_state_vhe(host_ctxt);
 
 	__activate_traps(vcpu);
@@ -524,6 +553,9 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 
 	__debug_switch_to_host(vcpu);
 
+	if (pmu_switch_needed)
+		__pmu_switch_to_host(host_ctxt);
+
 	return exit_code;
 }
 
@@ -532,6 +564,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
+	bool pmu_switch_needed;
 	u64 exit_code;
 
 	vcpu = kern_hyp_va(vcpu);
@@ -540,6 +573,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	host_ctxt->__hyp_running_vcpu = vcpu;
 	guest_ctxt = &vcpu->arch.ctxt;
 
+	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
+
 	__sysreg_save_state_nvhe(host_ctxt);
 
 	__activate_traps(vcpu);
@@ -586,6 +621,9 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	 */
 	__debug_switch_to_host(vcpu);
 
+	if (pmu_switch_needed)
+		__pmu_switch_to_host(host_ctxt);
+
 	return exit_code;
 }
 
-- 
2.7.4

[PATCH v2 4/4] arm64: KVM: Enable support for :G/:H perf event modifiers

[Andrew Murray]

Enable/disable event counters as appropriate when entering and exiting
the guest to enable support for guest or host only event counting.

For both VHE and non-VHE we switch the counters between host/guest at
EL2. EL2 is filtered out by the PMU when we are using the :G modifier.

The PMU may be on when we change which counters are enabled however
we avoid adding an isb as we instead rely on existing context
synchronisation events: the isb in kvm_arm_vhe_guest_exit for VHE and
the eret from the hvc in kvm_call_hyp.

Signed-off-by: Andrew Murray <andrew.murray@arm.com>
---
 arch/arm64/kvm/hyp/switch.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index d496ef5..ebf0aac 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -373,6 +373,32 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
 	return true;
 }
 
+static bool __hyp_text __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
+{
+	u32 host_only = host_ctxt->events_host_only;
+	u32 guest_only = host_ctxt->events_guest_only;
+
+	if (host_only)
+		write_sysreg(host_only, pmcntenclr_el0);
+
+	if (guest_only)
+		write_sysreg(guest_only, pmcntenset_el0);
+
+	return (host_only || guest_only);
+}
+
+static void __hyp_text __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt)
+{
+	u32 host_only = host_ctxt->events_host_only;
+	u32 guest_only = host_ctxt->events_guest_only;
+
+	if (guest_only)
+		write_sysreg(guest_only, pmcntenclr_el0);
+
+	if (host_only)
+		write_sysreg(host_only, pmcntenset_el0);
+}
+
 /*
  * Return true when we were able to fixup the guest exit and should return to
  * the guest, false when we should restore the host state and return to the
@@ -488,12 +514,15 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
+	bool pmu_switch_needed;
 	u64 exit_code;
 
 	host_ctxt = vcpu->arch.host_cpu_context;
 	host_ctxt->__hyp_running_vcpu = vcpu;
 	guest_ctxt = &vcpu->arch.ctxt;
 
+	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
+
 	sysreg_save_host_state_vhe(host_ctxt);
 
 	__activate_traps(vcpu);
@@ -524,6 +553,9 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 
 	__debug_switch_to_host(vcpu);
 
+	if (pmu_switch_needed)
+		__pmu_switch_to_host(host_ctxt);
+
 	return exit_code;
 }
 
@@ -532,6 +564,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
+	bool pmu_switch_needed;
 	u64 exit_code;
 
 	vcpu = kern_hyp_va(vcpu);
@@ -540,6 +573,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	host_ctxt->__hyp_running_vcpu = vcpu;
 	guest_ctxt = &vcpu->arch.ctxt;
 
+	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
+
 	__sysreg_save_state_nvhe(host_ctxt);
 
 	__activate_traps(vcpu);
@@ -586,6 +621,9 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	 */
 	__debug_switch_to_host(vcpu);
 
+	if (pmu_switch_needed)
+		__pmu_switch_to_host(host_ctxt);
+
 	return exit_code;
 }
 
-- 
2.7.4

Re: [PATCH v2 1/4] arm64: arm_pmu: remove unnecessary isb instruction

[Suzuki K Poulose]

On 11/20/2018 02:15 PM, Andrew Murray wrote:
> The armv8pmu_enable_event_counter function issues an isb instruction
> after enabling a pair of counters - this doesn't provide any value
> and is inconsistent with the armv8pmu_disable_event_counter.
> 
> In any case armv8pmu_enable_event_counter is always called with the
> PMU stopped. Starting the PMU with armv8pmu_start results in an isb
> instruction being issued.
> 
> Let's remove the unnecessary isb instruction.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>   arch/arm64/kernel/perf_event.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 8e38d52..de564ae 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
>   	armv8pmu_enable_counter(idx);
>   	if (armv8pmu_event_is_chained(event))
>   		armv8pmu_enable_counter(idx - 1);
> -	isb();
>   }
>   
>   static inline int armv8pmu_disable_counter(int idx)
> 

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

[PATCH v2 1/4] arm64: arm_pmu: remove unnecessary isb instruction

[Suzuki K Poulose]

On 11/20/2018 02:15 PM, Andrew Murray wrote:
> The armv8pmu_enable_event_counter function issues an isb instruction
> after enabling a pair of counters - this doesn't provide any value
> and is inconsistent with the armv8pmu_disable_event_counter.
> 
> In any case armv8pmu_enable_event_counter is always called with the
> PMU stopped. Starting the PMU with armv8pmu_start results in an isb
> instruction being issued.
> 
> Let's remove the unnecessary isb instruction.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>   arch/arm64/kernel/perf_event.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 8e38d52..de564ae 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
>   	armv8pmu_enable_counter(idx);
>   	if (armv8pmu_event_is_chained(event))
>   		armv8pmu_enable_counter(idx - 1);
> -	isb();
>   }
>   
>   static inline int armv8pmu_disable_counter(int idx)
> 

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Re: [PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Suzuki K Poulose]

On 11/20/2018 02:15 PM, Andrew Murray wrote:
> Add support for the :G and :H attributes in perf by handling the
> exclude_host/exclude_guest event attributes.
> 
> We notify KVM of counters that we wish to be enabled or disabled on
> guest entry/exit and thus defer from starting or stopping :G events
> as per the events exclude_host attribute.
> 
> When using VHE, EL2 is unused by the guest - therefore we can filter
> out these events with the PMU as per the 'exclude_host' attribute.
> 
> With both VHE and non-VHE we switch the counters between host/guest
> at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
> 
> These changes eliminate counters counting host events on the
> boundaries of guest entry/exit when using :G. However when using :H
> unless exclude_hv is set on non-VHE then there is a small blackout
> window at the guest entry/exit where host events are not captured.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>   arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
>   1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index de564ae..412bd80 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -26,6 +26,7 @@
>   
>   #include <linux/acpi.h>
>   #include <linux/clocksource.h>
> +#include <linux/kvm_host.h>
>   #include <linux/of.h>
>   #include <linux/perf/arm_pmu.h>
>   #include <linux/platform_device.h>
> @@ -647,11 +648,23 @@ static inline int armv8pmu_enable_counter(int idx)
>   
>   static inline void armv8pmu_enable_event_counter(struct perf_event *event)
>   {
> +	struct perf_event_attr *attr = &event->attr;
>   	int idx = event->hw.idx;
> +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
>   
> -	armv8pmu_enable_counter(idx);
>   	if (armv8pmu_event_is_chained(event))
> -		armv8pmu_enable_counter(idx - 1);
> +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));

minor nit: If you rearrange the code below a bit

> +
> +	if (attr->exclude_host)
> +		kvm_set_clr_guest_pmu_events(0, counter_bits);
> +	if (attr->exclude_guest)
> +		kvm_set_clr_host_pmu_events(0, counter_bits);
> +
> +	if (!attr->exclude_host) {
> +		armv8pmu_enable_counter(idx);
> +		if (armv8pmu_event_is_chained(event))
> +			armv8pmu_enable_counter(idx - 1);

we could have :

	if (attr->exclude_guest)
		kvm_set_clr_host_pmu_events(0, counter_bits);

	if (attr->exclude_host) {
		kvm_set_clr_guest_pmu_events(0, counter_bits);
		return;
	}

	armv8pmu_enable_counter(idx);
	if (armv8pmu_event_is_chained(event))
		armv8pmu_enable_counter(idx - 1);

Similarly for disable_event_counter.
	
> +	}
>   }
>   
>   static inline int armv8pmu_disable_counter(int idx)
> @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
>   static inline void armv8pmu_disable_event_counter(struct perf_event *event)
>   {
>   	struct hw_perf_event *hwc = &event->hw;
> +	struct perf_event_attr *attr = &event->attr;
>   	int idx = hwc->idx;
> +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
>   
>   	if (armv8pmu_event_is_chained(event))
> -		armv8pmu_disable_counter(idx - 1);
> -	armv8pmu_disable_counter(idx);
> +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> +
> +	if (attr->exclude_host)
> +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
> +	if (attr->exclude_guest)
> +		kvm_set_clr_host_pmu_events(counter_bits, 0);
> +
> +	if (!attr->exclude_host) {
> +		if (armv8pmu_event_is_chained(event))
> +			armv8pmu_disable_counter(idx - 1);
> +		armv8pmu_disable_counter(idx);
> +	}
>   }
>   
>   static inline int armv8pmu_enable_intens(int idx)
> @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>   	 * with other architectures (x86 and Power).
>   	 */
>   	if (is_kernel_in_hyp_mode()) {
> -		if (!attr->exclude_kernel)
> +		if (!attr->exclude_kernel && !attr->exclude_host)
>   			config_base |= ARMV8_PMU_INCLUDE_EL2;

Shouldn't we handle "exclude_kernel" for a "Guest" event ?
i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
the "exclude_kernel" apply to the event filtering after we enter
guest and thus, we need to set the EXCLUDE_EL1 ?

Also I am wondering what is the situation with Nested virtualisation
coming in. i.e, if the host hyp wanted to profile the guest hyp, should
we set EL2 events ? I understand this is something which should be
solved with the nested virt changes. But it would be good to see
if we could filter "exclude_host" and "exclude_guest" at the hypervisor
level (i.e, in software, without PMU filtering) to allow the normal
controls to make use of the hardware filtering ?

>   	} else {
>   		if (attr->exclude_kernel)
>   			config_base |= ARMV8_PMU_EXCLUDE_EL1;
> -		if (!attr->exclude_hv)
> +		if (!attr->exclude_hv && !attr->exclude_host)
>   			config_base |= ARMV8_PMU_INCLUDE_EL2;
>   	}


>   	if (attr->exclude_user)
> @@ -976,6 +1001,10 @@ static void armv8pmu_reset(void *info)
>   		armv8pmu_disable_intens(idx);
>   	}
>   
> +	/* Clear the counters we flip at guest entry/exit */
> +	kvm_set_clr_host_pmu_events(U32_MAX, 0);
> +	kvm_set_clr_guest_pmu_events(U32_MAX, 0);
> +
>   	/*
>   	 * Initialize & Reset PMNC. Request overflow interrupt for
>   	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
> 

Suzuki

[PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Suzuki K Poulose]

On 11/20/2018 02:15 PM, Andrew Murray wrote:
> Add support for the :G and :H attributes in perf by handling the
> exclude_host/exclude_guest event attributes.
> 
> We notify KVM of counters that we wish to be enabled or disabled on
> guest entry/exit and thus defer from starting or stopping :G events
> as per the events exclude_host attribute.
> 
> When using VHE, EL2 is unused by the guest - therefore we can filter
> out these events with the PMU as per the 'exclude_host' attribute.
> 
> With both VHE and non-VHE we switch the counters between host/guest
> at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
> 
> These changes eliminate counters counting host events on the
> boundaries of guest entry/exit when using :G. However when using :H
> unless exclude_hv is set on non-VHE then there is a small blackout
> window at the guest entry/exit where host events are not captured.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>   arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
>   1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index de564ae..412bd80 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -26,6 +26,7 @@
>   
>   #include <linux/acpi.h>
>   #include <linux/clocksource.h>
> +#include <linux/kvm_host.h>
>   #include <linux/of.h>
>   #include <linux/perf/arm_pmu.h>
>   #include <linux/platform_device.h>
> @@ -647,11 +648,23 @@ static inline int armv8pmu_enable_counter(int idx)
>   
>   static inline void armv8pmu_enable_event_counter(struct perf_event *event)
>   {
> +	struct perf_event_attr *attr = &event->attr;
>   	int idx = event->hw.idx;
> +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
>   
> -	armv8pmu_enable_counter(idx);
>   	if (armv8pmu_event_is_chained(event))
> -		armv8pmu_enable_counter(idx - 1);
> +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));

minor nit: If you rearrange the code below a bit

> +
> +	if (attr->exclude_host)
> +		kvm_set_clr_guest_pmu_events(0, counter_bits);
> +	if (attr->exclude_guest)
> +		kvm_set_clr_host_pmu_events(0, counter_bits);
> +
> +	if (!attr->exclude_host) {
> +		armv8pmu_enable_counter(idx);
> +		if (armv8pmu_event_is_chained(event))
> +			armv8pmu_enable_counter(idx - 1);

we could have :

	if (attr->exclude_guest)
		kvm_set_clr_host_pmu_events(0, counter_bits);

	if (attr->exclude_host) {
		kvm_set_clr_guest_pmu_events(0, counter_bits);
		return;
	}

	armv8pmu_enable_counter(idx);
	if (armv8pmu_event_is_chained(event))
		armv8pmu_enable_counter(idx - 1);

Similarly for disable_event_counter.
	
> +	}
>   }
>   
>   static inline int armv8pmu_disable_counter(int idx)
> @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
>   static inline void armv8pmu_disable_event_counter(struct perf_event *event)
>   {
>   	struct hw_perf_event *hwc = &event->hw;
> +	struct perf_event_attr *attr = &event->attr;
>   	int idx = hwc->idx;
> +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
>   
>   	if (armv8pmu_event_is_chained(event))
> -		armv8pmu_disable_counter(idx - 1);
> -	armv8pmu_disable_counter(idx);
> +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> +
> +	if (attr->exclude_host)
> +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
> +	if (attr->exclude_guest)
> +		kvm_set_clr_host_pmu_events(counter_bits, 0);
> +
> +	if (!attr->exclude_host) {
> +		if (armv8pmu_event_is_chained(event))
> +			armv8pmu_disable_counter(idx - 1);
> +		armv8pmu_disable_counter(idx);
> +	}
>   }
>   
>   static inline int armv8pmu_enable_intens(int idx)
> @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>   	 * with other architectures (x86 and Power).
>   	 */
>   	if (is_kernel_in_hyp_mode()) {
> -		if (!attr->exclude_kernel)
> +		if (!attr->exclude_kernel && !attr->exclude_host)
>   			config_base |= ARMV8_PMU_INCLUDE_EL2;

Shouldn't we handle "exclude_kernel" for a "Guest" event ?
i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
the "exclude_kernel" apply to the event filtering after we enter
guest and thus, we need to set the EXCLUDE_EL1 ?

Also I am wondering what is the situation with Nested virtualisation
coming in. i.e, if the host hyp wanted to profile the guest hyp, should
we set EL2 events ? I understand this is something which should be
solved with the nested virt changes. But it would be good to see
if we could filter "exclude_host" and "exclude_guest" at the hypervisor
level (i.e, in software, without PMU filtering) to allow the normal
controls to make use of the hardware filtering ?

>   	} else {
>   		if (attr->exclude_kernel)
>   			config_base |= ARMV8_PMU_EXCLUDE_EL1;
> -		if (!attr->exclude_hv)
> +		if (!attr->exclude_hv && !attr->exclude_host)
>   			config_base |= ARMV8_PMU_INCLUDE_EL2;
>   	}


>   	if (attr->exclude_user)
> @@ -976,6 +1001,10 @@ static void armv8pmu_reset(void *info)
>   		armv8pmu_disable_intens(idx);
>   	}
>   
> +	/* Clear the counters we flip at guest entry/exit */
> +	kvm_set_clr_host_pmu_events(U32_MAX, 0);
> +	kvm_set_clr_guest_pmu_events(U32_MAX, 0);
> +
>   	/*
>   	 * Initialize & Reset PMNC. Request overflow interrupt for
>   	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
> 

Suzuki

Re: [PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

On Tue, Nov 20, 2018 at 02:49:05PM +0000, Suzuki K Poulose wrote:
> On 11/20/2018 02:15 PM, Andrew Murray wrote:
> > Add support for the :G and :H attributes in perf by handling the
> > exclude_host/exclude_guest event attributes.
> > 
> > We notify KVM of counters that we wish to be enabled or disabled on
> > guest entry/exit and thus defer from starting or stopping :G events
> > as per the events exclude_host attribute.
> > 
> > When using VHE, EL2 is unused by the guest - therefore we can filter
> > out these events with the PMU as per the 'exclude_host' attribute.
> > 
> > With both VHE and non-VHE we switch the counters between host/guest
> > at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
> > 
> > These changes eliminate counters counting host events on the
> > boundaries of guest entry/exit when using :G. However when using :H
> > unless exclude_hv is set on non-VHE then there is a small blackout
> > window at the guest entry/exit where host events are not captured.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > ---
> >   arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
> >   1 file changed, 35 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > index de564ae..412bd80 100644
> > --- a/arch/arm64/kernel/perf_event.c
> > +++ b/arch/arm64/kernel/perf_event.c
> > @@ -26,6 +26,7 @@
> >   #include <linux/acpi.h>
> >   #include <linux/clocksource.h>
> > +#include <linux/kvm_host.h>
> >   #include <linux/of.h>
> >   #include <linux/perf/arm_pmu.h>
> >   #include <linux/platform_device.h>
> > @@ -647,11 +648,23 @@ static inline int armv8pmu_enable_counter(int idx)
> >   static inline void armv8pmu_enable_event_counter(struct perf_event *event)
> >   {
> > +	struct perf_event_attr *attr = &event->attr;
> >   	int idx = event->hw.idx;
> > +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
> > -	armv8pmu_enable_counter(idx);
> >   	if (armv8pmu_event_is_chained(event))
> > -		armv8pmu_enable_counter(idx - 1);
> > +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> 
> minor nit: If you rearrange the code below a bit
> 
> > +
> > +	if (attr->exclude_host)
> > +		kvm_set_clr_guest_pmu_events(0, counter_bits);
> > +	if (attr->exclude_guest)
> > +		kvm_set_clr_host_pmu_events(0, counter_bits);
> > +
> > +	if (!attr->exclude_host) {
> > +		armv8pmu_enable_counter(idx);
> > +		if (armv8pmu_event_is_chained(event))
> > +			armv8pmu_enable_counter(idx - 1);
> 
> we could have :
> 
> 	if (attr->exclude_guest)
> 		kvm_set_clr_host_pmu_events(0, counter_bits);
> 
> 	if (attr->exclude_host) {
> 		kvm_set_clr_guest_pmu_events(0, counter_bits);
> 		return;
> 	}
> 
> 	armv8pmu_enable_counter(idx);
> 	if (armv8pmu_event_is_chained(event))
> 		armv8pmu_enable_counter(idx - 1);
> 
> Similarly for disable_event_counter.

I'm not really sure this is more readable. It makes the symmetric
clr_{guest|host} calls asymmetric. And moves the condition for
enabling counters away from the code that conditionally enables
it. Though it does get rid of one if statement...

> 	
> > +	}
> >   }
> >   static inline int armv8pmu_disable_counter(int idx)
> > @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
> >   static inline void armv8pmu_disable_event_counter(struct perf_event *event)
> >   {
> >   	struct hw_perf_event *hwc = &event->hw;
> > +	struct perf_event_attr *attr = &event->attr;
> >   	int idx = hwc->idx;
> > +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
> >   	if (armv8pmu_event_is_chained(event))
> > -		armv8pmu_disable_counter(idx - 1);
> > -	armv8pmu_disable_counter(idx);
> > +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> > +
> > +	if (attr->exclude_host)
> > +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
> > +	if (attr->exclude_guest)
> > +		kvm_set_clr_host_pmu_events(counter_bits, 0);
> > +
> > +	if (!attr->exclude_host) {
> > +		if (armv8pmu_event_is_chained(event))
> > +			armv8pmu_disable_counter(idx - 1);
> > +		armv8pmu_disable_counter(idx);
> > +	}
> >   }
> >   static inline int armv8pmu_enable_intens(int idx)
> > @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> >   	 * with other architectures (x86 and Power).
> >   	 */
> >   	if (is_kernel_in_hyp_mode()) {
> > -		if (!attr->exclude_kernel)
> > +		if (!attr->exclude_kernel && !attr->exclude_host)
> >   			config_base |= ARMV8_PMU_INCLUDE_EL2;
> 
> Shouldn't we handle "exclude_kernel" for a "Guest" event ?
> i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
> the "exclude_kernel" apply to the event filtering after we enter
> guest and thus, we need to set the EXCLUDE_EL1 ?

Yes you're right. This is a problem not just for a VHE host's guest but
for the host as well - for example perf -e instructions:h and
-e instructions:G will currently give the same count.

> 
> Also I am wondering what is the situation with Nested virtualisation
> coming in. i.e, if the host hyp wanted to profile the guest hyp, should
> we set EL2 events ? I understand this is something which should be
> solved with the nested virt changes. But it would be good to see
> if we could filter "exclude_host" and "exclude_guest" at the hypervisor
> level (i.e, in software, without PMU filtering) to allow the normal
> controls to make use of the hardware filtering ?

It took me a while to think this through and I think you're right in
that we can do more to future proof this for nested virt. In fact we
don't depend on the hunks in this function (i.e. addition of extra
'!attr->exclude_host' conditions) - we don't need them due to the
enable/disable of counters at guest entry/exit.

With the assumption of the hypervisor switch enabling/disabling
host/guest counters I think we can now do the following:

        /*
         * If we're running in hyp mode, then we *are* the hypervisor.
         * Therefore we ignore exclude_hv in this configuration, since
         * there's no hypervisor to sample anyway. This is consistent
         * with other architectures (x86 and Power).
         */
        if (is_kernel_in_hyp_mode())
                if (!attr->exclude_kernel)
                        config_base |= ARMV8_PMU_INCLUDE_EL2;
        else
                if (!attr->exclude_hv)
                        config_base |= ARMV8_PMU_INCLUDE_EL2;

        if (attr->exclude_kernel)
                config_base |= ARMV8_PMU_EXCLUDE_EL1;

        if (attr->exclude_user)
                config_base |= ARMV8_PMU_EXCLUDE_EL0;

Also for nested virt we'd need to update
kvm_pmu_set_counter_event_type to ensure exclude_kernel is set when
the guest attempts to include EL2 to count a VHE guest kernel.

Does this look right?

Thanks,

Andrew Murray

> 
> >   	} else {
> >   		if (attr->exclude_kernel)
> >   			config_base |= ARMV8_PMU_EXCLUDE_EL1;
> > -		if (!attr->exclude_hv)
> > +		if (!attr->exclude_hv && !attr->exclude_host)
> >   			config_base |= ARMV8_PMU_INCLUDE_EL2;
> >   	}
> 
> 
> >   	if (attr->exclude_user)
> > @@ -976,6 +1001,10 @@ static void armv8pmu_reset(void *info)
> >   		armv8pmu_disable_intens(idx);
> >   	}
> > +	/* Clear the counters we flip at guest entry/exit */
> > +	kvm_set_clr_host_pmu_events(U32_MAX, 0);
> > +	kvm_set_clr_guest_pmu_events(U32_MAX, 0);
> > +
> >   	/*
> >   	 * Initialize & Reset PMNC. Request overflow interrupt for
> >   	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
> > 
> 
> Suzuki

[PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

On Tue, Nov 20, 2018 at 02:49:05PM +0000, Suzuki K Poulose wrote:
> On 11/20/2018 02:15 PM, Andrew Murray wrote:
> > Add support for the :G and :H attributes in perf by handling the
> > exclude_host/exclude_guest event attributes.
> > 
> > We notify KVM of counters that we wish to be enabled or disabled on
> > guest entry/exit and thus defer from starting or stopping :G events
> > as per the events exclude_host attribute.
> > 
> > When using VHE, EL2 is unused by the guest - therefore we can filter
> > out these events with the PMU as per the 'exclude_host' attribute.
> > 
> > With both VHE and non-VHE we switch the counters between host/guest
> > at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
> > 
> > These changes eliminate counters counting host events on the
> > boundaries of guest entry/exit when using :G. However when using :H
> > unless exclude_hv is set on non-VHE then there is a small blackout
> > window at the guest entry/exit where host events are not captured.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > ---
> >   arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
> >   1 file changed, 35 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > index de564ae..412bd80 100644
> > --- a/arch/arm64/kernel/perf_event.c
> > +++ b/arch/arm64/kernel/perf_event.c
> > @@ -26,6 +26,7 @@
> >   #include <linux/acpi.h>
> >   #include <linux/clocksource.h>
> > +#include <linux/kvm_host.h>
> >   #include <linux/of.h>
> >   #include <linux/perf/arm_pmu.h>
> >   #include <linux/platform_device.h>
> > @@ -647,11 +648,23 @@ static inline int armv8pmu_enable_counter(int idx)
> >   static inline void armv8pmu_enable_event_counter(struct perf_event *event)
> >   {
> > +	struct perf_event_attr *attr = &event->attr;
> >   	int idx = event->hw.idx;
> > +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
> > -	armv8pmu_enable_counter(idx);
> >   	if (armv8pmu_event_is_chained(event))
> > -		armv8pmu_enable_counter(idx - 1);
> > +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> 
> minor nit: If you rearrange the code below a bit
> 
> > +
> > +	if (attr->exclude_host)
> > +		kvm_set_clr_guest_pmu_events(0, counter_bits);
> > +	if (attr->exclude_guest)
> > +		kvm_set_clr_host_pmu_events(0, counter_bits);
> > +
> > +	if (!attr->exclude_host) {
> > +		armv8pmu_enable_counter(idx);
> > +		if (armv8pmu_event_is_chained(event))
> > +			armv8pmu_enable_counter(idx - 1);
> 
> we could have :
> 
> 	if (attr->exclude_guest)
> 		kvm_set_clr_host_pmu_events(0, counter_bits);
> 
> 	if (attr->exclude_host) {
> 		kvm_set_clr_guest_pmu_events(0, counter_bits);
> 		return;
> 	}
> 
> 	armv8pmu_enable_counter(idx);
> 	if (armv8pmu_event_is_chained(event))
> 		armv8pmu_enable_counter(idx - 1);
> 
> Similarly for disable_event_counter.

I'm not really sure this is more readable. It makes the symmetric
clr_{guest|host} calls asymmetric. And moves the condition for
enabling counters away from the code that conditionally enables
it. Though it does get rid of one if statement...

> 	
> > +	}
> >   }
> >   static inline int armv8pmu_disable_counter(int idx)
> > @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
> >   static inline void armv8pmu_disable_event_counter(struct perf_event *event)
> >   {
> >   	struct hw_perf_event *hwc = &event->hw;
> > +	struct perf_event_attr *attr = &event->attr;
> >   	int idx = hwc->idx;
> > +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
> >   	if (armv8pmu_event_is_chained(event))
> > -		armv8pmu_disable_counter(idx - 1);
> > -	armv8pmu_disable_counter(idx);
> > +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> > +
> > +	if (attr->exclude_host)
> > +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
> > +	if (attr->exclude_guest)
> > +		kvm_set_clr_host_pmu_events(counter_bits, 0);
> > +
> > +	if (!attr->exclude_host) {
> > +		if (armv8pmu_event_is_chained(event))
> > +			armv8pmu_disable_counter(idx - 1);
> > +		armv8pmu_disable_counter(idx);
> > +	}
> >   }
> >   static inline int armv8pmu_enable_intens(int idx)
> > @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> >   	 * with other architectures (x86 and Power).
> >   	 */
> >   	if (is_kernel_in_hyp_mode()) {
> > -		if (!attr->exclude_kernel)
> > +		if (!attr->exclude_kernel && !attr->exclude_host)
> >   			config_base |= ARMV8_PMU_INCLUDE_EL2;
> 
> Shouldn't we handle "exclude_kernel" for a "Guest" event ?
> i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
> the "exclude_kernel" apply to the event filtering after we enter
> guest and thus, we need to set the EXCLUDE_EL1 ?

Yes you're right. This is a problem not just for a VHE host's guest but
for the host as well - for example perf -e instructions:h and
-e instructions:G will currently give the same count.

> 
> Also I am wondering what is the situation with Nested virtualisation
> coming in. i.e, if the host hyp wanted to profile the guest hyp, should
> we set EL2 events ? I understand this is something which should be
> solved with the nested virt changes. But it would be good to see
> if we could filter "exclude_host" and "exclude_guest" at the hypervisor
> level (i.e, in software, without PMU filtering) to allow the normal
> controls to make use of the hardware filtering ?

It took me a while to think this through and I think you're right in
that we can do more to future proof this for nested virt. In fact we
don't depend on the hunks in this function (i.e. addition of extra
'!attr->exclude_host' conditions) - we don't need them due to the
enable/disable of counters at guest entry/exit.

With the assumption of the hypervisor switch enabling/disabling
host/guest counters I think we can now do the following:

        /*
         * If we're running in hyp mode, then we *are* the hypervisor.
         * Therefore we ignore exclude_hv in this configuration, since
         * there's no hypervisor to sample anyway. This is consistent
         * with other architectures (x86 and Power).
         */
        if (is_kernel_in_hyp_mode())
                if (!attr->exclude_kernel)
                        config_base |= ARMV8_PMU_INCLUDE_EL2;
        else
                if (!attr->exclude_hv)
                        config_base |= ARMV8_PMU_INCLUDE_EL2;

        if (attr->exclude_kernel)
                config_base |= ARMV8_PMU_EXCLUDE_EL1;

        if (attr->exclude_user)
                config_base |= ARMV8_PMU_EXCLUDE_EL0;

Also for nested virt we'd need to update
kvm_pmu_set_counter_event_type to ensure exclude_kernel is set when
the guest attempts to include EL2 to count a VHE guest kernel.

Does this look right?

Thanks,

Andrew Murray

> 
> >   	} else {
> >   		if (attr->exclude_kernel)
> >   			config_base |= ARMV8_PMU_EXCLUDE_EL1;
> > -		if (!attr->exclude_hv)
> > +		if (!attr->exclude_hv && !attr->exclude_host)
> >   			config_base |= ARMV8_PMU_INCLUDE_EL2;
> >   	}
> 
> 
> >   	if (attr->exclude_user)
> > @@ -976,6 +1001,10 @@ static void armv8pmu_reset(void *info)
> >   		armv8pmu_disable_intens(idx);
> >   	}
> > +	/* Clear the counters we flip at guest entry/exit */
> > +	kvm_set_clr_host_pmu_events(U32_MAX, 0);
> > +	kvm_set_clr_guest_pmu_events(U32_MAX, 0);
> > +
> >   	/*
> >   	 * Initialize & Reset PMNC. Request overflow interrupt for
> >   	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
> > 
> 
> Suzuki

Re: [PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

On Wed, Nov 21, 2018 at 10:56:02AM +0000, Andrew Murray wrote:
> On Tue, Nov 20, 2018 at 02:49:05PM +0000, Suzuki K Poulose wrote:
> > On 11/20/2018 02:15 PM, Andrew Murray wrote:
> > > Add support for the :G and :H attributes in perf by handling the
> > > exclude_host/exclude_guest event attributes.
> > > 
> > > We notify KVM of counters that we wish to be enabled or disabled on
> > > guest entry/exit and thus defer from starting or stopping :G events
> > > as per the events exclude_host attribute.
> > > 
> > > When using VHE, EL2 is unused by the guest - therefore we can filter
> > > out these events with the PMU as per the 'exclude_host' attribute.
> > > 
> > > With both VHE and non-VHE we switch the counters between host/guest
> > > at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
> > > 
> > > These changes eliminate counters counting host events on the
> > > boundaries of guest entry/exit when using :G. However when using :H
> > > unless exclude_hv is set on non-VHE then there is a small blackout
> > > window at the guest entry/exit where host events are not captured.
> > > 
> > > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > > ---
> > >   arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
> 


> > 	
> > > +	}
> > >   }
> > >   static inline int armv8pmu_disable_counter(int idx)
> > > @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
> > >   static inline void armv8pmu_disable_event_counter(struct perf_event *event)
> > >   {
> > >   	struct hw_perf_event *hwc = &event->hw;
> > > +	struct perf_event_attr *attr = &event->attr;
> > >   	int idx = hwc->idx;
> > > +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
> > >   	if (armv8pmu_event_is_chained(event))
> > > -		armv8pmu_disable_counter(idx - 1);
> > > -	armv8pmu_disable_counter(idx);
> > > +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> > > +
> > > +	if (attr->exclude_host)
> > > +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
> > > +	if (attr->exclude_guest)
> > > +		kvm_set_clr_host_pmu_events(counter_bits, 0);
> > > +
> > > +	if (!attr->exclude_host) {
> > > +		if (armv8pmu_event_is_chained(event))
> > > +			armv8pmu_disable_counter(idx - 1);
> > > +		armv8pmu_disable_counter(idx);
> > > +	}
> > >   }
> > >   static inline int armv8pmu_enable_intens(int idx)
> > > @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> > >   	 * with other architectures (x86 and Power).
> > >   	 */
> > >   	if (is_kernel_in_hyp_mode()) {
> > > -		if (!attr->exclude_kernel)
> > > +		if (!attr->exclude_kernel && !attr->exclude_host)
> > >   			config_base |= ARMV8_PMU_INCLUDE_EL2;
> > 
> > Shouldn't we handle "exclude_kernel" for a "Guest" event ?
> > i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
> > the "exclude_kernel" apply to the event filtering after we enter
> > guest and thus, we need to set the EXCLUDE_EL1 ?
> 
> Yes you're right. This is a problem not just for a VHE host's guest but
> for the host as well - for example perf -e instructions:h and
> -e instructions:G will currently give the same count.
> 
> > 
> > Also I am wondering what is the situation with Nested virtualisation
> > coming in. i.e, if the host hyp wanted to profile the guest hyp, should
> > we set EL2 events ? I understand this is something which should be
> > solved with the nested virt changes. But it would be good to see
> > if we could filter "exclude_host" and "exclude_guest" at the hypervisor
> > level (i.e, in software, without PMU filtering) to allow the normal
> > controls to make use of the hardware filtering ?
> 
> It took me a while to think this through and I think you're right in
> that we can do more to future proof this for nested virt. In fact we
> don't depend on the hunks in this function (i.e. addition of extra
> '!attr->exclude_host' conditions) - we don't need them due to the
> enable/disable of counters at guest entry/exit.
> 
> With the assumption of the hypervisor switch enabling/disabling
> host/guest counters I think we can now do the following:
> 
>         /*
>          * If we're running in hyp mode, then we *are* the hypervisor.
>          * Therefore we ignore exclude_hv in this configuration, since
>          * there's no hypervisor to sample anyway. This is consistent
>          * with other architectures (x86 and Power).
>          */
>         if (is_kernel_in_hyp_mode())
>                 if (!attr->exclude_kernel)
>                         config_base |= ARMV8_PMU_INCLUDE_EL2;
>         else
>                 if (!attr->exclude_hv)
>                         config_base |= ARMV8_PMU_INCLUDE_EL2;
> 
>         if (attr->exclude_kernel)
>                 config_base |= ARMV8_PMU_EXCLUDE_EL1;
> 
>         if (attr->exclude_user)
>                 config_base |= ARMV8_PMU_EXCLUDE_EL0;
> 
> Also for nested virt we'd need to update
> kvm_pmu_set_counter_event_type to ensure exclude_kernel is set when
> the guest attempts to include EL2 to count a VHE guest kernel.
> 
> Does this look right?

Actually I think this is more correct:

        /*
         * If we're running in hyp mode, then we *are* the hypervisor.
         * Therefore we ignore exclude_hv in this configuration, since
         * there's no hypervisor to sample anyway. This is consistent
         * with other architectures (x86 and Power).
         *
         * To eliminate counting host events on the boundaries of
         * guest entry/exit we ensure EL2 is not included in hyp mode
         * with !exclude_host.
         */
        if (is_kernel_in_hyp_mode())
                if (!attr->exclude_kernel && !attr->exclude_host)
                        config_base |= ARMV8_PMU_INCLUDE_EL2;
        else
                if (!attr->exclude_hv)
                        config_base |= ARMV8_PMU_INCLUDE_EL2;

        /*
         * Filter out !VHE kernels and guest kernels
         */
        if (attr->exclude_kernel)
                config_base |= ARMV8_PMU_EXCLUDE_EL1;

        if (attr->exclude_user)
                config_base |= ARMV8_PMU_EXCLUDE_EL0;


The reason for re-adding the !attr->exclude_host is to prevent
leakage of host events getting counted when using guest_only and thus
prevents information exposing to the guest. On VHE we flip the counters
from host to guest shortly before entering the guest - if we don't
exclude EL2 then we start counting host time between the counter flip
and actually entering the guest. As the guests will always be EL1/EL0
we really should exclude EL2. I don't think this breaks any nested
virt use case as EL2 is only ever emulated right?

Thanks,

Andrew Murray

> 
> Thanks,
> 
> Andrew Murray
> 
> > 
> > >   	} else {
> > >   		if (attr->exclude_kernel)
> > >   			config_base |= ARMV8_PMU_EXCLUDE_EL1;
> > > -		if (!attr->exclude_hv)
> > > +		if (!attr->exclude_hv && !attr->exclude_host)
> > >   			config_base |= ARMV8_PMU_INCLUDE_EL2;
> > >   	}
> > 
> > 
> > >   	if (attr->exclude_user)
> > > @@ -976,6 +1001,10 @@ static void armv8pmu_reset(void *info)
> > >   		armv8pmu_disable_intens(idx);
> > >   	}
> > > +	/* Clear the counters we flip at guest entry/exit */
> > > +	kvm_set_clr_host_pmu_events(U32_MAX, 0);
> > > +	kvm_set_clr_guest_pmu_events(U32_MAX, 0);
> > > +
> > >   	/*
> > >   	 * Initialize & Reset PMNC. Request overflow interrupt for
> > >   	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
> > > 
> > 
> > Suzuki
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

On Wed, Nov 21, 2018 at 10:56:02AM +0000, Andrew Murray wrote:
> On Tue, Nov 20, 2018 at 02:49:05PM +0000, Suzuki K Poulose wrote:
> > On 11/20/2018 02:15 PM, Andrew Murray wrote:
> > > Add support for the :G and :H attributes in perf by handling the
> > > exclude_host/exclude_guest event attributes.
> > > 
> > > We notify KVM of counters that we wish to be enabled or disabled on
> > > guest entry/exit and thus defer from starting or stopping :G events
> > > as per the events exclude_host attribute.
> > > 
> > > When using VHE, EL2 is unused by the guest - therefore we can filter
> > > out these events with the PMU as per the 'exclude_host' attribute.
> > > 
> > > With both VHE and non-VHE we switch the counters between host/guest
> > > at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
> > > 
> > > These changes eliminate counters counting host events on the
> > > boundaries of guest entry/exit when using :G. However when using :H
> > > unless exclude_hv is set on non-VHE then there is a small blackout
> > > window at the guest entry/exit where host events are not captured.
> > > 
> > > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > > ---
> > >   arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
> 


> > 	
> > > +	}
> > >   }
> > >   static inline int armv8pmu_disable_counter(int idx)
> > > @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
> > >   static inline void armv8pmu_disable_event_counter(struct perf_event *event)
> > >   {
> > >   	struct hw_perf_event *hwc = &event->hw;
> > > +	struct perf_event_attr *attr = &event->attr;
> > >   	int idx = hwc->idx;
> > > +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
> > >   	if (armv8pmu_event_is_chained(event))
> > > -		armv8pmu_disable_counter(idx - 1);
> > > -	armv8pmu_disable_counter(idx);
> > > +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> > > +
> > > +	if (attr->exclude_host)
> > > +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
> > > +	if (attr->exclude_guest)
> > > +		kvm_set_clr_host_pmu_events(counter_bits, 0);
> > > +
> > > +	if (!attr->exclude_host) {
> > > +		if (armv8pmu_event_is_chained(event))
> > > +			armv8pmu_disable_counter(idx - 1);
> > > +		armv8pmu_disable_counter(idx);
> > > +	}
> > >   }
> > >   static inline int armv8pmu_enable_intens(int idx)
> > > @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> > >   	 * with other architectures (x86 and Power).
> > >   	 */
> > >   	if (is_kernel_in_hyp_mode()) {
> > > -		if (!attr->exclude_kernel)
> > > +		if (!attr->exclude_kernel && !attr->exclude_host)
> > >   			config_base |= ARMV8_PMU_INCLUDE_EL2;
> > 
> > Shouldn't we handle "exclude_kernel" for a "Guest" event ?
> > i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
> > the "exclude_kernel" apply to the event filtering after we enter
> > guest and thus, we need to set the EXCLUDE_EL1 ?
> 
> Yes you're right. This is a problem not just for a VHE host's guest but
> for the host as well - for example perf -e instructions:h and
> -e instructions:G will currently give the same count.
> 
> > 
> > Also I am wondering what is the situation with Nested virtualisation
> > coming in. i.e, if the host hyp wanted to profile the guest hyp, should
> > we set EL2 events ? I understand this is something which should be
> > solved with the nested virt changes. But it would be good to see
> > if we could filter "exclude_host" and "exclude_guest" at the hypervisor
> > level (i.e, in software, without PMU filtering) to allow the normal
> > controls to make use of the hardware filtering ?
> 
> It took me a while to think this through and I think you're right in
> that we can do more to future proof this for nested virt. In fact we
> don't depend on the hunks in this function (i.e. addition of extra
> '!attr->exclude_host' conditions) - we don't need them due to the
> enable/disable of counters at guest entry/exit.
> 
> With the assumption of the hypervisor switch enabling/disabling
> host/guest counters I think we can now do the following:
> 
>         /*
>          * If we're running in hyp mode, then we *are* the hypervisor.
>          * Therefore we ignore exclude_hv in this configuration, since
>          * there's no hypervisor to sample anyway. This is consistent
>          * with other architectures (x86 and Power).
>          */
>         if (is_kernel_in_hyp_mode())
>                 if (!attr->exclude_kernel)
>                         config_base |= ARMV8_PMU_INCLUDE_EL2;
>         else
>                 if (!attr->exclude_hv)
>                         config_base |= ARMV8_PMU_INCLUDE_EL2;
> 
>         if (attr->exclude_kernel)
>                 config_base |= ARMV8_PMU_EXCLUDE_EL1;
> 
>         if (attr->exclude_user)
>                 config_base |= ARMV8_PMU_EXCLUDE_EL0;
> 
> Also for nested virt we'd need to update
> kvm_pmu_set_counter_event_type to ensure exclude_kernel is set when
> the guest attempts to include EL2 to count a VHE guest kernel.
> 
> Does this look right?

Actually I think this is more correct:

        /*
         * If we're running in hyp mode, then we *are* the hypervisor.
         * Therefore we ignore exclude_hv in this configuration, since
         * there's no hypervisor to sample anyway. This is consistent
         * with other architectures (x86 and Power).
         *
         * To eliminate counting host events on the boundaries of
         * guest entry/exit we ensure EL2 is not included in hyp mode
         * with !exclude_host.
         */
        if (is_kernel_in_hyp_mode())
                if (!attr->exclude_kernel && !attr->exclude_host)
                        config_base |= ARMV8_PMU_INCLUDE_EL2;
        else
                if (!attr->exclude_hv)
                        config_base |= ARMV8_PMU_INCLUDE_EL2;

        /*
         * Filter out !VHE kernels and guest kernels
         */
        if (attr->exclude_kernel)
                config_base |= ARMV8_PMU_EXCLUDE_EL1;

        if (attr->exclude_user)
                config_base |= ARMV8_PMU_EXCLUDE_EL0;


The reason for re-adding the !attr->exclude_host is to prevent
leakage of host events getting counted when using guest_only and thus
prevents information exposing to the guest. On VHE we flip the counters
from host to guest shortly before entering the guest - if we don't
exclude EL2 then we start counting host time between the counter flip
and actually entering the guest. As the guests will always be EL1/EL0
we really should exclude EL2. I don't think this breaks any nested
virt use case as EL2 is only ever emulated right?

Thanks,

Andrew Murray

> 
> Thanks,
> 
> Andrew Murray
> 
> > 
> > >   	} else {
> > >   		if (attr->exclude_kernel)
> > >   			config_base |= ARMV8_PMU_EXCLUDE_EL1;
> > > -		if (!attr->exclude_hv)
> > > +		if (!attr->exclude_hv && !attr->exclude_host)
> > >   			config_base |= ARMV8_PMU_INCLUDE_EL2;
> > >   	}
> > 
> > 
> > >   	if (attr->exclude_user)
> > > @@ -976,6 +1001,10 @@ static void armv8pmu_reset(void *info)
> > >   		armv8pmu_disable_intens(idx);
> > >   	}
> > > +	/* Clear the counters we flip at guest entry/exit */
> > > +	kvm_set_clr_host_pmu_events(U32_MAX, 0);
> > > +	kvm_set_clr_guest_pmu_events(U32_MAX, 0);
> > > +
> > >   	/*
> > >   	 * Initialize & Reset PMNC. Request overflow interrupt for
> > >   	 * 64 bit cycle counter but cheat in armv8pmu_write_counter().
> > > 
> > 
> > Suzuki
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] arm64: arm_pmu: remove unnecessary isb instruction

[Mark Rutland]

On Tue, Nov 20, 2018 at 02:15:19PM +0000, Andrew Murray wrote:
> The armv8pmu_enable_event_counter function issues an isb instruction
> after enabling a pair of counters - this doesn't provide any value
> and is inconsistent with the armv8pmu_disable_event_counter.
> 
> In any case armv8pmu_enable_event_counter is always called with the
> PMU stopped. Starting the PMU with armv8pmu_start results in an isb
> instruction being issued.
> 
> Let's remove the unnecessary isb instruction.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

... though it would be nice to explicitly point out that the ISB in 
armv8pmu_start() is before the write to PMCR_EL1, if you happen to
respin this patch.

Mark.

> ---
>  arch/arm64/kernel/perf_event.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 8e38d52..de564ae 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
>  	armv8pmu_enable_counter(idx);
>  	if (armv8pmu_event_is_chained(event))
>  		armv8pmu_enable_counter(idx - 1);
> -	isb();
>  }
>  
>  static inline int armv8pmu_disable_counter(int idx)
> -- 
> 2.7.4
> 

[PATCH v2 1/4] arm64: arm_pmu: remove unnecessary isb instruction

[Mark Rutland]

On Tue, Nov 20, 2018 at 02:15:19PM +0000, Andrew Murray wrote:
> The armv8pmu_enable_event_counter function issues an isb instruction
> after enabling a pair of counters - this doesn't provide any value
> and is inconsistent with the armv8pmu_disable_event_counter.
> 
> In any case armv8pmu_enable_event_counter is always called with the
> PMU stopped. Starting the PMU with armv8pmu_start results in an isb
> instruction being issued.
> 
> Let's remove the unnecessary isb instruction.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

... though it would be nice to explicitly point out that the ISB in 
armv8pmu_start() is before the write to PMCR_EL1, if you happen to
respin this patch.

Mark.

> ---
>  arch/arm64/kernel/perf_event.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 8e38d52..de564ae 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
>  	armv8pmu_enable_counter(idx);
>  	if (armv8pmu_event_is_chained(event))
>  		armv8pmu_enable_counter(idx - 1);
> -	isb();
>  }
>  
>  static inline int armv8pmu_disable_counter(int idx)
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/4] arm64: KVM: add accessors to track guest/host only counters

[Marc Zyngier]

On 20/11/2018 14:15, Andrew Murray wrote:
> In order to effeciently enable/disable guest/host only perf counters
> at guest entry/exit we add bitfields to kvm_cpu_context for guest and
> host only events as well as accessors for updating them.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1550192..b6f998b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -203,6 +203,8 @@ struct kvm_cpu_context {
>  	};
>  
>  	struct kvm_vcpu *__hyp_running_vcpu;
> +	u32 events_host_only;
> +	u32 events_guest_only;
>  };
>  
>  typedef struct kvm_cpu_context kvm_cpu_context_t;
> @@ -472,6 +474,24 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>  {
>  	return kvm_arch_vcpu_run_map_fp(vcpu);
>  }
> +static inline void kvm_set_clr_host_pmu_events(u32 clr, u32 set)

nit: keep the arguments in an order that match the function name (saves
a lot of brain power...)

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v2 2/4] arm64: KVM: add accessors to track guest/host only counters

[Marc Zyngier]

On 20/11/2018 14:15, Andrew Murray wrote:
> In order to effeciently enable/disable guest/host only perf counters
> at guest entry/exit we add bitfields to kvm_cpu_context for guest and
> host only events as well as accessors for updating them.
> 
> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1550192..b6f998b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -203,6 +203,8 @@ struct kvm_cpu_context {
>  	};
>  
>  	struct kvm_vcpu *__hyp_running_vcpu;
> +	u32 events_host_only;
> +	u32 events_guest_only;
>  };
>  
>  typedef struct kvm_cpu_context kvm_cpu_context_t;
> @@ -472,6 +474,24 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>  {
>  	return kvm_arch_vcpu_run_map_fp(vcpu);
>  }
> +static inline void kvm_set_clr_host_pmu_events(u32 clr, u32 set)

nit: keep the arguments in an order that match the function name (saves
a lot of brain power...)

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Suzuki K Poulose]

On 21/11/2018 13:23, Andrew Murray wrote:
> On Wed, Nov 21, 2018 at 10:56:02AM +0000, Andrew Murray wrote:
>> On Tue, Nov 20, 2018 at 02:49:05PM +0000, Suzuki K Poulose wrote:
>>> On 11/20/2018 02:15 PM, Andrew Murray wrote:
>>>> Add support for the :G and :H attributes in perf by handling the
>>>> exclude_host/exclude_guest event attributes.
>>>>
>>>> We notify KVM of counters that we wish to be enabled or disabled on
>>>> guest entry/exit and thus defer from starting or stopping :G events
>>>> as per the events exclude_host attribute.
>>>>
>>>> When using VHE, EL2 is unused by the guest - therefore we can filter
>>>> out these events with the PMU as per the 'exclude_host' attribute.
>>>>
>>>> With both VHE and non-VHE we switch the counters between host/guest
>>>> at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
>>>>
>>>> These changes eliminate counters counting host events on the
>>>> boundaries of guest entry/exit when using :G. However when using :H
>>>> unless exclude_hv is set on non-VHE then there is a small blackout
>>>> window at the guest entry/exit where host events are not captured.
>>>>
>>>> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
>>>> ---
>>>>    arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
>>
> 
> 
>>> 	
>>>> +	}
>>>>    }
>>>>    static inline int armv8pmu_disable_counter(int idx)
>>>> @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
>>>>    static inline void armv8pmu_disable_event_counter(struct perf_event *event)
>>>>    {
>>>>    	struct hw_perf_event *hwc = &event->hw;
>>>> +	struct perf_event_attr *attr = &event->attr;
>>>>    	int idx = hwc->idx;
>>>> +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
>>>>    	if (armv8pmu_event_is_chained(event))
>>>> -		armv8pmu_disable_counter(idx - 1);
>>>> -	armv8pmu_disable_counter(idx);
>>>> +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
>>>> +
>>>> +	if (attr->exclude_host)
>>>> +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
>>>> +	if (attr->exclude_guest)
>>>> +		kvm_set_clr_host_pmu_events(counter_bits, 0);
>>>> +
>>>> +	if (!attr->exclude_host) {
>>>> +		if (armv8pmu_event_is_chained(event))
>>>> +			armv8pmu_disable_counter(idx - 1);
>>>> +		armv8pmu_disable_counter(idx);
>>>> +	}
>>>>    }
>>>>    static inline int armv8pmu_enable_intens(int idx)
>>>> @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>>>>    	 * with other architectures (x86 and Power).
>>>>    	 */
>>>>    	if (is_kernel_in_hyp_mode()) {
>>>> -		if (!attr->exclude_kernel)
>>>> +		if (!attr->exclude_kernel && !attr->exclude_host)
>>>>    			config_base |= ARMV8_PMU_INCLUDE_EL2;
>>>
>>> Shouldn't we handle "exclude_kernel" for a "Guest" event ?
>>> i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
>>> the "exclude_kernel" apply to the event filtering after we enter
>>> guest and thus, we need to set the EXCLUDE_EL1 ?
>>
>> Yes you're right. This is a problem not just for a VHE host's guest but
>> for the host as well - for example perf -e instructions:h and
>> -e instructions:G will currently give the same count.
>>
>>>
>>> Also I am wondering what is the situation with Nested virtualisation
>>> coming in. i.e, if the host hyp wanted to profile the guest hyp, should
>>> we set EL2 events ? I understand this is something which should be
>>> solved with the nested virt changes. But it would be good to see
>>> if we could filter "exclude_host" and "exclude_guest" at the hypervisor
>>> level (i.e, in software, without PMU filtering) to allow the normal
>>> controls to make use of the hardware filtering ?
>>
>> It took me a while to think this through and I think you're right in
>> that we can do more to future proof this for nested virt. In fact we
>> don't depend on the hunks in this function (i.e. addition of extra
>> '!attr->exclude_host' conditions) - we don't need them due to the
>> enable/disable of counters at guest entry/exit.
>>
>> With the assumption of the hypervisor switch enabling/disabling
>> host/guest counters I think we can now do the following:
>>
>>          /*
>>           * If we're running in hyp mode, then we *are* the hypervisor.
>>           * Therefore we ignore exclude_hv in this configuration, since
>>           * there's no hypervisor to sample anyway. This is consistent
>>           * with other architectures (x86 and Power).
>>           */
>>          if (is_kernel_in_hyp_mode())
>>                  if (!attr->exclude_kernel)
>>                          config_base |= ARMV8_PMU_INCLUDE_EL2;
>>          else
>>                  if (!attr->exclude_hv)
>>                          config_base |= ARMV8_PMU_INCLUDE_EL2;
>>
>>          if (attr->exclude_kernel)
>>                  config_base |= ARMV8_PMU_EXCLUDE_EL1;
>>
>>          if (attr->exclude_user)
>>                  config_base |= ARMV8_PMU_EXCLUDE_EL0;
>>
>> Also for nested virt we'd need to update
>> kvm_pmu_set_counter_event_type to ensure exclude_kernel is set when
>> the guest attempts to include EL2 to count a VHE guest kernel.
>>
>> Does this look right?
> 
> Actually I think this is more correct:
> 


>          /*
>           * If we're running in hyp mode, then we *are* the hypervisor.
>           * Therefore we ignore exclude_hv in this configuration, since
>           * there's no hypervisor to sample anyway. This is consistent
>           * with other architectures (x86 and Power).
>           *
>           * To eliminate counting host events on the boundaries of
>           * guest entry/exit we ensure EL2 is not included in hyp mode
>           * with !exclude_host.
>           */
>          if (is_kernel_in_hyp_mode())
>                  if (!attr->exclude_kernel && !attr->exclude_host)
>                          config_base |= ARMV8_PMU_INCLUDE_EL2;

You beat me to it. I was thinking about it and was planning to respond
with the same suggestion.

>          else
>                  if (!attr->exclude_hv)
>                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> 
>          /*
>           * Filter out !VHE kernels and guest kernels
>           */
>          if (attr->exclude_kernel)
>                  config_base |= ARMV8_PMU_EXCLUDE_EL1;
> 
>          if (attr->exclude_user)
>                  config_base |= ARMV8_PMU_EXCLUDE_EL0;
> 
> 
> The reason for re-adding the !attr->exclude_host is to prevent
> leakage of host events getting counted when using guest_only and thus
> prevents information exposing to the guest. On VHE we flip the counters
> from host to guest shortly before entering the guest - if we don't
> exclude EL2 then we start counting host time between the counter flip
> and actually entering the guest. As the guests will always be EL1/EL0
> we really should exclude EL2. I don't think this breaks any nested
> virt use case as EL2 is only ever emulated right?

I am not sure about the Nested virt case. But I think this looks
fine with me.

Cheers
Suzuki

[PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Suzuki K Poulose]

On 21/11/2018 13:23, Andrew Murray wrote:
> On Wed, Nov 21, 2018 at 10:56:02AM +0000, Andrew Murray wrote:
>> On Tue, Nov 20, 2018 at 02:49:05PM +0000, Suzuki K Poulose wrote:
>>> On 11/20/2018 02:15 PM, Andrew Murray wrote:
>>>> Add support for the :G and :H attributes in perf by handling the
>>>> exclude_host/exclude_guest event attributes.
>>>>
>>>> We notify KVM of counters that we wish to be enabled or disabled on
>>>> guest entry/exit and thus defer from starting or stopping :G events
>>>> as per the events exclude_host attribute.
>>>>
>>>> When using VHE, EL2 is unused by the guest - therefore we can filter
>>>> out these events with the PMU as per the 'exclude_host' attribute.
>>>>
>>>> With both VHE and non-VHE we switch the counters between host/guest
>>>> at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
>>>>
>>>> These changes eliminate counters counting host events on the
>>>> boundaries of guest entry/exit when using :G. However when using :H
>>>> unless exclude_hv is set on non-VHE then there is a small blackout
>>>> window at the guest entry/exit where host events are not captured.
>>>>
>>>> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
>>>> ---
>>>>    arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
>>
> 
> 
>>> 	
>>>> +	}
>>>>    }
>>>>    static inline int armv8pmu_disable_counter(int idx)
>>>> @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
>>>>    static inline void armv8pmu_disable_event_counter(struct perf_event *event)
>>>>    {
>>>>    	struct hw_perf_event *hwc = &event->hw;
>>>> +	struct perf_event_attr *attr = &event->attr;
>>>>    	int idx = hwc->idx;
>>>> +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
>>>>    	if (armv8pmu_event_is_chained(event))
>>>> -		armv8pmu_disable_counter(idx - 1);
>>>> -	armv8pmu_disable_counter(idx);
>>>> +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
>>>> +
>>>> +	if (attr->exclude_host)
>>>> +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
>>>> +	if (attr->exclude_guest)
>>>> +		kvm_set_clr_host_pmu_events(counter_bits, 0);
>>>> +
>>>> +	if (!attr->exclude_host) {
>>>> +		if (armv8pmu_event_is_chained(event))
>>>> +			armv8pmu_disable_counter(idx - 1);
>>>> +		armv8pmu_disable_counter(idx);
>>>> +	}
>>>>    }
>>>>    static inline int armv8pmu_enable_intens(int idx)
>>>> @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>>>>    	 * with other architectures (x86 and Power).
>>>>    	 */
>>>>    	if (is_kernel_in_hyp_mode()) {
>>>> -		if (!attr->exclude_kernel)
>>>> +		if (!attr->exclude_kernel && !attr->exclude_host)
>>>>    			config_base |= ARMV8_PMU_INCLUDE_EL2;
>>>
>>> Shouldn't we handle "exclude_kernel" for a "Guest" event ?
>>> i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
>>> the "exclude_kernel" apply to the event filtering after we enter
>>> guest and thus, we need to set the EXCLUDE_EL1 ?
>>
>> Yes you're right. This is a problem not just for a VHE host's guest but
>> for the host as well - for example perf -e instructions:h and
>> -e instructions:G will currently give the same count.
>>
>>>
>>> Also I am wondering what is the situation with Nested virtualisation
>>> coming in. i.e, if the host hyp wanted to profile the guest hyp, should
>>> we set EL2 events ? I understand this is something which should be
>>> solved with the nested virt changes. But it would be good to see
>>> if we could filter "exclude_host" and "exclude_guest" at the hypervisor
>>> level (i.e, in software, without PMU filtering) to allow the normal
>>> controls to make use of the hardware filtering ?
>>
>> It took me a while to think this through and I think you're right in
>> that we can do more to future proof this for nested virt. In fact we
>> don't depend on the hunks in this function (i.e. addition of extra
>> '!attr->exclude_host' conditions) - we don't need them due to the
>> enable/disable of counters at guest entry/exit.
>>
>> With the assumption of the hypervisor switch enabling/disabling
>> host/guest counters I think we can now do the following:
>>
>>          /*
>>           * If we're running in hyp mode, then we *are* the hypervisor.
>>           * Therefore we ignore exclude_hv in this configuration, since
>>           * there's no hypervisor to sample anyway. This is consistent
>>           * with other architectures (x86 and Power).
>>           */
>>          if (is_kernel_in_hyp_mode())
>>                  if (!attr->exclude_kernel)
>>                          config_base |= ARMV8_PMU_INCLUDE_EL2;
>>          else
>>                  if (!attr->exclude_hv)
>>                          config_base |= ARMV8_PMU_INCLUDE_EL2;
>>
>>          if (attr->exclude_kernel)
>>                  config_base |= ARMV8_PMU_EXCLUDE_EL1;
>>
>>          if (attr->exclude_user)
>>                  config_base |= ARMV8_PMU_EXCLUDE_EL0;
>>
>> Also for nested virt we'd need to update
>> kvm_pmu_set_counter_event_type to ensure exclude_kernel is set when
>> the guest attempts to include EL2 to count a VHE guest kernel.
>>
>> Does this look right?
> 
> Actually I think this is more correct:
> 


>          /*
>           * If we're running in hyp mode, then we *are* the hypervisor.
>           * Therefore we ignore exclude_hv in this configuration, since
>           * there's no hypervisor to sample anyway. This is consistent
>           * with other architectures (x86 and Power).
>           *
>           * To eliminate counting host events on the boundaries of
>           * guest entry/exit we ensure EL2 is not included in hyp mode
>           * with !exclude_host.
>           */
>          if (is_kernel_in_hyp_mode())
>                  if (!attr->exclude_kernel && !attr->exclude_host)
>                          config_base |= ARMV8_PMU_INCLUDE_EL2;

You beat me to it. I was thinking about it and was planning to respond
with the same suggestion.

>          else
>                  if (!attr->exclude_hv)
>                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> 
>          /*
>           * Filter out !VHE kernels and guest kernels
>           */
>          if (attr->exclude_kernel)
>                  config_base |= ARMV8_PMU_EXCLUDE_EL1;
> 
>          if (attr->exclude_user)
>                  config_base |= ARMV8_PMU_EXCLUDE_EL0;
> 
> 
> The reason for re-adding the !attr->exclude_host is to prevent
> leakage of host events getting counted when using guest_only and thus
> prevents information exposing to the guest. On VHE we flip the counters
> from host to guest shortly before entering the guest - if we don't
> exclude EL2 then we start counting host time between the counter flip
> and actually entering the guest. As the guests will always be EL1/EL0
> we really should exclude EL2. I don't think this breaks any nested
> virt use case as EL2 is only ever emulated right?

I am not sure about the Nested virt case. But I think this looks
fine with me.

Cheers
Suzuki

Re: [PATCH v2 1/4] arm64: arm_pmu: remove unnecessary isb instruction

[Andrew Murray]

On Wed, Nov 21, 2018 at 03:58:14PM +0000, Mark Rutland wrote:
> On Tue, Nov 20, 2018 at 02:15:19PM +0000, Andrew Murray wrote:
> > The armv8pmu_enable_event_counter function issues an isb instruction
> > after enabling a pair of counters - this doesn't provide any value
> > and is inconsistent with the armv8pmu_disable_event_counter.
> > 
> > In any case armv8pmu_enable_event_counter is always called with the
> > PMU stopped. Starting the PMU with armv8pmu_start results in an isb
> > instruction being issued.
> > 
> > Let's remove the unnecessary isb instruction.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> ... though it would be nice to explicitly point out that the ISB in 
> armv8pmu_start() is before the write to PMCR_EL1, if you happen to
> respin this patch.

Thanks, I'm respining so I'll update the log and preserve your ack.

Andrew Murray

> 
> Mark.
> 
> > ---
> >  arch/arm64/kernel/perf_event.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > index 8e38d52..de564ae 100644
> > --- a/arch/arm64/kernel/perf_event.c
> > +++ b/arch/arm64/kernel/perf_event.c
> > @@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
> >  	armv8pmu_enable_counter(idx);
> >  	if (armv8pmu_event_is_chained(event))
> >  		armv8pmu_enable_counter(idx - 1);
> > -	isb();
> >  }
> >  
> >  static inline int armv8pmu_disable_counter(int idx)
> > -- 
> > 2.7.4
> > 

[PATCH v2 1/4] arm64: arm_pmu: remove unnecessary isb instruction

[Andrew Murray]

On Wed, Nov 21, 2018 at 03:58:14PM +0000, Mark Rutland wrote:
> On Tue, Nov 20, 2018 at 02:15:19PM +0000, Andrew Murray wrote:
> > The armv8pmu_enable_event_counter function issues an isb instruction
> > after enabling a pair of counters - this doesn't provide any value
> > and is inconsistent with the armv8pmu_disable_event_counter.
> > 
> > In any case armv8pmu_enable_event_counter is always called with the
> > PMU stopped. Starting the PMU with armv8pmu_start results in an isb
> > instruction being issued.
> > 
> > Let's remove the unnecessary isb instruction.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> ... though it would be nice to explicitly point out that the ISB in 
> armv8pmu_start() is before the write to PMCR_EL1, if you happen to
> respin this patch.

Thanks, I'm respining so I'll update the log and preserve your ack.

Andrew Murray

> 
> Mark.
> 
> > ---
> >  arch/arm64/kernel/perf_event.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > index 8e38d52..de564ae 100644
> > --- a/arch/arm64/kernel/perf_event.c
> > +++ b/arch/arm64/kernel/perf_event.c
> > @@ -652,7 +652,6 @@ static inline void armv8pmu_enable_event_counter(struct perf_event *event)
> >  	armv8pmu_enable_counter(idx);
> >  	if (armv8pmu_event_is_chained(event))
> >  		armv8pmu_enable_counter(idx - 1);
> > -	isb();
> >  }
> >  
> >  static inline int armv8pmu_disable_counter(int idx)
> > -- 
> > 2.7.4
> > 

Re: [PATCH v2 2/4] arm64: KVM: add accessors to track guest/host only counters

[Andrew Murray]

On Wed, Nov 21, 2018 at 04:04:03PM +0000, Marc Zyngier wrote:
> On 20/11/2018 14:15, Andrew Murray wrote:
> > In order to effeciently enable/disable guest/host only perf counters
> > at guest entry/exit we add bitfields to kvm_cpu_context for guest and
> > host only events as well as accessors for updating them.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 1550192..b6f998b 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -203,6 +203,8 @@ struct kvm_cpu_context {
> >  	};
> >  
> >  	struct kvm_vcpu *__hyp_running_vcpu;
> > +	u32 events_host_only;
> > +	u32 events_guest_only;
> >  };
> >  
> >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > @@ -472,6 +474,24 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> >  {
> >  	return kvm_arch_vcpu_run_map_fp(vcpu);
> >  }
> > +static inline void kvm_set_clr_host_pmu_events(u32 clr, u32 set)
> 
> nit: keep the arguments in an order that match the function name (saves
> a lot of brain power...)

Thanks, every bit helps.

Andrew Murray

> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

[PATCH v2 2/4] arm64: KVM: add accessors to track guest/host only counters

[Andrew Murray]

On Wed, Nov 21, 2018 at 04:04:03PM +0000, Marc Zyngier wrote:
> On 20/11/2018 14:15, Andrew Murray wrote:
> > In order to effeciently enable/disable guest/host only perf counters
> > at guest entry/exit we add bitfields to kvm_cpu_context for guest and
> > host only events as well as accessors for updating them.
> > 
> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 1550192..b6f998b 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -203,6 +203,8 @@ struct kvm_cpu_context {
> >  	};
> >  
> >  	struct kvm_vcpu *__hyp_running_vcpu;
> > +	u32 events_host_only;
> > +	u32 events_guest_only;
> >  };
> >  
> >  typedef struct kvm_cpu_context kvm_cpu_context_t;
> > @@ -472,6 +474,24 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> >  {
> >  	return kvm_arch_vcpu_run_map_fp(vcpu);
> >  }
> > +static inline void kvm_set_clr_host_pmu_events(u32 clr, u32 set)
> 
> nit: keep the arguments in an order that match the function name (saves
> a lot of brain power...)

Thanks, every bit helps.

Andrew Murray

> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

Re: [PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

On Wed, Nov 21, 2018 at 05:01:47PM +0000, Suzuki K Poulose wrote:
> 
> 
> On 21/11/2018 13:23, Andrew Murray wrote:
> > On Wed, Nov 21, 2018 at 10:56:02AM +0000, Andrew Murray wrote:
> > > On Tue, Nov 20, 2018 at 02:49:05PM +0000, Suzuki K Poulose wrote:
> > > > On 11/20/2018 02:15 PM, Andrew Murray wrote:
> > > > > Add support for the :G and :H attributes in perf by handling the
> > > > > exclude_host/exclude_guest event attributes.
> > > > > 
> > > > > We notify KVM of counters that we wish to be enabled or disabled on
> > > > > guest entry/exit and thus defer from starting or stopping :G events
> > > > > as per the events exclude_host attribute.
> > > > > 
> > > > > When using VHE, EL2 is unused by the guest - therefore we can filter
> > > > > out these events with the PMU as per the 'exclude_host' attribute.
> > > > > 
> > > > > With both VHE and non-VHE we switch the counters between host/guest
> > > > > at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
> > > > > 
> > > > > These changes eliminate counters counting host events on the
> > > > > boundaries of guest entry/exit when using :G. However when using :H
> > > > > unless exclude_hv is set on non-VHE then there is a small blackout
> > > > > window at the guest entry/exit where host events are not captured.
> > > > > 
> > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > > > > ---
> > > > >    arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
> > > 
> > 
> > 
> > > > 	
> > > > > +	}
> > > > >    }
> > > > >    static inline int armv8pmu_disable_counter(int idx)
> > > > > @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
> > > > >    static inline void armv8pmu_disable_event_counter(struct perf_event *event)
> > > > >    {
> > > > >    	struct hw_perf_event *hwc = &event->hw;
> > > > > +	struct perf_event_attr *attr = &event->attr;
> > > > >    	int idx = hwc->idx;
> > > > > +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
> > > > >    	if (armv8pmu_event_is_chained(event))
> > > > > -		armv8pmu_disable_counter(idx - 1);
> > > > > -	armv8pmu_disable_counter(idx);
> > > > > +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> > > > > +
> > > > > +	if (attr->exclude_host)
> > > > > +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
> > > > > +	if (attr->exclude_guest)
> > > > > +		kvm_set_clr_host_pmu_events(counter_bits, 0);
> > > > > +
> > > > > +	if (!attr->exclude_host) {
> > > > > +		if (armv8pmu_event_is_chained(event))
> > > > > +			armv8pmu_disable_counter(idx - 1);
> > > > > +		armv8pmu_disable_counter(idx);
> > > > > +	}
> > > > >    }
> > > > >    static inline int armv8pmu_enable_intens(int idx)
> > > > > @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> > > > >    	 * with other architectures (x86 and Power).
> > > > >    	 */
> > > > >    	if (is_kernel_in_hyp_mode()) {
> > > > > -		if (!attr->exclude_kernel)
> > > > > +		if (!attr->exclude_kernel && !attr->exclude_host)
> > > > >    			config_base |= ARMV8_PMU_INCLUDE_EL2;
> > > > 
> > > > Shouldn't we handle "exclude_kernel" for a "Guest" event ?
> > > > i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
> > > > the "exclude_kernel" apply to the event filtering after we enter
> > > > guest and thus, we need to set the EXCLUDE_EL1 ?
> > > 
> > > Yes you're right. This is a problem not just for a VHE host's guest but
> > > for the host as well - for example perf -e instructions:h and
> > > -e instructions:G will currently give the same count.
> > > 
> > > > 
> > > > Also I am wondering what is the situation with Nested virtualisation
> > > > coming in. i.e, if the host hyp wanted to profile the guest hyp, should
> > > > we set EL2 events ? I understand this is something which should be
> > > > solved with the nested virt changes. But it would be good to see
> > > > if we could filter "exclude_host" and "exclude_guest" at the hypervisor
> > > > level (i.e, in software, without PMU filtering) to allow the normal
> > > > controls to make use of the hardware filtering ?
> > > 
> > > It took me a while to think this through and I think you're right in
> > > that we can do more to future proof this for nested virt. In fact we
> > > don't depend on the hunks in this function (i.e. addition of extra
> > > '!attr->exclude_host' conditions) - we don't need them due to the
> > > enable/disable of counters at guest entry/exit.
> > > 
> > > With the assumption of the hypervisor switch enabling/disabling
> > > host/guest counters I think we can now do the following:
> > > 
> > >          /*
> > >           * If we're running in hyp mode, then we *are* the hypervisor.
> > >           * Therefore we ignore exclude_hv in this configuration, since
> > >           * there's no hypervisor to sample anyway. This is consistent
> > >           * with other architectures (x86 and Power).
> > >           */
> > >          if (is_kernel_in_hyp_mode())
> > >                  if (!attr->exclude_kernel)
> > >                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> > >          else
> > >                  if (!attr->exclude_hv)
> > >                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> > > 
> > >          if (attr->exclude_kernel)
> > >                  config_base |= ARMV8_PMU_EXCLUDE_EL1;
> > > 
> > >          if (attr->exclude_user)
> > >                  config_base |= ARMV8_PMU_EXCLUDE_EL0;
> > > 
> > > Also for nested virt we'd need to update
> > > kvm_pmu_set_counter_event_type to ensure exclude_kernel is set when
> > > the guest attempts to include EL2 to count a VHE guest kernel.
> > > 
> > > Does this look right?
> > 
> > Actually I think this is more correct:
> > 
> 
> 
> >          /*
> >           * If we're running in hyp mode, then we *are* the hypervisor.
> >           * Therefore we ignore exclude_hv in this configuration, since
> >           * there's no hypervisor to sample anyway. This is consistent
> >           * with other architectures (x86 and Power).
> >           *
> >           * To eliminate counting host events on the boundaries of
> >           * guest entry/exit we ensure EL2 is not included in hyp mode
> >           * with !exclude_host.
> >           */
> >          if (is_kernel_in_hyp_mode())
> >                  if (!attr->exclude_kernel && !attr->exclude_host)
> >                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> 
> You beat me to it. I was thinking about it and was planning to respond
> with the same suggestion.
> 
> >          else
> >                  if (!attr->exclude_hv)
> >                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> > 
> >          /*
> >           * Filter out !VHE kernels and guest kernels
> >           */
> >          if (attr->exclude_kernel)
> >                  config_base |= ARMV8_PMU_EXCLUDE_EL1;
> > 
> >          if (attr->exclude_user)
> >                  config_base |= ARMV8_PMU_EXCLUDE_EL0;
> > 
> > 
> > The reason for re-adding the !attr->exclude_host is to prevent
> > leakage of host events getting counted when using guest_only and thus
> > prevents information exposing to the guest. On VHE we flip the counters
> > from host to guest shortly before entering the guest - if we don't
> > exclude EL2 then we start counting host time between the counter flip
> > and actually entering the guest. As the guests will always be EL1/EL0
> > we really should exclude EL2. I don't think this breaks any nested
> > virt use case as EL2 is only ever emulated right?
> 
> I am not sure about the Nested virt case. But I think this looks
> fine with me.

Thanks I'll respin this series.

Thanks,

Andrew Murray

> 
> Cheers
> Suzuki

[PATCH v2 3/4] arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes

[Andrew Murray]

On Wed, Nov 21, 2018 at 05:01:47PM +0000, Suzuki K Poulose wrote:
> 
> 
> On 21/11/2018 13:23, Andrew Murray wrote:
> > On Wed, Nov 21, 2018 at 10:56:02AM +0000, Andrew Murray wrote:
> > > On Tue, Nov 20, 2018 at 02:49:05PM +0000, Suzuki K Poulose wrote:
> > > > On 11/20/2018 02:15 PM, Andrew Murray wrote:
> > > > > Add support for the :G and :H attributes in perf by handling the
> > > > > exclude_host/exclude_guest event attributes.
> > > > > 
> > > > > We notify KVM of counters that we wish to be enabled or disabled on
> > > > > guest entry/exit and thus defer from starting or stopping :G events
> > > > > as per the events exclude_host attribute.
> > > > > 
> > > > > When using VHE, EL2 is unused by the guest - therefore we can filter
> > > > > out these events with the PMU as per the 'exclude_host' attribute.
> > > > > 
> > > > > With both VHE and non-VHE we switch the counters between host/guest
> > > > > at EL2. With non-VHE when using 'exclude_host' we filter out EL2.
> > > > > 
> > > > > These changes eliminate counters counting host events on the
> > > > > boundaries of guest entry/exit when using :G. However when using :H
> > > > > unless exclude_hv is set on non-VHE then there is a small blackout
> > > > > window at the guest entry/exit where host events are not captured.
> > > > > 
> > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > > > > ---
> > > > >    arch/arm64/kernel/perf_event.c | 41 +++++++++++++++++++++++++++++++++++------
> > > 
> > 
> > 
> > > > 	
> > > > > +	}
> > > > >    }
> > > > >    static inline int armv8pmu_disable_counter(int idx)
> > > > > @@ -664,11 +677,23 @@ static inline int armv8pmu_disable_counter(int idx)
> > > > >    static inline void armv8pmu_disable_event_counter(struct perf_event *event)
> > > > >    {
> > > > >    	struct hw_perf_event *hwc = &event->hw;
> > > > > +	struct perf_event_attr *attr = &event->attr;
> > > > >    	int idx = hwc->idx;
> > > > > +	u32 counter_bits = BIT(ARMV8_IDX_TO_COUNTER(idx));
> > > > >    	if (armv8pmu_event_is_chained(event))
> > > > > -		armv8pmu_disable_counter(idx - 1);
> > > > > -	armv8pmu_disable_counter(idx);
> > > > > +		counter_bits |= BIT(ARMV8_IDX_TO_COUNTER(idx - 1));
> > > > > +
> > > > > +	if (attr->exclude_host)
> > > > > +		kvm_set_clr_guest_pmu_events(counter_bits, 0);
> > > > > +	if (attr->exclude_guest)
> > > > > +		kvm_set_clr_host_pmu_events(counter_bits, 0);
> > > > > +
> > > > > +	if (!attr->exclude_host) {
> > > > > +		if (armv8pmu_event_is_chained(event))
> > > > > +			armv8pmu_disable_counter(idx - 1);
> > > > > +		armv8pmu_disable_counter(idx);
> > > > > +	}
> > > > >    }
> > > > >    static inline int armv8pmu_enable_intens(int idx)
> > > > > @@ -945,12 +970,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> > > > >    	 * with other architectures (x86 and Power).
> > > > >    	 */
> > > > >    	if (is_kernel_in_hyp_mode()) {
> > > > > -		if (!attr->exclude_kernel)
> > > > > +		if (!attr->exclude_kernel && !attr->exclude_host)
> > > > >    			config_base |= ARMV8_PMU_INCLUDE_EL2;
> > > > 
> > > > Shouldn't we handle "exclude_kernel" for a "Guest" event ?
> > > > i.e, what if we have exclude_kernel + exclude_host set ? Doesn't
> > > > the "exclude_kernel" apply to the event filtering after we enter
> > > > guest and thus, we need to set the EXCLUDE_EL1 ?
> > > 
> > > Yes you're right. This is a problem not just for a VHE host's guest but
> > > for the host as well - for example perf -e instructions:h and
> > > -e instructions:G will currently give the same count.
> > > 
> > > > 
> > > > Also I am wondering what is the situation with Nested virtualisation
> > > > coming in. i.e, if the host hyp wanted to profile the guest hyp, should
> > > > we set EL2 events ? I understand this is something which should be
> > > > solved with the nested virt changes. But it would be good to see
> > > > if we could filter "exclude_host" and "exclude_guest" at the hypervisor
> > > > level (i.e, in software, without PMU filtering) to allow the normal
> > > > controls to make use of the hardware filtering ?
> > > 
> > > It took me a while to think this through and I think you're right in
> > > that we can do more to future proof this for nested virt. In fact we
> > > don't depend on the hunks in this function (i.e. addition of extra
> > > '!attr->exclude_host' conditions) - we don't need them due to the
> > > enable/disable of counters at guest entry/exit.
> > > 
> > > With the assumption of the hypervisor switch enabling/disabling
> > > host/guest counters I think we can now do the following:
> > > 
> > >          /*
> > >           * If we're running in hyp mode, then we *are* the hypervisor.
> > >           * Therefore we ignore exclude_hv in this configuration, since
> > >           * there's no hypervisor to sample anyway. This is consistent
> > >           * with other architectures (x86 and Power).
> > >           */
> > >          if (is_kernel_in_hyp_mode())
> > >                  if (!attr->exclude_kernel)
> > >                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> > >          else
> > >                  if (!attr->exclude_hv)
> > >                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> > > 
> > >          if (attr->exclude_kernel)
> > >                  config_base |= ARMV8_PMU_EXCLUDE_EL1;
> > > 
> > >          if (attr->exclude_user)
> > >                  config_base |= ARMV8_PMU_EXCLUDE_EL0;
> > > 
> > > Also for nested virt we'd need to update
> > > kvm_pmu_set_counter_event_type to ensure exclude_kernel is set when
> > > the guest attempts to include EL2 to count a VHE guest kernel.
> > > 
> > > Does this look right?
> > 
> > Actually I think this is more correct:
> > 
> 
> 
> >          /*
> >           * If we're running in hyp mode, then we *are* the hypervisor.
> >           * Therefore we ignore exclude_hv in this configuration, since
> >           * there's no hypervisor to sample anyway. This is consistent
> >           * with other architectures (x86 and Power).
> >           *
> >           * To eliminate counting host events on the boundaries of
> >           * guest entry/exit we ensure EL2 is not included in hyp mode
> >           * with !exclude_host.
> >           */
> >          if (is_kernel_in_hyp_mode())
> >                  if (!attr->exclude_kernel && !attr->exclude_host)
> >                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> 
> You beat me to it. I was thinking about it and was planning to respond
> with the same suggestion.
> 
> >          else
> >                  if (!attr->exclude_hv)
> >                          config_base |= ARMV8_PMU_INCLUDE_EL2;
> > 
> >          /*
> >           * Filter out !VHE kernels and guest kernels
> >           */
> >          if (attr->exclude_kernel)
> >                  config_base |= ARMV8_PMU_EXCLUDE_EL1;
> > 
> >          if (attr->exclude_user)
> >                  config_base |= ARMV8_PMU_EXCLUDE_EL0;
> > 
> > 
> > The reason for re-adding the !attr->exclude_host is to prevent
> > leakage of host events getting counted when using guest_only and thus
> > prevents information exposing to the guest. On VHE we flip the counters
> > from host to guest shortly before entering the guest - if we don't
> > exclude EL2 then we start counting host time between the counter flip
> > and actually entering the guest. As the guests will always be EL1/EL0
> > we really should exclude EL2. I don't think this breaks any nested
> > virt use case as EL2 is only ever emulated right?
> 
> I am not sure about the Nested virt case. But I think this looks
> fine with me.

Thanks I'll respin this series.

Thanks,

Andrew Murray

> 
> Cheers
> Suzuki