| Message ID | 1542885950-23816-3-git-send-email-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Support perf event modifiers :G and :H | expand |
On Thu, Nov 22, 2018 at 11:25:48AM +0000, 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..4a828eb 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_clr_set_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; > +} Do you actually need this clr/set function, or can you just use the facilities provided by bitfield.h at the callsites? Will
On Mon, Dec 03, 2018 at 02:31:14PM +0000, Will Deacon wrote: > On Thu, Nov 22, 2018 at 11:25:48AM +0000, 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..4a828eb 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_clr_set_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; > > +} > > Do you actually need this clr/set function, or can you just use the > facilities provided by bitfield.h at the callsites? This modifies events_{host|guest}_only which is conditionally defined (CONFIG_KVM) yet used by arch/arm64/kernel/perf_events which is also conditionally defined (CONFIG_HW_PERF_EVENTS). By moving this away from the callsite we avoid a load of horrible #ifdef's and allow this function to become a nop when KVM isn't enabled. Is there value in updating this function to use set_bit/clear_bit functions in bitops/atomic.h ? Andrew Murray > > Will
On Mon, Dec 03, 2018 at 03:05:52PM +0000, Andrew Murray wrote: > On Mon, Dec 03, 2018 at 02:31:14PM +0000, Will Deacon wrote: > > On Thu, Nov 22, 2018 at 11:25:48AM +0000, 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..4a828eb 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_clr_set_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; > > > +} > > > > Do you actually need this clr/set function, or can you just use the > > facilities provided by bitfield.h at the callsites? > > This modifies events_{host|guest}_only which is conditionally defined > (CONFIG_KVM) yet used by arch/arm64/kernel/perf_events which is also > conditionally defined (CONFIG_HW_PERF_EVENTS). By moving this away > from the callsite we avoid a load of horrible #ifdef's and allow this > function to become a nop when KVM isn't enabled. > > Is there value in updating this function to use set_bit/clear_bit > functions in bitops/atomic.h ? You don't need the atomicity, so you'd be looking at the '__' variants instead. My main concern is that we're introducing a clr_set_* API which only ever clears or sets, so I think it's needlessly complicated. If you need two separate macros that are just #defined to __set_bit/__clear_bit or expand to nothing then that could work too. Will
On Mon, Dec 03, 2018 at 04:58:43PM +0000, Will Deacon wrote: > On Mon, Dec 03, 2018 at 03:05:52PM +0000, Andrew Murray wrote: > > On Mon, Dec 03, 2018 at 02:31:14PM +0000, Will Deacon wrote: > > > On Thu, Nov 22, 2018 at 11:25:48AM +0000, 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..4a828eb 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_clr_set_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; > > > > +} > > > > > > Do you actually need this clr/set function, or can you just use the > > > facilities provided by bitfield.h at the callsites? > > > > This modifies events_{host|guest}_only which is conditionally defined > > (CONFIG_KVM) yet used by arch/arm64/kernel/perf_events which is also > > conditionally defined (CONFIG_HW_PERF_EVENTS). By moving this away > > from the callsite we avoid a load of horrible #ifdef's and allow this > > function to become a nop when KVM isn't enabled. > > > > Is there value in updating this function to use set_bit/clear_bit > > functions in bitops/atomic.h ? > > You don't need the atomicity, so you'd be looking at the '__' variants > instead. My main concern is that we're introducing a clr_set_* API which > only ever clears or sets, so I think it's needlessly complicated. If you > need two separate macros that are just #defined to __set_bit/__clear_bit > or expand to nothing then that could work too. Though it would be 4 macros - a set and clear for both events_host and events_guest. Though with the proposed changes in the other patch, this could end up being 3 macros seeing as we'd always clear both events_host and events_guest at the same time. Thanks, Andrew Murray > > Will
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 1550192..4a828eb 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_clr_set_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_clr_set_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_clr_set_host_pmu_events(u32 clr, u32 set) {} +static inline void kvm_clr_set_guest_pmu_events(u32 clr, u32 set) {} #endif static inline void kvm_arm_vhe_guest_enter(void)
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(+)