| Message ID | 20240529121251.1993135-4-ptosi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add support for hypervisor kCFI | expand |
On Wed, May 29, 2024 at 01:12:09PM +0100, Pierre-Clément Tosi wrote: > In invalid_host_el2_vect (i.e. EL2{t,h} handlers in nVHE guest context), *guest* context? Are you sure? > remove the duplicate vCPU context check that __guest_exit_panic also > performs, allowing an unconditional branch to it. > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > --- > arch/arm64/kvm/hyp/nvhe/host.S | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > 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. > - > /* > * 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? Will
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
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
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 - /* * 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 .L__hyp_sp_overflow\@: /* Switch to the overflow stack */
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. Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> --- arch/arm64/kvm/hyp/nvhe/host.S | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)