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

Marc Zyngier
2019-07-03T14:15:41Z

On 25/06/2019 13:55, Julien Thierry wrote:
>
>
> 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 :) .

It's a whole new concept. Still working on it though! ;-)

>
>> 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?

Probably not a BUG(), but a WARN_ONCE() would be good.

Thanks,

M.
--
Jazz is not dead. It just smells funny...