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

[PATCH 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.

Andrew Murray (4):
  arm64: KVM: add accessors to track guest/host only counters
  arm64: arm_pmu: Add support for exclude_host/exclude_guest attributes
  arm64: arm_pmu: Exclude EL1,2 with :G :H perf 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    | 45 +++++++++++++++++++++++++++++++++------
 arch/arm64/kvm/hyp/switch.c       | 38 +++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH 1/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 2/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.

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

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 8e38d52..89d444f 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,12 +648,24 @@ 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);
-	isb();
+		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);
+		isb();
+	}
 }
 
 static inline int armv8pmu_disable_counter(int idx)
@@ -665,11 +678,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)
@@ -977,6 +1002,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 3/4] arm64: arm_pmu: Exclude EL1,2 with :G :H perf attributes

[Andrew Murray]

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

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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 89d444f..c079c1f 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -971,12 +971,14 @@ 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;
+		if (attr->exclude_guest)
+			config_base |= ARMV8_PMU_EXCLUDE_EL1;
 	} 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)
-- 
2.7.4

[PATCH 3/4] arm64: arm_pmu: Exclude EL1,2 with :G :H perf attributes

[Suzuki K Poulose]

Hi Andrew,

On 15/11/2018 12:55, Andrew Murray wrote:
> When using VHE, EL1 is unused by the host and EL2 is unused by the
> guest - therefore we can filter out these events with the PMU as per
> the 'exclude_host' and 'exclude_guest' attributes.
> 
> 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 | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 89d444f..c079c1f 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -971,12 +971,14 @@ 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;
> +		if (attr->exclude_guest)
> +			config_base |= ARMV8_PMU_EXCLUDE_EL1;

Do we really need this ? exclude_guest also implies you need to
exclude guest EL0. We anyway disable the events when we enter the
guest. So the above is not necessary and could possibly create
confusion.

>   	} 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)
> 


I think this can be folded into the previous patch, which adds the
support for exclude_host/guest support. :G, :H are nothing but
the those exclude_ flags.

Otherwise looks good to me.

Cheers
Suzuki

[PATCH 3/4] arm64: arm_pmu: Exclude EL1,2 with :G :H perf attributes

[Andrew Murray]

On Thu, Nov 15, 2018 at 02:57:59PM +0000, Suzuki K Poulose wrote:
> Hi Andrew,
> 
> On 15/11/2018 12:55, Andrew Murray wrote:
> > When using VHE, EL1 is unused by the host and EL2 is unused by the
> > guest - therefore we can filter out these events with the PMU as per
> > the 'exclude_host' and 'exclude_guest' attributes.
> > 
> > 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 | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > index 89d444f..c079c1f 100644
> > --- a/arch/arm64/kernel/perf_event.c
> > +++ b/arch/arm64/kernel/perf_event.c
> > @@ -971,12 +971,14 @@ 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;
> > +		if (attr->exclude_guest)
> > +			config_base |= ARMV8_PMU_EXCLUDE_EL1;
> 
> Do we really need this ? exclude_guest also implies you need to
> exclude guest EL0. We anyway disable the events when we enter the
> guest. So the above is not necessary and could possibly create
> confusion.

You're right, we disable the event anyway prior to entering EL1
and visa versa so there is no gain. It felt nice to utilise the
hardware as extra security but it does complicate this already
complex bit of logic. I'll drop it.

> 
> >   	} 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)
> > 
> 
> 
> I think this can be folded into the previous patch, which adds the
> support for exclude_host/guest support. :G, :H are nothing but
> the those exclude_ flags.

Thanks I'll do that.

Andrew Murray

> 
> Otherwise looks good to me.
> 
> Cheers
> Suzuki

[PATCH 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.

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 4/4] arm64: KVM: Enable support for :G/:H perf event modifiers

[Julien Thierry]

Hi Andrew,

On 15/11/18 12:55, Andrew Murray wrote:
> 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.
> 
> 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);

