On Fri, May 10, 2024 at 12:26:33PM +0100, Pierre-Clément Tosi wrote:
> In invalid_host_el2_vect (i.e. EL2{t,h} handlers in nVHE guest context),
> remove the duplicate vCPU context check that __guest_exit_panic also
> performs, allowing an unconditional branch to it.
>
> Rename __guest_exit_panic to __hyp_panic to better reflect that it might
> not exit through the guest but will always (directly or indirectly) end
> up executing hyp_panic(). Fix its wrong (probably bitrotten) ABI doc to
> reflect the ABI expected by VHE and (now) nVHE.
>
> Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic().
>
> Restore x0, x1 before calling hyp_panic when __hyp_panic is executed in
> host context (i.e. called from __kvm_hyp_vector).
Please don't mix cosmetic changes with functional changes. It really
makes the reviewer's life more difficult, especially when the diff is
nearly all in asm!
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index bcaaf1a11b4e..6a1ce9d21e5b 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -83,7 +83,7 @@ alternative_else_nop_endif
> eret
> sb
>
> -SYM_INNER_LABEL(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL)
> +SYM_INNER_LABEL(__hyp_restore_elr_and_panic, SYM_L_GLOBAL)
> // x0-x29,lr: hyp regs
>
> stp x0, x1, [sp, #-16]!
> @@ -92,13 +92,15 @@ SYM_INNER_LABEL(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL)
> 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
> +SYM_INNER_LABEL(__hyp_panic, SYM_L_GLOBAL)
> + // x0-x29,lr: vcpu regs
> +
> + stp x0, x1, [sp, #-16]!
>
> // If the hyp context is loaded, go straight to hyp_panic
> get_loaded_vcpu x0, x1
> cbnz x0, 1f
> + ldp x0, x1, [sp], #16
> b hyp_panic
Aren't these new stack accesses unnecessary for the case where the vector
is valid?
>
> 1:
> @@ -110,10 +112,12 @@ SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL)
> // accurate if the guest had been completely restored.
> adr_this_cpu x0, kvm_hyp_ctxt, x1
> adr_l x1, hyp_panic
> - str x1, [x0, #CPU_XREG_OFFSET(30)]
> + str x1, [x0, #CPU_LR_OFFSET]
>
> get_vcpu_ptr x1, x0
>
> + // Keep x0-x1 on the stack for __guest_exit
Didn't these get pushed twice? Once by the valid_vect macro and then
again by your stp in __hyp_panic?
I feel like I must be missing something here, but I don't really see why
this patch is needed.
Will
