Re: [PATCH 21/59] KVM: arm64: nv: Set a handler for the system instruction traps

Julien Thierry
2019-06-25T12:55:31Z

On 06/21/2019 10:38 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
> EL2. In addition, execution of the EL1 translation regime address
> aranslation instructions and TLB maintenance instructions that are only

What's "translation regime address aranslation" ? I would guess
"aranslation" should be removed, but since the same pattern appears
twice in the commit doubt took over me :) .

> accessible from EL2 and above are trapped to EL2. In these cases,
> ESR_EL2.EC will be set to 0x18.
>
> Change the existing handler to handle those system instructions as well
> as MRS/MSR instructions. Emulation of each system instructions will be
> done in separate patches.
>
> Signed-off-by: Jintack Lim <jintack.lim@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm64/include/asm/kvm_coproc.h | 2 +-
> arch/arm64/kvm/handle_exit.c | 2 +-
> arch/arm64/kvm/sys_regs.c | 53 +++++++++++++++++++++++++----
> arch/arm64/kvm/trace.h | 2 +-
> 4 files changed, 50 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_coproc.h b/arch/arm64/include/asm/kvm_coproc.h
> index 0b52377a6c11..1b3d21bd8adb 100644
> --- a/arch/arm64/include/asm/kvm_coproc.h
> +++ b/arch/arm64/include/asm/kvm_coproc.h
> @@ -43,7 +43,7 @@ int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
> int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
> int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
> int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
> -int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_handle_sys(struct kvm_vcpu *vcpu, struct kvm_run *run);
>
> #define kvm_coproc_table_init kvm_sys_reg_table_init
> void kvm_sys_reg_table_init(void);
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 2517711f034f..e662f23b63a1 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -236,7 +236,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 1d1312425cf2..e711dde4511c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2597,6 +2597,40 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
> return 1;
> }
>
> +static int emulate_tlbi(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *params)
> +{
> + /* TODO: support tlbi instruction emulation*/
> + kvm_inject_undefined(vcpu);
> + return 1;
> +}
> +
> +static int emulate_at(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *params)
> +{
> + /* TODO: support address translation instruction emulation */
> + kvm_inject_undefined(vcpu);
> + return 1;
> +}
> +
> +static int emulate_sys_instr(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *params)
> +{
> + int ret = 0;
> +
> + /* TLB maintenance instructions*/
> + if (params->CRn == 0b1000)
> + ret = emulate_tlbi(vcpu, params);
> + /* Address Translation instructions */
> + else if (params->CRn == 0b0111 && params->CRm == 0b1000)
> + ret = emulate_at(vcpu, params);
> +

So, in theory the NV bit shouldn't trap other Op0 == 1 instructions.
Would it be worth adding a WARN() or BUG() in an "else" branch here,
just in case?

Thanks,

--
Julien Thierry