Re: [PATCH v5 44/69] KVM: arm64: nv: Set a handler for the system instruction traps

[Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>]

On 30-11-2021 01:31 am, Marc Zyngier wrote:
> From: Jintack Lim <jintack.lim@linaro.org>
> 
> When HCR.NV bit is set, execution of the EL2 translation regime address
> aranslation instructions and TLB maintenance instructions are trapped to
Typo: translation

> EL2. In addition, execution of the EL1 translation regime address
> aranslation instructions and TLB maintenance instructions that are only
Typo: translation

> accessible from EL2 and above are trapped to EL2. In these cases,
> ESR_EL2.EC will be set to 0x18.
> 
> Rework the system instruction emulation framework to handle potentially
> all system instruction traps other than MSR/MRS instructions. Those
> system instructions would be AT and TLBI instructions controlled by
> HCR_EL2.NV, AT, and TTLB bits.
> 
> Signed-off-by: Jintack Lim <jintack.lim@linaro.org>
> [maz: squashed two patches together, redispatched various bits around]
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>   arch/arm64/include/asm/kvm_host.h |  4 +--
>   arch/arm64/kvm/handle_exit.c      |  2 +-
>   arch/arm64/kvm/sys_regs.c         | 48 +++++++++++++++++++++++++------
>   3 files changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 00c3366129b8..35f3d7939484 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -411,7 +411,7 @@ struct kvm_vcpu_arch {
>   	/*
>   	 * Guest registers we preserve during guest debugging.
>   	 *
> -	 * These shadow registers are updated by the kvm_handle_sys_reg
> +	 * These shadow registers are updated by the kvm_handle_sys
>   	 * trap handler if the guest accesses or updates them while we
>   	 * are using guest debug.
>   	 */
> @@ -729,7 +729,7 @@ int kvm_handle_cp14_32(struct kvm_vcpu *vcpu);
>   int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
>   int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
>   int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
> -int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
> +int kvm_handle_sys(struct kvm_vcpu *vcpu);
>   
>   void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
>   
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 6ff709c124d0..8b3b758e674e 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -234,7 +234,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>   	[ESR_ELx_EC_SMC32]	= handle_smc,
>   	[ESR_ELx_EC_HVC64]	= handle_hvc,
>   	[ESR_ELx_EC_SMC64]	= handle_smc,
> -	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
> +	[ESR_ELx_EC_SYS64]	= kvm_handle_sys,
>   	[ESR_ELx_EC_SVE]	= handle_sve,
>   	[ESR_ELx_EC_ERET]	= kvm_handle_eret,
>   	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 77f07f960d84..222f8c6f1d7e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1770,10 +1770,6 @@ static bool access_spsr_el2(struct kvm_vcpu *vcpu,
>    * more demanding guest...
>    */
>   static const struct sys_reg_desc sys_reg_descs[] = {
> -	{ SYS_DESC(SYS_DC_ISW), access_dcsw },
> -	{ SYS_DESC(SYS_DC_CSW), access_dcsw },
> -	{ SYS_DESC(SYS_DC_CISW), access_dcsw },
> -
>   	DBG_BCR_BVR_WCR_WVR_EL1(0),
>   	DBG_BCR_BVR_WCR_WVR_EL1(1),
>   	{ SYS_DESC(SYS_MDCCINT_EL1), trap_debug_regs, reset_val, MDCCINT_EL1, 0 },
> @@ -2240,6 +2236,14 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>   	{ SYS_DESC(SYS_SP_EL2), NULL, reset_unknown, SP_EL2 },
>   };
>   
> +#define SYS_INSN_TO_DESC(insn, access_fn, forward_fn)	\
> +	{ SYS_DESC((insn)), (access_fn), NULL, 0, 0, NULL, NULL, (forward_fn) }
> +static struct sys_reg_desc sys_insn_descs[] = {
> +	{ SYS_DESC(SYS_DC_ISW), access_dcsw },
> +	{ SYS_DESC(SYS_DC_CSW), access_dcsw },
> +	{ SYS_DESC(SYS_DC_CISW), access_dcsw },
> +};
> +
>   static bool trap_dbgdidr(struct kvm_vcpu *vcpu,
>   			struct sys_reg_params *p,
>   			const struct sys_reg_desc *r)
> @@ -2794,6 +2798,24 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
>   	return 1;
>   }
>   
> +static int emulate_sys_instr(struct kvm_vcpu *vcpu, struct sys_reg_params *p)
> +{
> +	const struct sys_reg_desc *r;
> +
> +	/* Search from the system instruction table. */
> +	r = find_reg(p, sys_insn_descs, ARRAY_SIZE(sys_insn_descs));
> +
> +	if (likely(r)) {
> +		perform_access(vcpu, p, r);
> +	} else {
> +		kvm_err("Unsupported guest sys instruction at: %lx\n",
> +			*vcpu_pc(vcpu));
> +		print_sys_reg_instr(p);
> +		kvm_inject_undefined(vcpu);
> +	}
> +	return 1;
> +}
> +
>   /**
>    * kvm_reset_sys_regs - sets system registers to reset value
>    * @vcpu: The VCPU pointer
> @@ -2811,10 +2833,11 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>   }
>   
>   /**
> - * kvm_handle_sys_reg -- handles a mrs/msr trap on a guest sys_reg access
> + * kvm_handle_sys-- handles a system instruction or mrs/msr instruction trap
> +		    on a guest execution
>    * @vcpu: The VCPU pointer
>    */
> -int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
> +int kvm_handle_sys(struct kvm_vcpu *vcpu)
>   {
>   	struct sys_reg_params params;
>   	unsigned long esr = kvm_vcpu_get_esr(vcpu);
> @@ -2826,10 +2849,16 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
>   	params = esr_sys64_to_params(esr);
>   	params.regval = vcpu_get_reg(vcpu, Rt);
>   
> -	ret = emulate_sys_reg(vcpu, &params);
> +	if (params.Op0 == 1) {
> +		/* System instructions */
> +		ret = emulate_sys_instr(vcpu, &params);
> +	} else {
> +		/* MRS/MSR instructions */
> +		ret = emulate_sys_reg(vcpu, &params);
> +		if (!params.is_write)
> +			vcpu_set_reg(vcpu, Rt, params.regval);
> +	}
>   
> -	if (!params.is_write)
> -		vcpu_set_reg(vcpu, Rt, params.regval);
>   	return ret;
>   }
>   
> @@ -3245,6 +3274,7 @@ void kvm_sys_reg_table_init(void)
>   	BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs), true));
>   	BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs), true));
>   	BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs), false));
> +	BUG_ON(check_sysreg_table(sys_insn_descs, ARRAY_SIZE(sys_insn_descs), false));
>   
>   	/* We abuse the reset function to overwrite the table itself. */
>   	for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)

It looks good to me, please feel free to add.
Reviewed-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>

Thanks,
Ganapat