Re: [PATCH v6 24/64] KVM: arm64: nv: Respect the virtual HCR_EL2.NV bit setting
From: Alexandru Elisei <hidden>
Date: 2022-02-07 16:04:22
Also in:
kvm, kvmarm
Hi Marc, On Fri, Jan 28, 2022 at 12:18:32PM +0000, Marc Zyngier wrote:
From: Jintack Lim <redacted> Forward traps due to HCR_EL2.NV bit to the virtual EL2 if they are not coming from the virtual EL2 and the virtual HCR_EL2.NV bit is set. In addition to EL2 register accesses, setting NV bit will also make EL12 register accesses trap to EL2. To emulate this for the virtual EL2, forword traps due to EL12 register accessses to the virtual EL2 if the virtual HCR_EL2.NV bit is set.
The patch also adds handling for the HCR_EL2.TSC bit. It might prove useful for the commit subject and message to reflect that. Also, HCR_EL2.NV also enables trapping of accesses to the *_EL02, *_EL2 and SP_EL1 registers, or trapping the execution of the ERET, ERETAA, ERETAB, and of certain AT and TLB maintenance instructions. I don't see those mentioned anywhere. IMO, the commit message should be reworded to say exactly is being forwarded, because as it stands it is very misleading.
This is for recursive nested virtualization. Signed-off-by: Jintack Lim <redacted> [Moved code to emulate-nested.c]
What goes in emulate-nested.c and what goes in nested.c?
quoted hunk ↗ jump to hunk
Signed-off-by: Christoffer Dall <redacted> Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/kvm_arm.h | 1 + arch/arm64/include/asm/kvm_nested.h | 2 ++ arch/arm64/kvm/emulate-nested.c | 27 +++++++++++++++++++++++++++ arch/arm64/kvm/handle_exit.c | 7 +++++++ arch/arm64/kvm/sys_regs.c | 21 +++++++++++++++++++++ 5 files changed, 58 insertions(+)diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index 5acb153a82c8..8043827e7dc0 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h@@ -20,6 +20,7 @@ #define HCR_AMVOFFEN (UL(1) << 51) #define HCR_FIEN (UL(1) << 47) #define HCR_FWB (UL(1) << 46) +#define HCR_NV (UL(1) << 42) #define HCR_API (UL(1) << 41) #define HCR_APK (UL(1) << 40) #define HCR_TEA (UL(1) << 37)diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h index 79d382fa02ea..37ff6458296d 100644 --- a/arch/arm64/include/asm/kvm_nested.h +++ b/arch/arm64/include/asm/kvm_nested.h@@ -66,5 +66,7 @@ static inline u64 translate_cnthctl_el2_to_cntkctl_el1(u64 cnthctl) } int handle_wfx_nested(struct kvm_vcpu *vcpu, bool is_wfe); +extern bool forward_traps(struct kvm_vcpu *vcpu, u64 control_bit); +extern bool forward_nv_traps(struct kvm_vcpu *vcpu); #endif /* __ARM64_KVM_NESTED_H */diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c index f52cd4458947..7dd98d6e96e0 100644 --- a/arch/arm64/kvm/emulate-nested.c +++ b/arch/arm64/kvm/emulate-nested.c@@ -13,6 +13,26 @@ #include "trace.h" +bool forward_traps(struct kvm_vcpu *vcpu, u64 control_bit) +{ + bool control_bit_set; + + if (!vcpu_has_nv(vcpu)) + return false; + + control_bit_set = __vcpu_sys_reg(vcpu, HCR_EL2) & control_bit; + if (!vcpu_is_el2(vcpu) && control_bit_set) { + kvm_inject_nested_sync(vcpu, kvm_vcpu_get_esr(vcpu)); + return true; + } + return false; +} + +bool forward_nv_traps(struct kvm_vcpu *vcpu) +{ + return forward_traps(vcpu, HCR_NV); +} + static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr) { u64 mode = spsr & PSR_MODE_MASK;@@ -49,6 +69,13 @@ void kvm_emulate_nested_eret(struct kvm_vcpu *vcpu) u64 spsr, elr, mode; bool direct_eret; + /* + * Forward this trap to the virtual EL2 if the virtual + * HCR_EL2.NV bit is set and this is coming from !EL2. + */
I was under the impression that Documentation/process/coding-style.rst frowns upon explaining what a function does. forward_traps() is small and simple, I think the comment is not needed for understanding what the function does.
quoted hunk ↗ jump to hunk
+ if (forward_nv_traps(vcpu)) + return; + /* * Going through the whole put/load motions is a waste of time * if this is a VHE guest hypervisor returning to its owndiff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index a5c698f188d6..867de65eb766 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c@@ -64,6 +64,13 @@ static int handle_smc(struct kvm_vcpu *vcpu) { int ret; + /* + * Forward this trapped smc instruction to the virtual EL2 if + * the guest has asked for it. + */ + if (forward_traps(vcpu, HCR_TSC))
Like I've said, this part is not mentioned in the commit message at all.
quoted hunk ↗ jump to hunk
+ return 1; + /* * "If an SMC instruction executed at Non-secure EL1 is * trapped to EL2 because HCR_EL2.TSC is 1, the exception is adiff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 7f074a7f6eb3..ccd063d6cb69 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c@@ -267,10 +267,19 @@ static u32 get_ccsidr(u32 csselr) return ccsidr; } +static bool el12_reg(struct sys_reg_params *p) +{ + /* All *_EL12 registers have Op1=5. */ + return (p->Op1 == 5); +} + static bool access_rw(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { + if (el12_reg(p) && forward_nv_traps(vcpu)) + return false; + if (p->is_write) vcpu_write_sys_reg(vcpu, p->regval, r->reg); else@@ -339,6 +348,9 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu, bool was_enabled = vcpu_has_cache_enabled(vcpu); u64 val, mask, shift; + if (el12_reg(p) && forward_nv_traps(vcpu)) + return false; + /* We don't expect TRVM on the host */ BUG_ON(!vcpu_is_el2(vcpu) && !p->is_write);@@ -1654,6 +1666,9 @@ static bool access_elr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { + if (el12_reg(p) && forward_nv_traps(vcpu))
ELR_EL2 has Op1 = 4, and ELR_EL1 has Op1 = 0, and as far as I can tell there are no _EL12 variants. Why use el12_reg() here when it always returns false?
quoted hunk ↗ jump to hunk
+ return false; + if (p->is_write) vcpu_write_sys_reg(vcpu, p->regval, ELR_EL1); else@@ -1666,6 +1681,9 @@ static bool access_spsr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { + if (el12_reg(p) && forward_nv_traps(vcpu)) + return false; + if (p->is_write) __vcpu_sys_reg(vcpu, SPSR_EL1) = p->regval; else@@ -1678,6 +1696,9 @@ static bool access_spsr_el2(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { + if (el12_reg(p) && forward_nv_traps(vcpu)) + return false;
spsr_el2 is an EL2 register, and el12_reg() always returns false (Op1 = 5). Shouldn't that check be only: if (forward_nv_traps(vcpu)) return false; Thanks, Alex
+ if (p->is_write) vcpu_write_sys_reg(vcpu, p->regval, SPSR_EL2); else -- 2.30.2
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel