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
