On Thu, 14 Mar 2024 20:23:22 +0000, Pierre-Clément Tosi <ptosi@xxxxxxxxxx> wrote: > > When the hypervisor receives a SError or synchronous exception (EL2h) nit: since this also affects SError, $subject seems wrong. > while running with the __kvm_hyp_vector and if ELR_EL2 doesn't point to > an extable entry, it panics indirectly by overwriting ELR with the > address of a panic handler in order for the asm routine it returns to to > ERET into the handler. This is done (instead of a simple function call) > to ensure that the panic handler runs with the SPSel that was in use > when the exception was triggered, necessary to support features such as > the shadow call stack. I don't understand this. Who changes SPsel? At any given point, SP_EL0 is that of either the host or the guest, but never the hypervisor's. If something was touching SP_EL0 behind our back, it would result in corruption (see 6e977984f6d8 for a bit of rationale about it). > > However, this clobbers ELR_EL2 for the handler itself. As a result, > hyp_panic(), when retrieving what it believes to be the PC where the > exception happened, actually ends up reading the address of the panic > handler that called it! This results in an erroneous and confusing panic > message where the source of any synchronous exception (e.g. BUG() or > kCFI) appears to be __guest_exit_panic, making it hard to locate the > actual BRK instruction. > > Therefore, store the original ELR_EL2 in a per-CPU struct and point the Can you elaborate on *which* per-CPU structure you are using? > sysreg to a routine that first restores it to its previous value before > running __guest_exit_panic. > > Fixes: 7db21530479f ("KVM: arm64: Restore hyp when panicking in guest context") > Signed-off-by: Pierre-Clément Tosi <ptosi@xxxxxxxxxx> > --- > arch/arm64/kernel/asm-offsets.c | 1 + > arch/arm64/kvm/hyp/entry.S | 9 +++++++++ > arch/arm64/kvm/hyp/include/hyp/switch.h | 6 ++++-- > 3 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c > index 5a7dbbe0ce63..e62353168a57 100644 > --- a/arch/arm64/kernel/asm-offsets.c > +++ b/arch/arm64/kernel/asm-offsets.c > @@ -128,6 +128,7 @@ int main(void) > DEFINE(VCPU_FAULT_DISR, offsetof(struct kvm_vcpu, arch.fault.disr_el1)); > DEFINE(VCPU_HCR_EL2, offsetof(struct kvm_vcpu, arch.hcr_el2)); > DEFINE(CPU_USER_PT_REGS, offsetof(struct kvm_cpu_context, regs)); > + DEFINE(CPU_ELR_EL2, offsetof(struct kvm_cpu_context, sys_regs[ELR_EL2])); > DEFINE(CPU_RGSR_EL1, offsetof(struct kvm_cpu_context, sys_regs[RGSR_EL1])); > DEFINE(CPU_GCR_EL1, offsetof(struct kvm_cpu_context, sys_regs[GCR_EL1])); > DEFINE(CPU_APIAKEYLO_EL1, offsetof(struct kvm_cpu_context, sys_regs[APIAKEYLO_EL1])); > diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S > index f3aa7738b477..9cdf46da3051 100644 > --- a/arch/arm64/kvm/hyp/entry.S > +++ b/arch/arm64/kvm/hyp/entry.S > @@ -83,6 +83,15 @@ alternative_else_nop_endif > eret > sb > > +SYM_INNER_LABEL(__guest_exit_panic_with_restored_elr, SYM_L_GLOBAL) The name of the function isn't great. Crucially, 'with_restored_elr' implies that ELR is already restored, while it is the hunk below that does the 'restore' part. Something like '__guest_exit_restore_elr_and_panic' seems more appropriate. > + // x0-x29,lr: hyp regs > + > + stp x0, x1, [sp, #-16]! > + adr_this_cpu x0, kvm_hyp_ctxt, x1 > + ldr x0, [x0, #CPU_ELR_EL2] > + msr elr_el2, x0 > + ldp x0, x1, [sp], #16 > + > SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL) > // x2-x29,lr: vcpu regs > // vcpu x0-x1 on the stack > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index a038320cdb08..6a8dc8d3c193 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -747,7 +747,7 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) > > static inline void __kvm_unexpected_el2_exception(void) > { > - extern char __guest_exit_panic[]; > + extern char __guest_exit_panic_with_restored_elr[]; > unsigned long addr, fixup; > struct kvm_exception_table_entry *entry, *end; > unsigned long elr_el2 = read_sysreg(elr_el2); > @@ -769,7 +769,9 @@ static inline void __kvm_unexpected_el2_exception(void) > } > > /* Trigger a panic after restoring the hyp context. */ > - write_sysreg(__guest_exit_panic, elr_el2); > + write_sysreg(__guest_exit_panic_with_restored_elr, elr_el2); > + > + this_cpu_ptr(&kvm_hyp_ctxt)->sys_regs[ELR_EL2] = elr_el2; nit: placing the saving of ELR_EL2 before overriding the sysreg makes the whole thing a bit more readable, specially given the poor choice of 'elr_el2' as a variable (visually clashing with 'elr_el2' as a system register). Thanks, M. -- Without deviation from the norm, progress is not possible.