Re: [PATCH v3 09/17] KVM: arm64: nv: Describe trap behaviour of MDCR_EL2.HPMN

[Marc Zyngier <maz@kernel.org>]

On Mon, 07 Oct 2024 18:45:51 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> MDCR_EL2.HPMN splits the PMU event counters into two ranges: the first
> range is accessible from all ELs, and the second range is accessible
> only to EL2/3. Supposing the guest hypervisor allows direct access to
> the PMU counters from the L2, KVM needs to locally handle those
> accesses.
> 
> Add a new complex trap configuration for HPMN that checks if the counter
> index is accessible to the current context. As written, the architecture
> suggests HPMN only causes PMEVCNTR<n>_EL0 to trap, though intuition (and
> the pseudocode) suggest that the trap applies to PMEVTYPER<n>_EL0 as
> well.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/emulate-nested.c | 167 ++++++++++++++++++++------------
>  arch/arm64/kvm/pmu-emul.c       |  11 +++
>  include/kvm/arm_pmu.h           |   6 ++
>  3 files changed, 120 insertions(+), 64 deletions(-)
> 
> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> index f9594296d69c..2d7a8b1f40fa 100644
> --- a/arch/arm64/kvm/emulate-nested.c
> +++ b/arch/arm64/kvm/emulate-nested.c
> @@ -110,6 +110,7 @@ enum cgt_group_id {
>  	CGT_HCR_TPU_TOCU,
>  	CGT_HCR_NV1_nNV2_ENSCXT,
>  	CGT_MDCR_TPM_TPMCR,
> +	CGT_MDCR_TPM_HPMN,
>  	CGT_MDCR_TDE_TDA,
>  	CGT_MDCR_TDE_TDOSA,
>  	CGT_MDCR_TDE_TDRA,
> @@ -126,6 +127,7 @@ enum cgt_group_id {
>  	CGT_CNTHCTL_EL1PTEN,
>  
>  	CGT_CPTR_TTA,
> +	CGT_MDCR_HPMN,
>  
>  	/* Must be last */
>  	__NR_CGT_GROUP_IDS__
> @@ -441,6 +443,7 @@ static const enum cgt_group_id *coarse_control_combo[] = {
>  	MCB(CGT_HCR_TPU_TOCU,		CGT_HCR_TPU, CGT_HCR_TOCU),
>  	MCB(CGT_HCR_NV1_nNV2_ENSCXT,	CGT_HCR_NV1_nNV2, CGT_HCR_ENSCXT),
>  	MCB(CGT_MDCR_TPM_TPMCR,		CGT_MDCR_TPM, CGT_MDCR_TPMCR),
> +	MCB(CGT_MDCR_TPM_HPMN,		CGT_MDCR_TPM, CGT_MDCR_HPMN),
>  	MCB(CGT_MDCR_TDE_TDA,		CGT_MDCR_TDE, CGT_MDCR_TDA),
>  	MCB(CGT_MDCR_TDE_TDOSA,		CGT_MDCR_TDE, CGT_MDCR_TDOSA),
>  	MCB(CGT_MDCR_TDE_TDRA,		CGT_MDCR_TDE, CGT_MDCR_TDRA),
> @@ -504,6 +507,41 @@ static enum trap_behaviour check_cptr_tta(struct kvm_vcpu *vcpu)
>  	return BEHAVE_HANDLE_LOCALLY;
>  }
>  
> +static enum trap_behaviour check_mdcr_hpmn(struct kvm_vcpu *vcpu)
> +{
> +	u32 sysreg = esr_sys64_to_sysreg(kvm_vcpu_get_esr(vcpu));
> +	unsigned int idx;
> +
> +
> +	switch (sysreg) {
> +	case SYS_PMEVTYPERn_EL0(0) ... SYS_PMEVTYPERn_EL0(30):
> +	case SYS_PMEVCNTRn_EL0(0) ... SYS_PMEVCNTRn_EL0(30):
> +		idx = (sys_reg_CRm(sysreg) & 0x3) << 3 | sys_reg_Op2(sysreg);
> +		break;
> +	case SYS_PMXEVTYPER_EL0:
> +	case SYS_PMXEVCNTR_EL0:
> +		idx = SYS_FIELD_GET(PMSELR_EL0, SEL,
> +				    __vcpu_sys_reg(vcpu, PMSELR_EL0));
> +		break;
> +	default:
> +		/* Someone used this trap helper for something else... */
> +		KVM_BUG_ON(1, vcpu->kvm);
> +		return BEHAVE_HANDLE_LOCALLY;
> +	}
> +
> +	/*
> +	 * Programming HPMN=0 is CONSTRAINED UNPREDICTABLE if FEAT_HPMN0 isn't
> +	 * implemented. Since KVM's ability to emulate HPMN=0 does not directly
> +	 * depend on hardware (all PMU registers are trapped), make the
> +	 * implementation choice that all counters are included in the second
> +	 * range reserved for EL2/EL3.
> +	 */

nit: I think this comment would make more sense with the helper,
rather than in the caller. The naming is rather explicit, and does
call for much questioning. The implementation is the more debatable
part, and this comment does provide the rationale.

Bonus point if you can include a reference to the spec (such as
K1.2.6.3 CONSTRAINED UNPREDICTABLE behavior caused by MDCR_EL2.HPMN).

Thanks,

	M.

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