In the perf_event code, there is an ISB after enabling an event. I guess 
we don't need it when setting the guest events since I believe the eret 
to the guess give us the context synchronization. But don't we need one 
here when restoring host only events?

Thanks,

-- 
Julien Thierry

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

[Andrew Murray]

On Thu, Nov 15, 2018 at 02:00:39PM +0000, Julien Thierry wrote:
> Hi Andrew,
> 
> On 15/11/18 12:55, Andrew Murray wrote:
> > 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.
> > 
> > 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);
> 
> In the perf_event code, there is an ISB after enabling an event. I guess we
> don't need it when setting the guest events since I believe the eret to the
> guess give us the context synchronization. But don't we need one here when
> restoring host only events?

It's not really clear to me why the isb is present in the existing code,
this was only recently introduced when adding the chained events support.

Ideally for chained events you'd want to start the overflow counter first
(idx) followed by the low counter second (idx-1) as to not miss overflows
so an isb inbetween may be helpful. Though the isb is after both enables, this
sets a clear line of where event counting starts - but ideally this would be
symmetrical with an isb after the disable.

At present chained counters aren't supported in the guest but in any case
we turn them all on/off atomically rather than individually.

I guess we get a trivial gain in accuracy by adding ISB's at some performance
cost - I'm not sure I see the benefit - unless I'm missing something?

Thanks,

Andrew Murray

> 
> Thanks,
> 
> -- 
> Julien Thierry

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

[Suzuki K Poulose]

On 15/11/2018 15:57, Andrew Murray wrote:
> On Thu, Nov 15, 2018 at 02:00:39PM +0000, Julien Thierry wrote:
>> Hi Andrew,
>>
>> On 15/11/18 12:55, Andrew Murray wrote:
>>> 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.
>>>
>>> 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);
>>
>> In the perf_event code, there is an ISB after enabling an event. I guess we
>> don't need it when setting the guest events since I believe the eret to the
>> guess give us the context synchronization. But don't we need one here when
>> restoring host only events?
> 
> It's not really clear to me why the isb is present in the existing code,
> this was only recently introduced when adding the chained events support.
> 
> Ideally for chained events you'd want to start the overflow counter first
> (idx) followed by the low counter second (idx-1) as to not miss overflows
> so an isb inbetween may be helpful. Though the isb is after both enables, this
> sets a clear line of where event counting starts - but ideally this would be
> symmetrical with an isb after the disable.

I think the isb() in the armv8_pmu_enable_event_counter() is
unnecessary, and might have been a left over from earlier versions
of the series. Please feel free to remove it.

> 
> At present chained counters aren't supported in the guest but in any case
> we turn them all on/off atomically rather than individually.
> 
> I guess we get a trivial gain in accuracy by adding ISB's at some performance
> cost - I'm not sure I see the benefit - unless I'm missing something?

But, I think Julien has a valid point here. When we modify the
pmcnten{set/clr} registers, the PMU could be enabled. (i.e, PMCR_E set).

So in order to synchronize the changes to the counters, we need an isb()
in the switch to host case to take immediate effect of the counter
changes.

Cheers
Suzuki

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

[Andrew Murray]

On Thu, Nov 15, 2018 at 05:40:24PM +0000, Suzuki K Poulose wrote:
> 
> 
> On 15/11/2018 15:57, Andrew Murray wrote:
> > On Thu, Nov 15, 2018 at 02:00:39PM +0000, Julien Thierry wrote:
> > > Hi Andrew,
> > > 
> > > On 15/11/18 12:55, Andrew Murray wrote:
> > > > 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.
> > > > 
> > > > 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);
> > > 
> > > In the perf_event code, there is an ISB after enabling an event. I guess we
> > > don't need it when setting the guest events since I believe the eret to the
> > > guess give us the context synchronization. But don't we need one here when
> > > restoring host only events?
> > 
> > It's not really clear to me why the isb is present in the existing code,
> > this was only recently introduced when adding the chained events support.
> > 
> > Ideally for chained events you'd want to start the overflow counter first
> > (idx) followed by the low counter second (idx-1) as to not miss overflows
> > so an isb inbetween may be helpful. Though the isb is after both enables, this
> > sets a clear line of where event counting starts - but ideally this would be
> > symmetrical with an isb after the disable.
> 
> I think the isb() in the armv8_pmu_enable_event_counter() is
> unnecessary, and might have been a left over from earlier versions
> of the series. Please feel free to remove it.

