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
