| Message ID | 20201026095116.72051-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Host EL2 entry improvements | expand |
On Monday 26 Oct 2020 at 09:51:09 (+0000), Marc Zyngier wrote: > The hyp-init code starts by stashing a register in TPIDR_EL2 > in in order to free a register. This happens no matter if the > HVC call is legal or not. > > Although nothing wrong seems to come out of it, it feels odd > to alter the EL2 state for something that eventually returns > an error. > > Instead, use the fact that we know exactly which bits of the > __kvm_hyp_init call are non-zero to perform the check with > a series of EOR/ROR instructions, combined with a build-time > check that the value is the one we expect. Alternatively, could we make __kvm_hyp_init non-SMCCC compliant? While I understand how it makes sense to be compliant for 'proper' HVCs, this one really is an odd one that only makes sense on a very transient state. That would let us define our convention, and we can just say x0-x18 can be clobbered like any function call, which eradicates the issue Andrew tried to avoid with this tpidr_el2 trick. Thoughts? Thanks, Quentin
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S index 47224dc62c51..b11a9d7db677 100644 --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S @@ -57,16 +57,25 @@ __do_hyp_init: cmp x0, #HVC_STUB_HCALL_NR b.lo __kvm_handle_stub_hvc - /* Set tpidr_el2 for use by HYP to free a register */ - msr tpidr_el2, x2 - - mov x2, #KVM_HOST_SMCCC_FUNC(__kvm_hyp_init) - cmp x0, x2 - b.eq 1f + // We only actively check bits [24:31], and everything + // else has to be zero, which we check at build time. +#if (KVM_HOST_SMCCC_FUNC(__kvm_hyp_init) & 0xFFFFFFFF00FFFFFF) +#error Unexpected __KVM_HOST_SMCCC_FUNC___kvm_hyp_init value +#endif + + ror x0, x0, #24 + eor x0, x0, #((KVM_HOST_SMCCC_FUNC(__kvm_hyp_init) >> 24) & 0xF) + ror x0, x0, #4 + eor x0, x0, #((KVM_HOST_SMCCC_FUNC(__kvm_hyp_init) >> 28) & 0xF) + cbz x0, 1f mov x0, #SMCCC_RET_NOT_SUPPORTED eret -1: phys_to_ttbr x0, x1 +1: + /* Set tpidr_el2 for use by HYP to free a register */ + msr tpidr_el2, x2 + + phys_to_ttbr x0, x1 alternative_if ARM64_HAS_CNP orr x0, x0, #TTBR_CNP_BIT alternative_else_nop_endif
The hyp-init code starts by stashing a register in TPIDR_EL2 in in order to free a register. This happens no matter if the HVC call is legal or not. Although nothing wrong seems to come out of it, it feels odd to alter the EL2 state for something that eventually returns an error. Instead, use the fact that we know exactly which bits of the __kvm_hyp_init call are non-zero to perform the check with a series of EOR/ROR instructions, combined with a build-time check that the value is the one we expect. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/hyp/nvhe/hyp-init.S | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)