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

From: Will Deacon <will@kernel.org>
To: "Pierre-Clément Tosi" <ptosi@google.com>
Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Vincent Donnefort <vdonnefort@google.com>
Subject: Re: [PATCH v4 03/13] KVM: arm64: nVHE: Simplify __guest_exit_panic path
Date: Wed, 5 Jun 2024 17:02:56 +0100	[thread overview]
Message-ID: <20240605160256.GA22199@willie-the-truck> (raw)
In-Reply-To: <qob5gnca2nte4ggkrnn4uil5mfbkz3p55lmk3egpxstnumixfr@lq7xomrhf6za>

On Tue, Jun 04, 2024 at 04:48:02PM +0100, Pierre-Clément Tosi wrote:
> 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).

I think having a patch which removes the dead code and has the
unconditional branch to hyp_panic is the best thing here. It might
change later on in the series, but it's a sensible patch on its own and,
with assembly, I think having small incremental changes is the best
option.

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

Oh, sorry, I missed that you'd dropped the stp earlier on. So the SP doesn't
need any adjusting and we can just branch to hyp_panic after the overflow
check.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel