Re: [PATCH v2 2/2] KVM: arm64: PMU: Don't overwrite PMUSERENR with vcpu loaded

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Apr 11, 2023 at 10:14:10PM -0700, Reiji Watanabe wrote:
> Uh, right, interrupts are not masked during those windows...
> 
> What I am currently considering on this would be disabling
> IRQs while manipulating the register, and introducing a new flag
> to indicate whether the PMUSERENR for the guest EL0 is loaded,
> and having kvm_set_pmuserenr() check the new flag.
> 
> The code would be something like below (local_irq_save/local_irq_restore
> needs to be excluded for NVHE though).
> 
> What do you think ?

I'm happy with that; it doesn't change the arm_pmu side of the interface and it
looks good from a functional perspective.

I'll have to leave it to Marc and Oliver to say whether they're happy with the
KVM side.

Thanks,
Mark.

> 
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -668,6 +668,8 @@ struct kvm_vcpu_arch {
>  /* Software step state is Active-pending */
>  #define DBG_SS_ACTIVE_PENDING	__vcpu_single_flag(sflags, BIT(5))
>  
> +/* PMUSERENR for the guest EL0 is on physical CPU */
> +#define PMUSERENR_ON_CPU	__vcpu_single_flag(sflags, BIT(6))
>  
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>  #define vcpu_sve_pffr(vcpu) (kern_hyp_va((vcpu)->arch.sve_state) +	\
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 6718731729fd..57e4f480874a 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -82,12 +82,19 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)
>  	 */
>  	if (kvm_arm_support_pmu_v3()) {
>  		struct kvm_cpu_context *hctxt;
> +		unsigned long flags;
>  
>  		write_sysreg(0, pmselr_el0);
>  
>  		hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> +
> +		local_irq_save(flags);
> +
>  		ctxt_sys_reg(hctxt, PMUSERENR_EL0) = read_sysreg(pmuserenr_el0);
>  		write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0);
> +		vcpu_set_flag(vcpu, PMUSERENR_ON_CPU);
> +
> +		local_irq_restore(flags);
>  	}
>  
>  	vcpu->arch.mdcr_el2_host = read_sysreg(mdcr_el2);
> @@ -112,9 +119,16 @@ static inline void __deactivate_traps_common(struct kvm_vcpu *vcpu)
>  	write_sysreg(0, hstr_el2);
>  	if (kvm_arm_support_pmu_v3()) {
>  		struct kvm_cpu_context *hctxt;
> +		unsigned long flags;
>  
>  		hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> +
> +		local_irq_save(flags);
> +
>  		write_sysreg(ctxt_sys_reg(hctxt, PMUSERENR_EL0), pmuserenr_el0);
> +		vcpu_clear_flag(vcpu, PMUSERENR_ON_CPU);
> +
> +		local_irq_restore(flags);
>  	}
>  
>  	if (cpus_have_final_cap(ARM64_SME)) {
> diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> index 40bb2cb13317..33cd8e1ecbd6 100644
> --- a/arch/arm64/kvm/pmu.c
> +++ b/arch/arm64/kvm/pmu.c
> @@ -221,8 +221,13 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu)
>  bool kvm_set_pmuserenr(u64 val)
>  {
>  	struct kvm_cpu_context *hctxt;
> +	struct kvm_vcpu *vcpu;
>  
> -	if (!kvm_arm_support_pmu_v3() || !has_vhe() || !kvm_get_running_vcpu())
> +	if (!kvm_arm_support_pmu_v3() || !has_vhe())
> +		return false;
> +
> +	vcpu = kvm_get_running_vcpu();
> +	if (!vcpu || !vcpu_get_flag(vcpu, PMUSERENR_ON_CPU))
>  		return false;
>  
>  	hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> -- 
> 
> Thank you,
> Reiji
> 
> 
> > 
> > Thanks,
> > Mark.
> > 
> > > Suggested-by: Mark Rutland <mark.rutland@arm.com>
> > > Suggested-by: Marc Zyngier <maz@kernel.org>
> > > Fixes: 83a7a4d643d3 ("arm64: perf: Enable PMU counter userspace access for perf event")
> > > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > > ---
> > >  arch/arm64/include/asm/kvm_host.h |  5 +++++
> > >  arch/arm64/kernel/perf_event.c    | 21 ++++++++++++++++++---
> > >  arch/arm64/kvm/pmu.c              | 20 ++++++++++++++++++++
> > >  3 files changed, 43 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index bcd774d74f34..22db2f885c17 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -1028,9 +1028,14 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
> > >  #ifdef CONFIG_KVM
> > >  void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr);
> > >  void kvm_clr_pmu_events(u32 clr);
> > > +bool kvm_set_pmuserenr(u64 val);
> > >  #else
> > >  static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {}
> > >  static inline void kvm_clr_pmu_events(u32 clr) {}
> > > +static inline bool kvm_set_pmuserenr(u64 val)
> > > +{
> > > +	return false;
> > > +}
> > >  #endif
> > >  
> > >  void kvm_vcpu_load_sysregs_vhe(struct kvm_vcpu *vcpu);
> > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > > index dde06c0f97f3..0fffe4c56c28 100644
> > > --- a/arch/arm64/kernel/perf_event.c
> > > +++ b/arch/arm64/kernel/perf_event.c
> > > @@ -741,9 +741,25 @@ static inline u32 armv8pmu_getreset_flags(void)
> > >  	return value;
> > >  }
> > >  
> > > +static void update_pmuserenr(u64 val)
> > > +{
> > > +	lockdep_assert_irqs_disabled();
> > > +
> > > +	/*
> > > +	 * The current pmuserenr value might be the value for the guest.
> > > +	 * If that's the case, have KVM keep tracking of the register value
> > > +	 * for the host EL0 so that KVM can restore it before returning to
> > > +	 * the host EL0. Otherwise, update the register now.
> > > +	 */
> > > +	if (kvm_set_pmuserenr(val))
> > > +		return;
> > > +
> > > +	write_sysreg(val, pmuserenr_el0);
> > > +}
> > > +
> > >  static void armv8pmu_disable_user_access(void)
> > >  {
> > > -	write_sysreg(0, pmuserenr_el0);
> > > +	update_pmuserenr(0);
> > >  }
> > >  
> > >  static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
> > > @@ -759,8 +775,7 @@ static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
> > >  			armv8pmu_write_evcntr(i, 0);
> > >  	}
> > >  
> > > -	write_sysreg(0, pmuserenr_el0);
> > > -	write_sysreg(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR, pmuserenr_el0);
> > > +	update_pmuserenr(ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_CR);
> > >  }
> > >  
> > >  static void armv8pmu_enable_event(struct perf_event *event)
> > > diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> > > index 7887133d15f0..40bb2cb13317 100644
> > > --- a/arch/arm64/kvm/pmu.c
> > > +++ b/arch/arm64/kvm/pmu.c
> > > @@ -209,3 +209,23 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu)
> > >  	kvm_vcpu_pmu_enable_el0(events_host);
> > >  	kvm_vcpu_pmu_disable_el0(events_guest);
> > >  }
> > > +
> > > +/*
> > > + * With VHE, keep track of the PMUSERENR_EL0 value for the host EL0 on
> > > + * the pCPU where vCPU is loaded, since PMUSERENR_EL0 is switched to
> > > + * the value for the guest on vcpu_load().  The value for the host EL0
> > > + * will be restored on vcpu_put(), before returning to the EL0.
> > > + *
> > > + * Return true if KVM takes care of the register. Otherwise return false.
> > > + */
> > > +bool kvm_set_pmuserenr(u64 val)
> > > +{
> > > +	struct kvm_cpu_context *hctxt;
> > > +
> > > +	if (!kvm_arm_support_pmu_v3() || !has_vhe() || !kvm_get_running_vcpu())
> > > +		return false;
> > > +
> > > +	hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> > > +	ctxt_sys_reg(hctxt, PMUSERENR_EL0) = val;
> > > +	return true;
> > > +}
> > > -- 
> > > 2.40.0.577.gac1e443424-goog
> > > 
> > > 
>