OK I'll do that.

> 
> > 
> > At present chained counters aren't supported in the guest but in any case
> > we turn them all on/off atomically rather than individually.
> > 
> > I guess we get a trivial gain in accuracy by adding ISB's at some performance
> > cost - I'm not sure I see the benefit - unless I'm missing something?
> 
> But, I think Julien has a valid point here. When we modify the
> pmcnten{set/clr} registers, the PMU could be enabled. (i.e, PMCR_E set).
> 
> So in order to synchronize the changes to the counters, we need an isb()
> in the switch to host case to take immediate effect of the counter
> changes.

For VHE we already do an isb in kvm_arm_vhe_guest_exit (next line of code
to kvm_arm_vhe_guest_exit).

For !VHE as I understand we will eret from EL2 (due to kvm_call_hyp call
completing) and thus also implicitly isb.

If that's correct we don't need to add any isb's right?

Thanks,

Andrew Murray

> 
> Cheers
> Suzuki

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

[Suzuki K Poulose]

On 16/11/2018 12:12, Andrew Murray wrote:
> On Thu, Nov 15, 2018 at 05:40:24PM +0000, Suzuki K Poulose wrote:
>>
>>
>> On 15/11/2018 15:57, Andrew Murray wrote:
>>> On Thu, Nov 15, 2018 at 02:00:39PM +0000, Julien Thierry wrote:
>>>> Hi Andrew,
>>>>
>>>> On 15/11/18 12:55, Andrew Murray wrote:
>>>>> 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.
>>>>>
>>>>> 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);
>>>>
>>>> In the perf_event code, there is an ISB after enabling an event. I guess we
>>>> don't need it when setting the guest events since I believe the eret to the
>>>> guess give us the context synchronization. But don't we need one here when
>>>> restoring host only events?
>>>
>>> It's not really clear to me why the isb is present in the existing code,
>>> this was only recently introduced when adding the chained events support.
>>>
>>> Ideally for chained events you'd want to start the overflow counter first
>>> (idx) followed by the low counter second (idx-1) as to not miss overflows
>>> so an isb inbetween may be helpful. Though the isb is after both enables, this
>>> sets a clear line of where event counting starts - but ideally this would be
>>> symmetrical with an isb after the disable.
>>
>> I think the isb() in the armv8_pmu_enable_event_counter() is
>> unnecessary, and might have been a left over from earlier versions
>> of the series. Please feel free to remove it.
> 
> OK I'll do that.
> 
>>
>>>
>>> At present chained counters aren't supported in the guest but in any case
>>> we turn them all on/off atomically rather than individually.
>>>
>>> I guess we get a trivial gain in accuracy by adding ISB's at some performance
>>> cost - I'm not sure I see the benefit - unless I'm missing something?
>>
>> But, I think Julien has a valid point here. When we modify the
>> pmcnten{set/clr} registers, the PMU could be enabled. (i.e, PMCR_E set).
>>
>> So in order to synchronize the changes to the counters, we need an isb()
>> in the switch to host case to take immediate effect of the counter
>> changes.
> 
> For VHE we already do an isb in kvm_arm_vhe_guest_exit (next line of code
> to kvm_arm_vhe_guest_exit).
> 
> For !VHE as I understand we will eret from EL2 (due to kvm_call_hyp call
> completing) and thus also implicitly isb.
> 
> If that's correct we don't need to add any isb's right?

Yes, you're right. May be it is worth mentioning it where we switch to
host, so that we don't have to dig this again when we look at it later.

Cheers
Suzuki