Re: [PATCH v3 10/22] KVM: arm64: Set up FGT for Partitioned PMU

[Marc Zyngier <maz@kernel.org>]

On Thu, 26 Jun 2025 21:04:46 +0100,
Colton Lewis <coltonlewis@google.com> wrote:
> 
> In order to gain the best performance benefit from partitioning the
> PMU, utilize fine grain traps (FEAT_FGT and FEAT_FGT2) to avoid
> trapping common PMU register accesses by the guest to remove that
> overhead.
> 
> There should be no information leaks between guests as all these
> registers are context swapped by a later patch in this series.
> 
> Untrapped:
> * PMCR_EL0
> * PMUSERENR_EL0
> * PMSELR_EL0
> * PMCCNTR_EL0
> * PMINTEN_EL0
> * PMEVCNTRn_EL0
> 
> Trapped:
> * PMOVS_EL0
> * PMEVTYPERn_EL0
> * PMCCFILTR_EL0
> * PMICNTR_EL0
> * PMICFILTR_EL0
> 
> PMOVS remains trapped so KVM can track overflow IRQs that will need to
> be injected into the guest.
> 
> PMICNTR remains trapped because KVM is not handling that yet.
> 
> PMEVTYPERn remains trapped so KVM can limit which events guests can
> count, such as disallowing counting at EL2. PMCCFILTR and PMCIFILTR
> are the same.

I'd rather you explain why it is safe not to trap the rest.

> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
>  arch/arm64/include/asm/kvm_pmu.h        | 23 ++++++++++
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 58 +++++++++++++++++++++++++
>  arch/arm64/kvm/pmu-part.c               | 32 ++++++++++++++
>  3 files changed, 113 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_pmu.h b/arch/arm64/include/asm/kvm_pmu.h
> index 6328e90952ba..73b7161e3f4e 100644
> --- a/arch/arm64/include/asm/kvm_pmu.h
> +++ b/arch/arm64/include/asm/kvm_pmu.h
> @@ -94,6 +94,21 @@ u64 kvm_pmu_guest_counter_mask(struct arm_pmu *pmu);
>  void kvm_pmu_host_counters_enable(void);
>  void kvm_pmu_host_counters_disable(void);
>  
> +#if !defined(__KVM_NVHE_HYPERVISOR__)
> +bool kvm_vcpu_pmu_is_partitioned(struct kvm_vcpu *vcpu);
> +bool kvm_vcpu_pmu_use_fgt(struct kvm_vcpu *vcpu);
> +#else
> +static inline bool kvm_vcpu_pmu_is_partitioned(struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +
> +static inline bool kvm_vcpu_pmu_use_fgt(struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +#endif
> +
>  /*
>   * Updates the vcpu's view of the pmu events for this cpu.
>   * Must be called before every vcpu run after disabling interrupts, to ensure
> @@ -133,6 +148,14 @@ static inline u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu,
>  {
>  	return 0;
>  }
> +static inline bool kvm_vcpu_pmu_is_partitioned(struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +static inline bool kvm_vcpu_pmu_use_fgt(struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
>  static inline void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu,
>  					     u64 select_idx, u64 val) {}
>  static inline void kvm_pmu_set_counter_value_user(struct kvm_vcpu *vcpu,
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 825b81749972..47d2db8446df 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -191,6 +191,61 @@ static inline bool cpu_has_amu(void)
>                 ID_AA64PFR0_EL1_AMU_SHIFT);
>  }
>  
> +/**
> + * __activate_pmu_fgt() - Activate fine grain traps for partitioned PMU
> + * @vcpu: Pointer to struct kvm_vcpu
> + *
> + * Clear the most commonly accessed registers for a partitioned
> + * PMU. Trap the rest.
> + */
> +static inline void __activate_pmu_fgt(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpu_context *hctxt = host_data_ptr(host_ctxt);
> +	struct kvm *kvm = kern_hyp_va(vcpu->kvm);
> +	u64 set;
> +	u64 clr;
> +
> +	set = HDFGRTR_EL2_PMOVS
> +		| HDFGRTR_EL2_PMCCFILTR_EL0
> +		| HDFGRTR_EL2_PMEVTYPERn_EL0;
> +	clr = HDFGRTR_EL2_PMUSERENR_EL0
> +		| HDFGRTR_EL2_PMSELR_EL0
> +		| HDFGRTR_EL2_PMINTEN
> +		| HDFGRTR_EL2_PMCNTEN
> +		| HDFGRTR_EL2_PMCCNTR_EL0
> +		| HDFGRTR_EL2_PMEVCNTRn_EL0;
> +
> +	update_fgt_traps_cs(hctxt, vcpu, kvm, HDFGRTR_EL2, clr, set);
> +
> +	set = HDFGWTR_EL2_PMOVS
> +		| HDFGWTR_EL2_PMCCFILTR_EL0
> +		| HDFGWTR_EL2_PMEVTYPERn_EL0;
> +	clr = HDFGWTR_EL2_PMUSERENR_EL0
> +		| HDFGWTR_EL2_PMCR_EL0
> +		| HDFGWTR_EL2_PMSELR_EL0
> +		| HDFGWTR_EL2_PMINTEN
> +		| HDFGWTR_EL2_PMCNTEN
> +		| HDFGWTR_EL2_PMCCNTR_EL0
> +		| HDFGWTR_EL2_PMEVCNTRn_EL0;
> +
> +	update_fgt_traps_cs(hctxt, vcpu, kvm, HDFGWTR_EL2, clr, set);
> +
> +	if (!cpus_have_final_cap(ARM64_HAS_FGT2))
> +		return;
> +
> +	set = HDFGRTR2_EL2_nPMICFILTR_EL0
> +		| HDFGRTR2_EL2_nPMICNTR_EL0;
> +	clr = 0;
> +
> +	update_fgt_traps_cs(hctxt, vcpu, kvm, HDFGRTR2_EL2, clr, set);
> +
> +	set = HDFGWTR2_EL2_nPMICFILTR_EL0
> +		| HDFGWTR2_EL2_nPMICNTR_EL0;
> +	clr = 0;
> +
> +	update_fgt_traps_cs(hctxt, vcpu, kvm, HDFGWTR2_EL2, clr, set);

This feels wrong. There should be one place to populate the FGTs that
apply to a guest as set from the host, not two or more.

There is such a construct in the SME series, and maybe you could have
a look at it, specially if the trap configuration is this static.

	M.

-- 
Without deviation from the norm, progress is not possible.