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

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 12/10/24 14:35, Marc Zyngier wrote:
> 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.
> 

Hello Marc,

Thanks for your review and apologies for the delayed response. I am still
processing some of your review comments, and need some more time to fully
understand them in the given context. I will get back to you after doing
some more reading.

- Anshuman