[PATCH v4 03/13] KVM: arm64: nVHE: Simplify __guest_exit_panic path

[Pierre-Clément Tosi]

Hi Will,

Thanks for the review; I will make sure to Cc you on v5, with your Acked-by.

On Mon, Jun 03, 2024 at 03:30:30PM +0100, Will Deacon wrote:
> On Wed, May 29, 2024 at 01:12:09PM +0100, Pierre-Clément Tosi wrote:
> > diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
> > index 135cfb294ee5..71fb311b4c0e 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/host.S
> > +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> > @@ -197,18 +197,13 @@ SYM_FUNC_END(__host_hvc)
> >  	sub	x0, sp, x0			// x0'' = sp' - x0' = (sp + x0) - sp = x0
> >  	sub	sp, sp, x0			// sp'' = sp' - x0 = (sp + x0) - x0 = sp
> >  
> > -	/* If a guest is loaded, panic out of it. */
> > -	stp	x0, x1, [sp, #-16]!
> > -	get_loaded_vcpu x0, x1
> > -	cbnz	x0, __guest_exit_panic
> > -	add	sp, sp, #16
> 
> I think this is actually dead code and we should just remove it. AFAICT,
> invalid_host_el2_vect is only used for the host vectors and the loaded
> vCPU will always be NULL, so this is pointless. set_loaded_vcpu() is
> only called by the low-level guest entry/exit code and with the guest
> EL2 vectors installed.

This is correct.

> > -
> >  	/*
> >  	 * The panic may not be clean if the exception is taken before the host
> >  	 * context has been saved by __host_exit or after the hyp context has
> >  	 * been partially clobbered by __host_enter.
> >  	 */
> > -	b	hyp_panic
> > +	stp	x0, x1, [sp, #-16]!
> > +	b	__guest_exit_panic
> 
> In which case, this should just be:
> 
> 	add	sp, sp, #16
> 	b	hyp_panic
> 
> Did I miss something?

Jumping to hyp_panic directly makes sense.

However, this patch keeps jumping to __guest_exit_panic() to prepare for the
kCFI changes as having a single point where all handlers (from various vectors)
panicking from assembly end up before branching to C turns out to be very
convenient for hooking in the kCFI handler (e.g.  when saving the registers, to
be parsed from C). I also didn't want to modify the same code twice in the
series and found it easier to limit the scope of this commit to a minimum by
following the existing code and keeping the same branch target.

With this in mind, please confirm if you still prefer this fix to jump to
hyp_panic directly (knowing the branch will be modified again in the series).

Also, I don't get why the 'add sp, sp, #16' is needed; what is it undoing?

Thanks,

Pierre