Re: [PATCH 27/59] KVM: arm64: nv: Respect virtual HCR_EL2.TVM and TRVM settings

Marc Zyngier
2019-07-04T14:57:47Z

On 26/06/2019 07:55, Julien Thierry wrote:
>
>
> On 06/21/2019 10:38 AM, Marc Zyngier wrote:
>> From: Jintack Lim <jintack.lim@linaro.org>
>>
>> Forward the EL1 virtual memory register traps to the virtual EL2 if they
>> are not coming from the virtual EL2 and the virtual HCR_EL2.TVM or TRVM
>> bit is set.
>>
>> This is for recursive nested virtualization.
>>
>> Signed-off-by: Jintack Lim <jintack.lim@linaro.org>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> arch/arm64/kvm/sys_regs.c | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 582d62aa48b7..0f74b9277a86 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -436,6 +436,27 @@ static bool access_dcsw(struct kvm_vcpu *vcpu,
>> return true;
>> }
>>
>> +/* This function is to support the recursive nested virtualization */
>> +static bool forward_vm_traps(struct kvm_vcpu *vcpu, struct sys_reg_params *p)
>> +{
>> + u64 hcr_el2 = __vcpu_sys_reg(vcpu, HCR_EL2);
>> +
>> + /* If a trap comes from the virtual EL2, the host hypervisor handles. */
>> + if (vcpu_mode_el2(vcpu))
>> + return false;
>> +
>> + /*
>> + * If the virtual HCR_EL2.TVM or TRVM bit is set, we need to foward
>> + * this trap to the virtual EL2.
>> + */
>> + if ((hcr_el2 & HCR_TVM) && p->is_write)
>> + return true;
>> + else if ((hcr_el2 & HCR_TRVM) && !p->is_write)
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> /*
>> * Generic accessor for VM registers. Only called as long as HCR_TVM
>> * is set. If the guest enables the MMU, we stop trapping the VM
>> @@ -452,6 +473,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
>> if (el12_reg(p) && forward_nv_traps(vcpu))
>> return false;
>>
>> + if (!el12_reg(p) && forward_vm_traps(vcpu, p))
>> + return kvm_inject_nested_sync(vcpu, kvm_vcpu_get_hsr(vcpu));
>
> Since we already have forward_traps(), isn't this just:
>
> if (!el12_reg(p) && forward_traps(vcpu, p->is_write ? HCR_TVM : HCR_TRVM))
> return true;
>
> We could maybe simplify forward_vm_traps() to just call forward_traps()
> similar to forward_nv_traps().

Odd. I remember doing something like that. Where has it gone? Yes, this
looks sensible.

Thanks,

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