Hi Marc,
On Fri, Jan 28, 2022 at 12:18:26PM +0000, Marc Zyngier wrote:
> From: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>
> When running in virtual EL2 mode, we actually run the hardware in EL1
> and therefore have to use the EL1 registers to ensure correct operation.
>
> By setting the HCR.TVM and HCR.TVRM we ensure that the virtual EL2 mode
> doesn't shoot itself in the foot when setting up what it believes to be
> a different mode's system register state (for example when preparing to
> switch to a VM).
>
> We can leverage the existing sysregs infrastructure to support trapped
> accesses to these registers.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> ---
> arch/arm64/kvm/hyp/include/hyp/switch.h | 4 +---
> arch/arm64/kvm/hyp/nvhe/switch.c | 2 +-
> arch/arm64/kvm/hyp/vhe/switch.c | 7 ++++++-
> arch/arm64/kvm/sys_regs.c | 19 ++++++++++++++++---
> 4 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 58e14f8ead23..49c3b9eb09d7 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -110,10 +110,8 @@ static inline void __deactivate_traps_common(struct kvm_vcpu *vcpu)
> write_sysreg(0, pmuserenr_el0);
> }
>
> -static inline void ___activate_traps(struct kvm_vcpu *vcpu)
> +static inline void ___activate_traps(struct kvm_vcpu *vcpu, u64 hcr)
> {
> - u64 hcr = vcpu->arch.hcr_el2;
> -
> if (cpus_have_final_cap(ARM64_WORKAROUND_CAVIUM_TX2_219_TVM))
> hcr |= HCR_TVM;
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index 6410d21d8695..61a5627fd456 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -38,7 +38,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
> {
> u64 val;
>
> - ___activate_traps(vcpu);
> + ___activate_traps(vcpu, vcpu->arch.hcr_el2);
> __activate_traps_common(vcpu);
>
> val = vcpu->arch.cptr_el2;
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index 82ddaebe66de..6ed9e4893a02 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -32,9 +32,14 @@ DEFINE_PER_CPU(unsigned long, kvm_hyp_vector);
>
> static void __activate_traps(struct kvm_vcpu *vcpu)
> {
> + u64 hcr = vcpu->arch.hcr_el2;
> u64 val;
>
> - ___activate_traps(vcpu);
> + /* Trap VM sysreg accesses if an EL2 guest is not using VHE. */
> + if (vcpu_is_el2(vcpu) && !vcpu_el2_e2h_is_set(vcpu))
> + hcr |= HCR_TVM | HCR_TRVM;
> +
> + ___activate_traps(vcpu, hcr);
>
> val = read_sysreg(cpacr_el1);
> val |= CPACR_EL1_TTA;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 102bc4906723..9d3520f1d17a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -322,8 +322,15 @@ static void get_access_mask(const struct sys_reg_desc *r, u64 *mask, u64 *shift)
>
> /*
> * 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
> - * sys_regs and leave it in complete control of the caches.
> + * is set.
> + *
> + * This is set in two cases: either (1) we're running at vEL2, or (2)
> + * we're running at EL1 and the guest has its MMU off.
> + *
> + * (1) TVM/TRVM is set, as we need to virtualise some of the VM
> + * registers for the guest hypervisor
> + * (2) Once the guest enables the MMU, we stop trapping the VM sys_regs
> + * and leave it in complete control of the caches.
> */
> static bool access_vm_reg(struct kvm_vcpu *vcpu,
> struct sys_reg_params *p,
> @@ -332,7 +339,13 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
> bool was_enabled = vcpu_has_cache_enabled(vcpu);
> u64 val, mask, shift;
>
> - BUG_ON(!p->is_write);
> + /* We don't expect TRVM on the host */
I don't get what that means. Isn't KVM setting HCR_EL2.TRVM to trap reads?
Other than that, the patch looks good:
Reviewed-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
Thanks,
Alex
> + BUG_ON(!vcpu_is_el2(vcpu) && !p->is_write);
> +
> + if (!p->is_write) {
> + p->regval = vcpu_read_sys_reg(vcpu, r->reg);
> + return true;
> + }
>
> get_access_mask(r, &mask, &shift);
>
> --
> 2.30.2
>
