Hi Julien, On 3/20/19 5:43 PM, Julien Thierry wrote:
Hi Amit, On 19/03/2019 08:30, Amit Daniel Kachhap wrote:From: Mark Rutland <mark.rutland@xxxxxxx> When pointer authentication is supported, a guest may wish to use it. This patch adds the necessary KVM infrastructure for this to work, with a semi-lazy context switch of the pointer auth state. Pointer authentication feature is only enabled when VHE is built in the kernel and present in the CPU implementation so only VHE code paths are modified. When we schedule a vcpu, we disable guest usage of pointer authentication instructions and accesses to the keys. While these are disabled, we avoid context-switching the keys. When we trap the guest trying to use pointer authentication functionality, we change to eagerly context-switching the keys, and enable the feature. The next time the vcpu is scheduled out/in, we start again. However the host key save is optimized and implemented inside ptrauth instruction/register access trap. Pointer authentication consists of address authentication and generic authentication, and CPUs in a system might have varied support for either. Where support for either feature is not uniform, it is hidden from guests via ID register emulation, as a result of the cpufeature framework in the host. Unfortunately, address authentication and generic authentication cannot be trapped separately, as the architecture provides a single EL2 trap covering both. If we wish to expose one without the other, we cannot prevent a (badly-written) guest from intermittently using a feature which is not uniformly supported (when scheduled on a physical CPU which supports the relevant feature). Hence, this patch expects both type of authentication to be present in a cpu. This switch of key is done from guest enter/exit assembly as preperation for the upcoming in-kernel pointer authentication support. Hence, these key switching routines are not implemented in C code as they may cause pointer authentication key signing error in some situations. Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx> [Only VHE, key switch in full assembly, vcpu_has_ptrauth checks , save host key in ptrauth exception trap] Signed-off-by: Amit Daniel Kachhap <amit.kachhap@xxxxxxx> Reviewed-by: Julien Thierry <julien.thierry@xxxxxxx> Cc: Marc Zyngier <marc.zyngier@xxxxxxx> Cc: Christoffer Dall <christoffer.dall@xxxxxxx> Cc: kvmarm@xxxxxxxxxxxxxxxxxxxxx --- arch/arm64/include/asm/kvm_host.h | 17 ++++++ arch/arm64/include/asm/kvm_ptrauth_asm.h | 100 +++++++++++++++++++++++++++++++ arch/arm64/kernel/asm-offsets.c | 6 ++ arch/arm64/kvm/guest.c | 14 +++++ arch/arm64/kvm/handle_exit.c | 24 +++++--- arch/arm64/kvm/hyp/entry.S | 7 +++ arch/arm64/kvm/reset.c | 7 +++ arch/arm64/kvm/sys_regs.c | 46 +++++++++++++- virt/kvm/arm/arm.c | 2 + 9 files changed, 212 insertions(+), 11 deletions(-) create mode 100644 arch/arm64/include/asm/kvm_ptrauth_asm.h
[...]
Actually the values of CPU_AP*_EL1 are exceeding the immediate range (i.e 512), so this was done to keep the immediate offset within the range. The other way would have been to calculate the destination register but these would add one more add instruction everywhere.+#ifdef CONFIG_ARM64_PTR_AUTH + +#define PTRAUTH_REG_OFFSET(x) (x - CPU_APIAKEYLO_EL1)I don't really see the point of this macro. You move the pointers of kvm_cpu_contexts to point to where the ptr auth registers are (which is in the middle of an array) by adding the offset of APIAKEYLO and then we have to recompute all offsets with this macro. Why not just pass the kvm_cpu_context pointers to ptrauth_save/restore_state and use the already defined offsets (CPU_AP*_EL1) directly? I think this would also allow to use one less register for the ptrauth_switch_to_* macros.
I should have mentioned them as comments somewhere.
+ +.macro ptrauth_save_state base, reg1, reg2 + mrs_s \reg1, SYS_APIAKEYLO_EL1 + mrs_s \reg2, SYS_APIAKEYHI_EL1 + stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)] + mrs_s \reg1, SYS_APIBKEYLO_EL1 + mrs_s \reg2, SYS_APIBKEYHI_EL1 + stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)] + mrs_s \reg1, SYS_APDAKEYLO_EL1 + mrs_s \reg2, SYS_APDAKEYHI_EL1 + stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)] + mrs_s \reg1, SYS_APDBKEYLO_EL1 + mrs_s \reg2, SYS_APDBKEYHI_EL1 + stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)] + mrs_s \reg1, SYS_APGAKEYLO_EL1 + mrs_s \reg2, SYS_APGAKEYHI_EL1 + stp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)] +.endm + +.macro ptrauth_restore_state base, reg1, reg2 + ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)] + msr_s SYS_APIAKEYLO_EL1, \reg1 + msr_s SYS_APIAKEYHI_EL1, \reg2 + ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)] + msr_s SYS_APIBKEYLO_EL1, \reg1 + msr_s SYS_APIBKEYHI_EL1, \reg2 + ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)] + msr_s SYS_APDAKEYLO_EL1, \reg1 + msr_s SYS_APDAKEYHI_EL1, \reg2 + ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)] + msr_s SYS_APDBKEYLO_EL1, \reg1 + msr_s SYS_APDBKEYHI_EL1, \reg2 + ldp \reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)] + msr_s SYS_APGAKEYLO_EL1, \reg1 + msr_s SYS_APGAKEYHI_EL1, \reg2 + .endmNit: Bad indentation.
oops.
+ +.macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3 + ldr \reg1, [\g_ctxt, #CPU_HCR_EL2] + and \reg1, \reg1, #(HCR_API | HCR_APK) + cbz \reg1, skip_switch_to_guest + add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 + ptrauth_restore_state \reg1, \reg2, \reg3As mentioned we probably can get rid of one temporary register and directly pass g_ctxt here.+skip_switch_to_guest:I believe you should use numbered labels for assembly macros, otherwise multiple references to that macros will lead to symbol redefinitions.
Yes agreed. Thanks, Amit
+.endm + +.macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3 + ldr \reg1, [\g_ctxt, #CPU_HCR_EL2] + and \reg1, \reg1, #(HCR_API | HCR_APK) + cbz \reg1, skip_switch_to_host + add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 + ptrauth_save_state \reg1, \reg2, \reg3 + add \reg1, \h_ctxt, #CPU_APIAKEYLO_EL1 + ptrauth_restore_state \reg1, \reg2, \reg3 + isb +skip_switch_to_host:Same. Cheers, Julien+.endm + +#else /* !CONFIG_ARM64_PTR_AUTH */ +.macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3 +.endm +.macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3
[...]
_______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
