Re: [PATCH V2 46/46] KVM: arm64: nv: Add trap forwarding for FEAT_FGT2 described registers

[Marc Zyngier <maz@kernel.org>]

On Tue, 10 Dec 2024 05:53:11 +0000,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> Describe remaining MDCR_EL2 register, and associate that with all FEAT_FGT2
> exposed system registers it allows to trap.

MDCR_EL2 register *bits*? How is that related to FGT2 at all?

> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.linux.dev
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Changes in V2:
> 
> - Dropped check_cntr_accessible_N and CGT_CNTR_ACCESSIBLE_N constructs
> - SYS_PMEVCNTSVR_EL1(N) access traps have been forwarded to CGT_MDCR_HPMN
> - Updated check_mdcr_hpmn() to handle SYS_PMEVCNTSVR_EL1(N) registers
> - Changed behaviour as BEHAVE_FORWARD_RW for CGT_MDCR_EnSPM
> 
>  arch/arm64/include/asm/kvm_host.h |   2 +
>  arch/arm64/kvm/emulate-nested.c   | 158 ++++++++++++++++++++++++++++++
>  2 files changed, 160 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c80c07be3358..4cdce62642d1 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -441,6 +441,7 @@ enum vcpu_sysreg {
>  	PMINTENSET_EL1,	/* Interrupt Enable Set Register */
>  	PMOVSSET_EL0,	/* Overflow Flag Status Set Register */
>  	PMUSERENR_EL0,	/* User Enable Register */
> +	SPMSELR_EL0,	/* System PMU Select Register */

How could a system PMU be relevant to a VM?  What is the point of
bloating the vcpu for something that we will hopefully *never* make
visible to guests?

>  
>  	/* Pointer Authentication Registers in a strict increasing order. */
>  	APIAKEYLO_EL1,
> @@ -501,6 +502,7 @@ enum vcpu_sysreg {
>  	CNTHP_CVAL_EL2,
>  	CNTHV_CTL_EL2,
>  	CNTHV_CVAL_EL2,
> +	SPMACCESSR_EL2, /* System PMU Access Register */

Same here. It is pretty striking that these registers are never
saved/restored or handled as traps, which is a good indication that
this is pretty pointless.

>  
>  	/* Anything from this can be RES0/RES1 sanitised */
>  	MARKER(__SANITISED_REG_START__),
> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> index 6c63cbfc11ea..c7d6d2034f27 100644
> --- a/arch/arm64/kvm/emulate-nested.c
> +++ b/arch/arm64/kvm/emulate-nested.c
> @@ -79,6 +79,7 @@ enum cgt_group_id {
>  	CGT_MDCR_TDRA,
>  	CGT_MDCR_E2PB,
>  	CGT_MDCR_TPMS,
> +	CGT_MDCR_EnSPM,
>  	CGT_MDCR_TTRF,
>  	CGT_MDCR_E2TB,
>  	CGT_MDCR_TDCC,
> @@ -125,6 +126,7 @@ enum cgt_group_id {
>  	CGT_CNTHCTL_EL1PCTEN = __COMPLEX_CONDITIONS__,
>  	CGT_CNTHCTL_EL1PTEN,
>  
> +	CGT_SPMSEL_SPMACCESS,
>  	CGT_CPTR_TTA,
>  	CGT_MDCR_HPMN,
>  
> @@ -351,6 +353,12 @@ static const struct trap_bits coarse_trap_bits[] = {
>  		.mask		= MDCR_EL2_TPMS,
>  		.behaviour	= BEHAVE_FORWARD_RW,
>  	},
> +	[CGT_MDCR_EnSPM] = {
> +		.index		= MDCR_EL2,
> +		.value		= MDCR_EL2_EnSPM,
> +		.mask		= MDCR_EL2_EnSPM,
> +		.behaviour	= BEHAVE_FORWARD_RW,
> +	},
>  	[CGT_MDCR_TTRF] = {
>  		.index		= MDCR_EL2,
>  		.value		= MDCR_EL2_TTRF,
> @@ -509,6 +517,7 @@ static enum trap_behaviour check_mdcr_hpmn(struct kvm_vcpu *vcpu)
>  	switch (sysreg) {
>  	case SYS_PMEVTYPERn_EL0(0) ... SYS_PMEVTYPERn_EL0(30):
>  	case SYS_PMEVCNTRn_EL0(0) ... SYS_PMEVCNTRn_EL0(30):
> +	case SYS_PMEVCNTSVR_EL1(0) ... SYS_PMEVCNTSVR_EL1(30):
>  		idx = (sys_reg_CRm(sysreg) & 0x3) << 3 | sys_reg_Op2(sysreg);
>  		break;
>  	case SYS_PMXEVTYPER_EL0:
> @@ -528,6 +537,22 @@ static enum trap_behaviour check_mdcr_hpmn(struct kvm_vcpu *vcpu)
>  	return BEHAVE_HANDLE_LOCALLY;
>  }
>  
> +static enum trap_behaviour check_spmsel_spmaccess(struct kvm_vcpu *vcpu)
> +{
> +	u64 spmaccessr_el2, spmselr_el2;
> +	int syspmusel;
> +
> +	if (__vcpu_sys_reg(vcpu, MDCR_EL2) & MDCR_EL2_EnSPM) {

I don't mind the test, but I don't see any sanitising of MDCR_EL2 to
make EnSPM as RES0 when FEAT_SPMU is not implemented, which will be
100% of the cases.

> +		spmselr_el2 = __vcpu_sys_reg(vcpu, SPMSELR_EL0);
> +		spmaccessr_el2 = __vcpu_sys_reg(vcpu, SPMACCESSR_EL2);

So these two values are *guaranteed* to be zero. At this stage, what
is the point?

> +		syspmusel = FIELD_GET(SPMSELR_EL0_SYSPMUSEL_MASK, spmselr_el2);
> +
> +		if (((spmaccessr_el2 >> (syspmusel * 2)) & 0x3) == 0x0)
> +			return BEHAVE_FORWARD_RW;

What about value 0b01, which causes *writes* to be trapped?

> +	}
> +	return BEHAVE_HANDLE_LOCALLY;

And then what? How do we handle this locally?

Honestly, short of any additional handling, we would be better off
just injecting an UNDEF back into the guest.

	M.

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