Re: [PATCH v12 4/8] arm64: arm_pmu: Add !VHE support for exclude_host/exclude_guest attributes
On Thu, Apr 04, 2019 at 05:34:00PM +0100, Will Deacon wrote:
> On Thu, Mar 28, 2019 at 10:37:27AM +, 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 events based
> > on their event attributes.
> >
> > With !VHE we switch the counters between host/guest at EL2. We are able
> > to eliminate counters counting host events on the boundaries of guest
> > entry/exit when using :G by filtering out EL2 for exclude_host. When
> > using !exclude_hv there is a small blackout window at the guest
> > entry/exit where host events are not captured.
> >
> > Signed-off-by: Andrew Murray
> > ---
> > arch/arm64/kernel/perf_event.c | 43 --
> > 1 file changed, 36 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > index cccf4fc86d67..6bb28aaf5aea 100644
> > --- a/arch/arm64/kernel/perf_event.c
> > +++ b/arch/arm64/kernel/perf_event.c
> > @@ -26,6 +26,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -528,11 +529,21 @@ 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));
> > +
> > + kvm_set_pmu_events(counter_bits, attr);
> > +
> > + /* We rely on the hypervisor switch code to enable guest counters */
> > + if (!kvm_pmu_counter_deferred(attr)) {
> > + armv8pmu_enable_counter(idx);
> > + if (armv8pmu_event_is_chained(event))
> > + armv8pmu_enable_counter(idx - 1);
> > + }
> > }
> >
> > static inline int armv8pmu_disable_counter(int idx)
> > @@ -545,11 +556,21 @@ 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));
> > +
> > + kvm_clr_pmu_events(counter_bits);
> > +
> > + /* We rely on the hypervisor switch code to disable guest counters */
> > + if (!kvm_pmu_counter_deferred(attr)) {
> > + if (armv8pmu_event_is_chained(event))
> > + armv8pmu_disable_counter(idx - 1);
> > + armv8pmu_disable_counter(idx);
> > + }
> > }
> >
> > static inline int armv8pmu_enable_intens(int idx)
> > @@ -829,11 +850,16 @@ static int armv8pmu_set_event_filter(struct
> > hw_perf_event *event,
> > if (!attr->exclude_kernel)
> > 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;
>
> FWIW, that doesn't align with my personal view of what the host is, but
> I'm not going to argue if Marc and Christoffer are happy with it.
Ideally armv8pmu_set_event_filter needs no knowledge of exclude_guest
and exclude_host as we leave the config_base alone and instead rely on
turning the counters on/off at guest entry/exit...
The addition of "&& !attr->exclude_host" here (!VHE) and likewise for VHE in
"arm64: KVM: Enable VHE support for :G/:H perf event modifiers" (in case
you missed it) is to eliminate counters counting host events on the boundaries
of guest entry/exit when counting for guest_only (:G). Thus this is really
an optimisation for more accurate counting.
Consider the case where a user wants to record guest events only - we switch
over to the guest counters at EL2 on world-switch (both VHE and !VHE host) -
now there is a small period of time between when we start the guest counters
and when we actually jump into the guest. This period of time in the host
shouldn't be counted - therefore so long as exclude_host is set we can filter
EL2 to remove these unwanted counts.
Therefore the above hunks likely won't describe what our view of the host is.
>
> Anyway, thanks for persevering with this:
It's fun even though it hurts my head every time each time I come back
Re: [PATCH v12 4/8] arm64: arm_pmu: Add !VHE support for exclude_host/exclude_guest attributes
On Thu, Mar 28, 2019 at 10:37:27AM +, 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 events based
> on their event attributes.
>
> With !VHE we switch the counters between host/guest at EL2. We are able
> to eliminate counters counting host events on the boundaries of guest
> entry/exit when using :G by filtering out EL2 for exclude_host. When
> using !exclude_hv there is a small blackout window at the guest
> entry/exit where host events are not captured.
>
> Signed-off-by: Andrew Murray
> ---
> arch/arm64/kernel/perf_event.c | 43 --
> 1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index cccf4fc86d67..6bb28aaf5aea 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -26,6 +26,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -528,11 +529,21 @@ 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));
> +
> + kvm_set_pmu_events(counter_bits, attr);
> +
> + /* We rely on the hypervisor switch code to enable guest counters */
> + if (!kvm_pmu_counter_deferred(attr)) {
> + armv8pmu_enable_counter(idx);
> + if (armv8pmu_event_is_chained(event))
> + armv8pmu_enable_counter(idx - 1);
> + }
> }
>
> static inline int armv8pmu_disable_counter(int idx)
> @@ -545,11 +556,21 @@ 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));
> +
> + kvm_clr_pmu_events(counter_bits);
> +
> + /* We rely on the hypervisor switch code to disable guest counters */
> + if (!kvm_pmu_counter_deferred(attr)) {
> + if (armv8pmu_event_is_chained(event))
> + armv8pmu_disable_counter(idx - 1);
> + armv8pmu_disable_counter(idx);
> + }
> }
>
> static inline int armv8pmu_enable_intens(int idx)
> @@ -829,11 +850,16 @@ static int armv8pmu_set_event_filter(struct
> hw_perf_event *event,
> if (!attr->exclude_kernel)
> 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;
FWIW, that doesn't align with my personal view of what the host is, but
I'm not going to argue if Marc and Christoffer are happy with it.
Anyway, thanks for persevering with this:
Acked-by: Will Deacon
Will
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v12 4/8] arm64: arm_pmu: Add !VHE support for exclude_host/exclude_guest attributes
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 events based
on their event attributes.
With !VHE we switch the counters between host/guest at EL2. We are able
to eliminate counters counting host events on the boundaries of guest
entry/exit when using :G by filtering out EL2 for exclude_host. When
using !exclude_hv there is a small blackout window at the guest
entry/exit where host events are not captured.
Signed-off-by: Andrew Murray
---
arch/arm64/kernel/perf_event.c | 43 --
1 file changed, 36 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index cccf4fc86d67..6bb28aaf5aea 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -26,6 +26,7 @@
#include
#include
+#include
#include
#include
#include
@@ -528,11 +529,21 @@ 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));
+
+ kvm_set_pmu_events(counter_bits, attr);
+
+ /* We rely on the hypervisor switch code to enable guest counters */
+ if (!kvm_pmu_counter_deferred(attr)) {
+ armv8pmu_enable_counter(idx);
+ if (armv8pmu_event_is_chained(event))
+ armv8pmu_enable_counter(idx - 1);
+ }
}
static inline int armv8pmu_disable_counter(int idx)
@@ -545,11 +556,21 @@ 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));
+
+ kvm_clr_pmu_events(counter_bits);
+
+ /* We rely on the hypervisor switch code to disable guest counters */
+ if (!kvm_pmu_counter_deferred(attr)) {
+ if (armv8pmu_event_is_chained(event))
+ armv8pmu_disable_counter(idx - 1);
+ armv8pmu_disable_counter(idx);
+ }
}
static inline int armv8pmu_enable_intens(int idx)
@@ -829,11 +850,16 @@ static int armv8pmu_set_event_filter(struct hw_perf_event
*event,
if (!attr->exclude_kernel)
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;
}
+
+ /*
+* 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;
@@ -863,6 +889,9 @@ static void armv8pmu_reset(void *info)
armv8pmu_disable_intens(idx);
}
+ /* Clear the counters we flip at guest entry/exit */
+ kvm_clr_pmu_events(U32_MAX);
+
/*
* Initialize & Reset PMNC. Request overflow interrupt for
* 64 bit cycle counter but cheat in armv8pmu_write_counter().
--
2.21.0
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
